Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 25 commits into from Oct 7, 2021

Conversation

arindam1993
Copy link
Contributor

@arindam1993 arindam1993 commented Jun 22, 2021

Lots changed with the design and implementation of this since I first started this, leaving behind the original post under ----------OLD------------- for documentation.

The first part(#10923) was changing it so the user doesn't have to write their own separate filter and symbol-clip expressions. Instead this allows the user to write one single filter expression that then gets split during the filter compilation into separate filter and dynamicFilter expressions.

The filter part is evaluated in the worker just like before, whereas the dynamicFilter part is evaluated continuously during placement.

The next part was adding all of this new filter validation logic to the style-spec package(#10977)

The last part was tweaking the distance metric slightly and making it slightly different from fog (#11065).
Screen Shot 2021-10-01 at 12 17 28 PM
Instead of being a distance measured from a point, its now a distance measured from a line passing through the center, perpendicular to the bearing.

@mapbox/map-design-team @mapbox/gl-native

------------------ OLD-------------------------

This PR adds a new boolean spec-property for the symbol layer that lets the user define clipping constraints based on the current state of the camera.
When this property is true for a given symbol, the symbol gets hidden, and when its false it gets displayed. Thus to keep it backwards compatible is false by default.

The primary goal of this is to be able to control-ably reduce symbol density in the far distance when the map is pitched.
With this in mind this spec property:

  • Supports data-driven styling

  • Supports zoom , pitch and distance-from-camera expressions

  • Has NO restrictions on where in the expression zoom, pitch and distance-from-camera can appear (Currently we only support zoom as a top-level param to an interpolate expression)

  • pitch expression returns the pitch in degrees

  • distance-from-camera expression returns the distance of the symbol from the camera in the same viewport size normalized units that are used for near and far distance measurements for fog.

Demo:

With the following code:

map.setLayoutProperty('building-number-label', 'symbol-clip',
    ["case",
        ["<", ["pitch"], 60], false, // Don't clip any symbols when pitch is LOW
        ["all", [">=", ["pitch"], 60], ["<", ["distance-from-camera"], 2]], false, // When pitch is HIGH, don't clip symbols near to camera
        true // Otherwise hide the symbol (because its far-away, and pitch is high)
    ]
    );

    map.setLayoutProperty('poi-label', 'symbol-clip',
        ["case",
            ["<", ["pitch"], 60], false,
            ["all", [">=", ["pitch"], 60], ["<", ["distance-from-camera"], 4]], false,
            true
        ]
    );

    map.setLayoutProperty('transit-label', 'symbol-clip',
    ["case",
        ["<", ["pitch"], 60], false,
        ["all", [">=", ["pitch"], 60], ["<", ["distance-from-camera"], 3]], false,
        true
    ]
    );
Screen.Recording.2021-06-07.at.3.17.27.PM.mov

This hides all POI labels in the far-distance, and lets street labels show through. This works because clipping takes place before collisions, thus the screen real-estate freed manually up by carefully crafted constraints can be filled with more pertinent information automatically by the placement engine.

How this works

This expression is evaluated on the main thread during placement , if the symbol needs to be clipped it early-exits and completely skips placing it. For a collision box debug-view rendering it gets treated it as an "unused" symbol, same as rejected placement locations for text-variable-anchor based symbols are treated.

  • Box sizing, and projection logic on the main thread is skipped
  • Insertion into collision grid and collision evaluation for it is skipped.
  • They still get drawn with 0 opacity overdraw just like symbols that are hidden due to collision.

What about performance?

Still very early, but framerate for the case I described above in the Demo seems much better. At high pitch viewport-aligned symbols at a far distance overlap a LOT and that places a lot of load on the collision evaluation. This skips all of that work.
A particularly bad case:
Before:
Screen Shot 2021-06-21 at 8 49 46 PM

After:
Screen Shot 2021-06-21 at 8 50 40 PM

Launch Checklist

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog> Add the ability to use ["pitch"]and["distance-from-center"]expressions infilterfield ofsymbol layers. </changelog>

@arindam1993
Copy link
Contributor Author

This Observable hosts a playground for prototyping styles with this prop

https://observablehq.com/@arindam-mapbox/symbol-clipping-playground

Arindam Bose and others added 3 commits September 29, 2021 14:51
)

* 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>
@arindam1993 arindam1993 changed the title [WIP] symbol-clip with pitch and distance-from-camera expressions ~symbol-clip~ dynamic-filtering with pitch and distance-from-camera expressions Oct 1, 2021
@arindam1993 arindam1993 marked this pull request as ready for review October 1, 2021 22:51
@SnailBones
Copy link
Contributor

SnailBones commented Oct 1, 2021

I like the line-based distance metric!
Can you provide more context on the example? Should I be entering expressions here? Do you have some example expressions that might be common use cases?

@arindam1993 arindam1993 changed the title ~symbol-clip~ dynamic-filtering with pitch and distance-from-camera expressions s̶y̶m̶b̶o̶l̶-̶c̶l̶i̶p̶ dynamic-filtering with pitch and distance-from-camera expressions Oct 4, 2021
src/geo/transform.js Outdated Show resolved Hide resolved
src/geo/transform.js Outdated Show resolved Hide resolved
src/geo/transform.js Outdated Show resolved Hide resolved
src/style/style.js Outdated Show resolved Hide resolved
src/symbol/placement.js Outdated Show resolved Hide resolved
src/style-spec/expression/index.js Show resolved Hide resolved
src/symbol/placement.js Outdated Show resolved Hide resolved
src/geo/transform.js Outdated Show resolved Hide resolved
src/style-spec/expression/evaluation_context.js Outdated Show resolved Hide resolved
src/geo/transform.js Outdated Show resolved Hide resolved
src/geo/transform.js Outdated Show resolved Hide resolved
@arindam1993
Copy link
Contributor Author

Ran the full-loading benchmarks and seeing a ~1.6% slowdown in loadTime.
The diagnostic metrics are a bit confusing because it shows an increase in the workerIdle part, which doesnt make sense for the changes in this PR.
Maybe its because of importing v8.json directly which might be causing an increase in the bundle size.

Screen Shot 2021-10-05 at 5 59 44 PM

@arindam1993
Copy link
Contributor Author

Ok, I've been able to get the load times much more under control by hackily removing the duplication of the filterSpec in v8.json.
Screen Shot 2021-10-06 at 8 31 40 AM

But that also removes the metadata studio would like, trying to think of a solution.

src/geo/transform.js Outdated Show resolved Hide resolved
src/geo/transform.js Outdated Show resolved Hide resolved
@arindam1993
Copy link
Contributor Author

ran one more benchmap run, looks like the latest commit with the lazy filter compilation actually helps much more with load times.
Screen Shot 2021-10-06 at 7 10 29 PM

Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @arindam1993 ! LGTM 🟢

@arindam1993 arindam1993 merged commit cb4778f into main Oct 7, 2021
@arindam1993 arindam1993 deleted the symbol-clip branch October 7, 2021 16:52
katydecorah pushed a commit that referenced this pull request Oct 20, 2021
* main:
  Add touch pan blocker to gesture handling for touch devices (#11116)
  Address accessibility issues (#11064)
  add support for non-mercator projections (#11124)
  Image fallback expressions within paint properties (#11049)
  Replaces EPSG:4326 with OGC:CRS84 in GL JS `LngLat` doc (#11072)
  Add globe view support to heatmap shaders (#11120)
  Exclude flaky test (#11118)
  consistify YOUR_MAPBOX_ACCESS_TOKEN as placeholder string (#11113)
  Allow adding multiple layers to `map.on()` event handler (h/t @omerbn) (#11114)
  render-test-flakiness:clear worker storage (#11111)
  upgrade to supercluster v7.1.4, earcut v2.2.3, vt-pbf v3.1.3, geojson-rewind v0.5.1 (#11110)
  Added v1.13.2 changelog entry (#11108)
  One weird JSON.parse() trick (#11098)
  Fixed doc usage of map.getCenter (#11093)
  s̶y̶m̶b̶o̶l̶-̶c̶l̶i̶p̶ dynamic-filtering with `pitch` and `distance-from-camera` expressions (#10795)
  Update link to transpiling guide (#11096)
  Cherry pick 2.5.1 changelog (#11099)
  Fix an iOS15 issue where Safari tab bar interrupts panning (#11084)
  Fix conditional check for isFullscreen to accommodate Safari (#11086)
  Render tests for #11041 (#11070)
@CptHolzschnauz
Copy link

VERY nice feature but i can't get it to run, not even on your playground:

properties.js:374 Uncaught TypeError: Cannot read properties of undefined (reading 'value')
at gs.getValue (properties.js:374)
at Lh.getLayoutProperty (style_layer.js:124)
at je.setLayoutProperty (style.js:1050)
at Map.setLayoutProperty (map.js:2348)
at Map. (map56.html:1346)
at Map.fire (evented.js:129)
at Map._render (map.js:2911)
at map.js:3216

Can you add a example (Full code or include it somewhere in the Mapbox examples) ? Thx

@SnailBones
Copy link
Contributor

@CptHolzschnauz We're currently putting together some examples, they'll be live with our launch within a few days.

@CptHolzschnauz
Copy link

CptHolzschnauz commented Nov 11, 2021

Thx. Whatever i try, i can't see any filter effect on my layer. This amazing feature deserves a fully working example.

@CptHolzschnauz
Copy link

If someone finds this thread:
https://docs.mapbox.com/mapbox-gl-js/example/switch-symbol-style-pitch/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants