symbol-clip: dynamic filter style spec changes #10977

arindam1993 · 2021-08-30T22:13:49Z

This PR adds the necessary changes in the style-spec to account for dynamic filters ( filters with ["pitch"] and ["distance-from-center"]). The diff looks a bit weird because its a diff against the symbol-clip branch which is the branch the original prototype was built in using the symbol-clip layout property.

This support is only being added to symbol-layers so, to communicate that this PR adds some new root-level properties in the style-spec v8.json.

These are root level properties with keys formatted like filter_${layerType}.
The value for these new entries is an expressionSpec object, exactly like the expression spec objects used to declare expression compatibility for paint/layout properties.
The updated style-spec validation code uses these new objects for filter validation, so there is a slight change in the formatting of error messages. 450ca3f and c8d7ed5 show some of the new error messages.

…iplication function

debug/index.html

rreusser

Mainly some wording and doc touchups, but overall looking good. Excited for this feature. 🎉

debug/index.html

src/style-spec/reference/v8.json

test/unit/style-spec/fixture/filters-dynamic-distance.output-api-supported.json

SnailBones · 2021-09-21T16:25:57Z

src/style-spec/feature_filter/index.js

            needGeometry
        };
+    } else {
+        // This branch cannot happen but need to keep flow happy :(


It seems like Flow is dictating the structure of this whole section at the expense of readability!
Would Flow be ok with a if (!filterFunc) {return;} so that you could unwrap the main return statement?
Or could you make filterFunc a const and use a try / catch?

Nope, flow cannot detect early returns statically.

I did a force cast instead, looks slightly cleaner.

SnailBones · 2021-09-21T20:11:03Z

src/style-spec/reference/v8.json

@@ -652,7 +652,7 @@
    },
    "filter": {
      "type": "filter",
-      "doc": "A expression specifying conditions on source features. Only features that match the filter are displayed. Zoom expressions in filters are only evaluated at integer zoom levels. The `feature-state` expression is not supported in filter expressions."
+      "doc": "An expression specifying conditions on source features. Only features that match the filter are displayed. Zoom expressions in filters are only evaluated at integer zoom levels. The `feature-state` expression is not supported in filter expressions.  The `['pitch']` and `['distance-from-center']` expressions are supported only for filter expressions on the `symbol` layer."


Suggested change

"doc": "An expression specifying conditions on source features. Only features that match the filter are displayed. Zoom expressions in filters are only evaluated at integer zoom levels. The `feature-state` expression is not supported in filter expressions. The `['pitch']` and `['distance-from-center']` expressions are supported only for filter expressions on the `symbol` layer."

"doc": "An expression specifying conditions on source features. Only features that match the filter are displayed. [`zoom`](#zoom) expressions in filters are only evaluated at integer zoom levels. The [`feature-state`](#feature-state) expression is not supported in filter expressions. The [`pitch'](#pitch) and [`distance-from-center'`](distance-from-center) expressions are supported only for filter expressions on the [`symbol`](/mapbox-gl-js/style-spec/layers/#symbol) layer."

This is a good idea but i don't think we should do this manually, instead the docs codegen system should handle this, that way if the structure of the docs website changes it can account for it.

That sounds like a good idea to me!

SnailBones · 2021-09-21T20:25:57Z

src/style-spec/reference/v8.json

@@ -2524,6 +2500,72 @@
    "value": "*",
    "doc": "A filter selects specific features from a layer."
  },
+  "filter_symbol": {
+    "type": "boolean",
+    "doc": "Expression which determines whether or not to display a symbol. Symbols support `dynamic-filtering`, meaning this expression can use the `[\"pitch\"]` and `[\"distance-from-center\"]` expressions to reference the current state of the view.",


Nit:
Is dynamic-filtering a string that users will need to type? If not, I think should just be written in plain text as dynamic filtering.

I'm noticing that in the style-spec expressions are inconsistently formatted as: expression-name, 'expression-name', ["expression-name"], or [\"expression-name\"]. I'd love to hear from the docs team on this.

SnailBones · 2021-09-21T20:28:49Z

src/style-spec/reference/v8.json

@@ -4713,9 +4755,6 @@
      "doc": "Defines a gradient with which to color a line feature. Can only be used with GeoJSON sources that specify `\"lineMetrics\": true`.",
      "transition": false,
      "requires": [
-        {
-          "!": "line-dasharray"


Good catch!

SnailBones · 2021-09-22T01:39:22Z

src/style-spec/reference/v8.json

+  },
+  "filter_heatmap": {
+    "type": "boolean",
+    "doc": "Expression used to determine whether a point is being displayed or not. Heatmap layer does NOT support `dynamic-filtering`, meaning this expression can NOT use the `[\"pitch\"]` and `[\"distance-from-center\"]` expressions to reference the current state of the view.",


Rephrase for consistency

SnailBones · 2021-09-22T01:40:33Z

src/style-spec/reference/v8.json

@@ -3183,6 +3225,24 @@
          }
        }
      },
