Tile ArcGis FeatureServer requests #7370
Conversation
|
It might be of interest to look into the layer mentioned in #6840. That layer is failing with
|
|
|
…olisation support
…ings for unsupported ESRI drawingInfo, and add skeleton column (traits) when tiling
|
There is a bug I haven't been able to fix. It occurs in the 3D (cesium) viewer, with layers that have a minimum zoom level, when you create a share link with a picked feature You get this error 
|
| (colorColumn && | ||
| !colorTraits.mapType && | ||
| colorColumn.type === TableColumnType.scalar) || | ||
| colorTraits.mapType === "continuous" || | ||
| colorTraits.mapType === "bin" |
There was a problem hiding this comment.
Loosen condition here, as we need to allow Discrete/Continuous color maps when colorColumn is undefined (eg empty)
There was a problem hiding this comment.
Nitpick: is there a scope to make the intent more clear for the condition? One possibility might be to assign first to a temporarily variable like: const isScalarColorColumn = colorColumn?.type === TableColumnType.scalar.
| ): GeomType | null => { | ||
| switch (type) { | ||
| case "Point": | ||
| case "MultiPoint": |
There was a problem hiding this comment.
Does the input for this function ever come from the code updated in #7432, or is that a separate code path?
There was a problem hiding this comment.
Most likely yes, but depends on a few other factors
There was a problem hiding this comment.
I'm not sure if your yes means that the input comes from the other code or if it is a separate code path, but thank you for merging the PR.
| const geom = f.geometry as any as [number, number][][]; | ||
| transformedGeom = geom.map((g1) => | ||
| g1.map((g2) => { | ||
| const x = g2[0]; |
There was a problem hiding this comment.
Is this function body identical to the function body in the else-branch on line 271?
There was a problem hiding this comment.
Yes, but I think it is fine. IMO generally not worth refactoring duplication out unless there are more then 2 instances
| let transformedGeom: Point[][] = []; | ||
| let numVertices = 0; |
There was a problem hiding this comment.
I don't think these two need to be initialized (looks like they are assigned in all code paths below).
There was a problem hiding this comment.
Strictly speaking no, but they make sense as default values, so i'm going to leave them there
There was a problem hiding this comment.
@nf-s - this is an awesome feature improvement. Great work on the implementation 🎉
I have left some comments in the code review plus a couple of other observations:
-
I noticed in some cases, the column values differ between protomap and geojson rendering modes. This affects the symbology. Not sure if it is easily fixable though.
protomap:
geojson:
-
Not something from this PR but I think the second parameter here should be
this._featureServer. This for example affects the behaviour when splitting an item for compare.
| `WARNING: Terria only supports ArcGisFeatureService UniqueValueRenderers with a single field (\`field1\`), not multiple fields (\`field2\` or \`field3\`)` | ||
| ); | ||
| } | ||
|
|
||
| if (uniqueValueRenderer.visualVariables?.length) { | ||
| console.warn( | ||
| `WARNING: Terria does not support visual variables in ArcGisFeatureService SimpleRenderers` |
There was a problem hiding this comment.
Small typo: should this say UniqueValueRenderers instead of SimpleRenderers? Same for classbreaks
| @@ -315,6 +388,12 @@ class FeatureServerStratum extends LoadableStratum( | |||
| const simpleRenderer = renderer as SimpleRenderer; | |||
| const symbol = simpleRenderer.symbol; | |||
|
|
|||
There was a problem hiding this comment.
Suggestion: is it worth splitting styling and associated functions into a separate Stratum and file?
| supportedQueryFormats?: string; | ||
| maxRecordCount?: number; | ||
| tileMaxRecordCount?: number; | ||
| maxRecordCountFactor?: number; | ||
| supportsCoordinatesQuantization?: boolean; | ||
| supportsTilesAndBasicQueriesMode?: boolean; | ||
| objectIdField?: string; | ||
| fields?: Field[]; |
There was a problem hiding this comment.
Like for the other types, is there a spec link for FeatureServer we can point to in comment?
| // Not sure how to handle request here - as we are making multiple requests | ||
| // request: request |
There was a problem hiding this comment.
Would it work if we clone the request? AFAICT, for imagery layers, it only sets some flags at this stage in the code path. Cancellation might work even without passing request as it seems to create a new Request by default.
| abstract mapValueToColor( | ||
| value: string | number | null | undefined | ||
| ): Readonly<Color>; | ||
| abstract mapValueToColor(value: JsonValue | undefined): Readonly<Color>; |
There was a problem hiding this comment.
Doubt: is this change required? Most implementations of ColorMap assumes the param to be string | .... Also the JsonValue imported here comes from protomap-leaflet
| MappableMixin.isMixedInto(catalogItem) | ||
| ? catalogItem.showStringIfPropertyValueIsNull | ||
| : undefined | ||
| currentTime, |
There was a problem hiding this comment.
Doubt: Should we do currentTime ?? JulianDate.now() instead so that it is always current and not the time when the closure was generated.
| export function isLine(json: any): json is Feature<LineString> { | ||
| return ( | ||
| isJsonObject(json, false) && | ||
| json.type === "Feature" && | ||
| isJsonObject(json.geometry, false) && | ||
| json.geometry.type === "LineString" | ||
| ); | ||
| } | ||
|
|
||
| export function isMultiLineString(json: any): json is Feature<MultiLineString> { |
There was a problem hiding this comment.
At some point maybe we should probably extract these functions to a new file. I have found them useful in other contexts.
| (colorColumn && | ||
| !colorTraits.mapType && | ||
| colorColumn.type === TableColumnType.scalar) || | ||
| colorTraits.mapType === "continuous" || | ||
| colorTraits.mapType === "bin" |
There was a problem hiding this comment.
Nitpick: is there a scope to make the intent more clear for the condition? One possibility might be to assign first to a temporarily variable like: const isScalarColorColumn = colorColumn?.type === TableColumnType.scalar.
| "imageryProvider" in d && | ||
| d.imageryProvider instanceof ProtomapsImageryProvider |
There was a problem hiding this comment.
Nitpick: There's also ImageryParts.is() for this test
|
On the cesium error, I think it is happening when the One fix we could do at our end is to wrap these setTrait calls within |
Tile ArcGis FeatureServer requests
Fixes #3661 #7331 #6840
Merge after #7144Changes
Main breaking changes
Add tiling support to
ArcGisFeatureServerCatalogItem- this will be enabled by default if the server supports tiling and unsupported marker/point styles aren't used. SeeArcGisFeatureServerCatalogTraitstileRequests. When enabled,pbftiles will be fetched and drawn usingProtomapsImageryProvidertileRequeststofalsePointSymbolTraitsare available - fill, stroke, radius (which uses height). Custom markers (markers other thanpointorcircle) are not supported.Other changes
requestparameter toArcGisImageServerImageryProvider.buildImageResource- this enables Cesium to manage requestsProtomapsImageryProviderto use a "soft" minimum level, so no tiles will be created below theminimumZoomprovided.ProtomapsImageryProvider.pickFeaturesGeoJSON logic to insideProtomapsGeojsonSource.pickFeatures.FeatureInfoUrlTemplateMixinreactivity warningsGeojsonMixinprotomaps paint/label rules totableStyleToProtomaps.getStyleReactiveDependenciesthat can be used to track (and react to) table styling traitsdashtoOutlineStyleTraits(only supported by line features), andoutlineStyletoLegendTraits.MinMaxLevelMixintoArcGisFeatureServerCatalogItem- only applied when tiling requestsTableStyleMapandTableColorMapconditions to handle emptyTableColumns(to support stylingArcGisFeatureServerCatalogItemwhen tiling requests)MultiPolygonfeatures were being droppedTodo
can we filter out null features in the request?ProtomapsArcGisPbfSourcerequestto image servertableStyleToProtomapsnew properties - dash/color/style/width + point featuresMake sure requests are cancelled when moving cesium camera aroundThis is harder than expected, so will leave unless it seems necessary laterThis is a problem for larger layers, need to at least cancel requests when layers are turned off or removed from the wokrbenchThis is a big issue, but I can't find an easy way to do this. Not for this release, will make a ticket. This ties into larger issues of large network requests crashing Terria. After testing many layers, I haven't been able to crash Terria, but sometimes it takes a while for layer requests to finish, even if you have removed it from the workbenchAdd support for marker icons...Not for this release, will make a ticketMinMaxLevelMixinmixinFix bug where tiles are reloaded everytime the style changes (eg edit style)leaving this bug due to lack of time - will create a ticketFix share bug with minmaxlevelsee Tile ArcGis FeatureServer requests #7370 (comment) - leaving this bug due to lack of time - will create a ticketTest me
Checklist
doc/.