+      "pitch": {
+        "doc": "Gets the current pitch in degrees. `[\"pitch\"]` may only be used in the `filter` expression for a `symbol` layer.",


Nit: consider "Returns" instead of "Gets"

This is consistent with the wording for ["zoom"], do you think we should change both?

Yes, I think "Returns" is slightly more clear.

SnailBones · 2021-09-22T01:46:05Z

src/style-spec/validate/validate_expression.js

+
+    if (e instanceof CompoundExpression) {
+        if (disallowedParameters.has(e.name)) {
+            return [new ValidationError(options.key, options.value, `["${e.name}"] expression is not supported in a filter for a ${options.object.type} layer with id: ${options.object.id}`)];


Stylistic question here! Do we prefer to put the correct-use-case return in a nested value and error returns (or throws) at the end? I usually do it the other way around. In any case I'd like to be consistent!

This applies to line 25 too.

I don't think we have a hard rule. I think doing it a way that makes it easier to read a particular function is better. I personally like error handling/validation at the top and doing the meat of the logic after.

SnailBones · 2021-09-22T01:55:03Z

Great work @arindam1993! I have a few nits about code styling and documentation syntax.
A nice-to-have would be more links in the documentation.
I think that we should get some eyes from the docs team on this. Do you think it's best to do that in one go after it's merged into the symbol-clip branch?

src/style-spec/reference/v8.json

arindam1993 · 2021-09-28T17:38:27Z

Great work @arindam1993! I have a few nits about code styling and documentation syntax. A nice-to-have would be more links in the documentation. I think that we should get some eyes from the docs team on this. Do you think it's best to do that in one go after it's merged into the symbol-clip branch?

If we consistently name expressions, I think its something the docs-page-generation tooling should handle. That way it can account for changes to the website hierarchy without that having to be hardcoded in here.

SnailBones · 2021-09-28T18:37:41Z

Just tested this with the docs and it seems to be breaking.

types.js:40 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'sort')
    at types.js:40
    at Array.map (<anonymous>)
    at Module.T1xj (types.js:37)
    at __webpack_require__ (bootstrap:84)
    at Module.flda (mapbox-gl-js-api~mapbox-gl-js-api-events~mapbox-gl-js-api-geography~mapbox-gl-js-api-handlers~mapbox~559181ac.chunk.js:6205)
    at __webpack_require__ (bootstrap:84)
    at Module.1wO5 (mapbox-gl-js-api~mapbox-gl-js-api-events~mapbox-gl-js-api-geography~mapbox-gl-js-api-handlers~mapbox~559181ac.chunk.js:694)
    at __webpack_require__ (bootstrap:84)
    at Object.ZwM2 (query-terrain-elevation.md:22)
    at __webpack_require__ (bootstrap:84)

EDIT: This starts with this commit. Not directly caused by the changes here.

src/style-spec/reference/v8.json

Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com>

…rom-camera` expressions (#10795) * Setup plumbing for pitch and distance-from-camera expressions * Add symbol-clip with support for pitch and distance-from-camera expressions * hackily working pitch expression * distance clipping * Track clipped state in JointPlacement and JointOpacity, reflect clipping using notUsed state in debug buffers * Rename distance-from-camera to distance-from-center * symbol-clip: splitting filter expressions into dynamic and static parts (#10923) * symbol-clip: dynamic filter style spec changes (#10977) * symbol-clip: Render tests, debug page and distance matrix rework (#11065) * WIP dynamic filter splitting stage 1 * Add tests for isDynamicFilter * More test cases for isDynamicFilter * Existing static filters get passed through unmodified. * WIP extracting static filters * Working case -> any translation * More tests for case -> any conversion * Add support for match branches * WIP: V0 of adding collapsing of dynamic expressions to true * more test cases * tests and some more refactoring * remove temory inspection code from unit tests * Fix dynamic filter detection * Fix failing spec test * Units tests hopefully :green: now * Add test to cover for `null` in expressions * Address CR comments and reduce number of temporary allocations * remove gl-matrix as dependency of style-spec and inline a matrix multiplication function * Fix lint issues * Add better error messages for expression compilation failures * Ensure location parameter is passed down * Remove symbol-clip from the style-spec * Remove unused property-expression type * Move filterSpec to v8.json and replace symbol-clip with dynamic filter * Fix unit tests * Fix most flow errors * finally flow is happy * Add expression validation code * Add api-supported tests for validation * Fix some failing unit tests * Ensure layer._featureFIlter is updated on main thread as well * Ensure layerType is passed down to validation * Fix flow and linting * Fix unit tests * Pass 1 of addressing CR comments * Add basic placementTime metric * Move to using total placement time * Fix lint errors * simplify placement time tracking * Fix silent conflict in with SymbolInstanceStruct changes * try benchmarking with filter splitting algorithm removed * Fix versions microbenchmark page * Revert "try benchmarking with filter splitting algorithm removed" This reverts commit 865f354. * Simplify by calculating distance using tile coordinates of the symbol directly * Use new specific centerDistanceMatrix * Sign flipping for consistency * add new debug page with distance visualizer * First set of pure distance based render tests * Pitch thresholding tests * Fix lint errors * More tests * Add first batch of point symbol render tests * Increase threshold * Increase allowed threshold for the correct tests 🤦 * remove flaky collision debug boxes instead * Move geojson test data to be in separate files instead of inlined into styles * Update flaky render test expectation * Fix distanceMatrix comment Co-authored-by: Aidan H <aidan.hendrickson@mapbox.com> * Switch to linear scale and update line placement tests * Update point placement tests * Update test expectations * Fix lint error * Remove flaky collision boxes Co-authored-by: Aidan H <aidan.hendrickson@mapbox.com> * Remove frametime logging change * Ensure that feature deserialization happens only when needed * Fix matrixKey naming leftover from copying fogMatrix code Co-authored-by: Karim Naaji <karim.naaji@gmail.com> * Rename matrices as per CR comments * Default layerType inside filter validation function. * add `VectorTileFeature` deserialization cache * Switch to matrix-free distance calculation * Add cache in Transform * Precompute bearing vector * prescale by windowScaleFactor * Inline `getSymbolFeature` * Lazy filter compilation * Move distance matrix calculation out to the debug page Co-authored-by: Aidan H <aidan.hendrickson@mapbox.com> Co-authored-by: Karim Naaji <karim.naaji@gmail.com>

Arindam Bose added 30 commits July 26, 2021 17:31

WIP dynamic filter splitting stage 1

2969f28

Add tests for isDynamicFilter

b4689b9

More test cases for isDynamicFilter

ff60896

Existing static filters get passed through unmodified.

24fd71c

WIP extracting static filters

b66e22c

Working case -> any translation

c68f159

More tests for case -> any conversion

4bdf7a1

Add support for match branches

a609a4d

WIP: V0 of adding collapsing of dynamic expressions to true

aa8cc2b

more test cases

e18336f

tests and some more refactoring

035ec77

remove temory inspection code from unit tests

495b279

Fix dynamic filter detection

eac58f6

Fix failing spec test

81cdbc8

Units tests hopefully :green: now

3c2f2d3

Add test to cover for null in expressions

4bee8da

Address CR comments and reduce number of temporary allocations

0d8d809

remove gl-matrix as dependency of style-spec and inline a matrix mult…

e015ec5

…iplication function

Fix lint issues

53c644e

Add better error messages for expression compilation failures

2a063b0

Ensure location parameter is passed down

d4e9ac3

Remove symbol-clip from the style-spec

0265792

Remove unused property-expression type

80a52d7

Move filterSpec to v8.json and replace symbol-clip with dynamic filter

ea727a5

Fix unit tests

b0533d8

Fix most flow errors

2f8ed09

finally flow is happy

566b59c

Add expression validation code

450ca3f

Add api-supported tests for validation

c8d7ed5

Fix some failing unit tests

a121924

Fix flow and linting

9bb07a5

arindam1993 marked this pull request as ready for review September 15, 2021 17:07

Fix unit tests

6a9defa

arindam1993 requested a review from rreusser September 15, 2021 19:57

avpeery reviewed Sep 15, 2021

View reviewed changes

debug/index.html Outdated Show resolved Hide resolved

rreusser suggested changes Sep 15, 2021

View reviewed changes

Pass 1 of addressing CR comments

8e6bd6e

avpeery reviewed Sep 20, 2021

View reviewed changes

test/unit/style-spec/fixture/filters-dynamic-distance.output-api-supported.json Show resolved Hide resolved

SnailBones reviewed Sep 21, 2021

View reviewed changes

SnailBones reviewed Sep 22, 2021

View reviewed changes

rreusser reviewed Sep 28, 2021

View reviewed changes

src/style-spec/reference/v8.json Outdated Show resolved Hide resolved

Arindam Bose added 2 commits September 28, 2021 10:26

Address review comments

5189a1a

cast through any to clean up

e99fe09

rreusser reviewed Sep 29, 2021

View reviewed changes

src/style-spec/reference/v8.json Outdated Show resolved Hide resolved

rreusser reviewed Sep 29, 2021

View reviewed changes

src/style-spec/reference/v8.json Outdated Show resolved Hide resolved

rreusser approved these changes Sep 29, 2021

View reviewed changes

Arindam Bose and others added 2 commits September 29, 2021 13:48

backticks 1

ce2ace4

Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com>

backticks 2

13ffe33

Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com>

arindam1993 merged commit 0d3b8d5 into symbol-clip Sep 29, 2021

arindam1993 deleted the dynamic-filter/style-spec-changes branch September 29, 2021 21:51

arindam1993 mentioned this pull request Oct 1, 2021

s̶y̶m̶b̶o̶l̶-̶c̶l̶i̶p̶ dynamic-filtering with pitch and distance-from-camera expressions #10795

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

symbol-clip: dynamic filter style spec changes #10977

symbol-clip: dynamic filter style spec changes #10977

arindam1993 commented Aug 30, 2021 •

edited

Loading

rreusser left a comment

SnailBones Sep 21, 2021

arindam1993 Sep 28, 2021

arindam1993 Sep 28, 2021

SnailBones Sep 21, 2021

arindam1993 Sep 28, 2021

SnailBones Sep 28, 2021

SnailBones Sep 21, 2021 •

edited

Loading

SnailBones Sep 21, 2021

SnailBones Sep 22, 2021

SnailBones Sep 22, 2021

arindam1993 Sep 28, 2021

SnailBones Sep 28, 2021

SnailBones Sep 22, 2021

arindam1993 Sep 28, 2021

SnailBones commented Sep 22, 2021

arindam1993 commented Sep 28, 2021

SnailBones commented Sep 28, 2021 •

edited

Loading

symbol-clip: dynamic filter style spec changes #10977

symbol-clip: dynamic filter style spec changes #10977

Conversation

arindam1993 commented Aug 30, 2021 • edited Loading

rreusser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SnailBones Sep 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SnailBones commented Sep 22, 2021

arindam1993 commented Sep 28, 2021

SnailBones commented Sep 28, 2021 • edited Loading

arindam1993 commented Aug 30, 2021 •

edited

Loading

SnailBones Sep 21, 2021 •

edited

Loading

SnailBones commented Sep 28, 2021 •

edited

Loading