port CollisionIndex and Placement changes from -native #5890

ansis · 2017-12-19T01:17:40Z

This ports changes to CrossTileSymbolIndex and Placement from -native (mapbox/mapbox-gl-native#10436) back to -js.

Previously symbol opacities were stored directly on SymbolInstances. Whenever a tile was added or removed CrossTileSymbolIndex would do a bunch of calculations and copy these opacities directly between SymbolInstances, which it would hold references to. The approach was really stateful. The new approach untangles things into four more distinct steps:

match symbols across tiles and assign matching crossTileIDs to matching symbols (CrossTileIndex)
calculate boolean placement values for each crossTileID in the current set of tiles (Placement.placeLayer)
combine these boolean placement values with the previous values to calculate opacities (Placement.commit)
write these placement and opacity values to the buffers of current tiles by matching based on crossTileID (Placement.updateLayerOpacities)

A stale Placement can be used to update newly loaded buckets. This lets us avoid doing a full, expensive placement on every frame where a tile gets loaded. It uses the old values and updates them after a new Placement (split over several frames) has been produced.

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
document any changes to public APIs
post benchmark scores https://bl.ocks.org/anonymous/raw/f5ce771d18384774bead7f8a06f1af7f/
The most critical benchmark for these changes is Paint and the results look good.
manually test the debug page
manually test a setData symbol example

I still need to finish writing new tests but the pr is at the point where it could be reviewed.

ChrisLoer · 2017-12-19T02:21:36Z

Awesome! I'm just starting to work through the PR, should get more comments in tomorrow.

It appears to me that this fixes part of #5887 (in that tiles never "lose" their collision index because they don't hold references to the collision index any more), but maybe makes more of a problem in the case when a FeatureIndex changes during tile reload? Because we may now keep using the old collision index for many frames (until the next placement happens), it could be more problematic if the indices in the collision index don't match up with the feature index. I wonder if it might even trigger an error if the new feature index has fewer features and filterMatching indexes off the end of the array...

ansis · 2017-12-19T17:25:09Z

I wonder if it might even trigger an error if the new feature index has fewer features and filterMatching indexes off the end of the array...

Yes... I think so. Would adding a bucketInstanceId check be good enough? In the case of a bucket update and stale placement it would return nothing.

ChrisLoer · 2017-12-19T22:14:48Z

src/style/placement.js

@@ -1,25 +1,26 @@
 // @flow


Can we rename this file to pauseable_placement.js so the name doesn't collide with src/symbol/placement.js?

ChrisLoer · 2017-12-19T22:50:17Z

Haven't figured out the problem yet, but just panning around I see sometimes placement doesn't seem to finish (?) until some later map interaction:

Also on zooming out I'll see symbols disappear:

(This is running against 7bb3d45)

ChrisLoer · 2017-12-19T23:08:29Z

src/symbol/placement.js

+
+                let placeText = false;
+                let placeIcon = false;
+                //let offscreen = true;


Is the plan to hook this back up later?

ChrisLoer · 2017-12-19T23:23:59Z

src/symbol/cross_tile_symbol_index.js


 import type {OverscaledTileID} from '../source/tile_id';
 import type {SymbolInstance} from '../data/bucket/symbol_bucket';
+import type SymbolBucket from '../data/bucket/symbol_bucket';
+import type StyleLayer from '../style/style_layer';
+import type Tile from '../source/tile';

 /*
    The CrossTileSymbolIndex generally works on the assumption that


This comment needs updating.

updated in 8e20dd3

ansis · 2017-12-19T23:25:39Z

70eb215 should fix the missing labels

ChrisLoer · 2017-12-19T23:33:21Z

src/symbol/cross_tile_symbol_index.js

        }

-        const tileIndex = new TileLayerIndex(tileID, symbolInstances);
-
        // make all higher-res child tiles block duplicate labels in this tile


These two loops (child index and parent index) still have structure and comments from when child and parent indexes had different blocking logic. Now that the logic is really just "run findMatches across all zoom levels", maybe we could express this more clearly with one loop that just has a branch for looking up child vs. parent indices?

ChrisLoer · 2017-12-19T23:41:33Z

src/style/style.js

+                    .sort((a, b) => (b.tileID.overscaledZ - a.tileID.overscaledZ) || (a.tileID.isLessThan(b.tileID) ? -1 : 1));
+            }
+
+            if (this.crossTileSymbolIndex.addLayer(styleLayer, layerTiles[styleLayer.source])) symbolBucketsChanged = true;


How about symbolBucketsChanged |= addLayer(... instead?

I think a regular if is clearer than cleverly using a bitwise or

and flow expects numbers not booleans for |=

OK, how about just bringing the symbolBucketsChanged = true down to its own line? My reasoning is that when someone is scanning the logic looking for where assignments happen, they're easiest (most common) to see when they start the line they're on.

good point, would this alternative be better than a separate line?

const layerBucketsChanged = this.crossTileSymbolIndex.addLayer(styleLayer, layerTiles[styleLayer.source]); symbolBucketsChanged = symbolBucketsChanged || layerBucketsChanged;

Yeah that's good.

ChrisLoer · 2017-12-19T23:49:25Z

70eb215 should fix the missing labels

👍 Looking good now.

ChrisLoer · 2017-12-20T00:00:01Z

Would adding a bucketInstanceId check be good enough? In the case of a bucket update and stale placement it would return nothing.

It would work to prevent errors, but wouldn't that still exhibit the behavior from #5716 (comment)? Ideally, I think the results of queryRenderedFeatures should only change when a new placement is committed. For this PR, though, it's probably OK to just make sure we don't error out.

ChrisLoer · 2017-12-20T18:46:09Z

It looks like getting rid of full placement on tile loading is working to make the ten-second pitched rotation animation case significantly smoother (looks like 2 instances of stutter after vs. about 15 before):

fps_meter_during_rotation.zip

🎉 !

ChrisLoer

OK, I think the "don't error when querying tiles with stale placement" solution is good enough for now. This should fix #5716 but leave #5887 for later.

ansis · 2018-01-08T21:04:47Z

@ChrisLoer can you review c47acaa, d44241b, ba0dc8a, 98dbc7f which are meant to fix flickering bugs related to data and style updates?

ChrisLoer · 2018-01-08T21:45:37Z

src/style/style.js

+                    this.collisionIndex = this.placement.collisionIndex;
+                    this.placement.setRecent(browser.now());
+                } else {
+                    this.placement.setRecent(browser.now());


This was tricky for me to read -- it looks like setRecent is just being duplicated, it's easy to miss that setRecent is going to set stale to false, and confusing that we want to undo it right away (because we want to update _recentUntil for throttling, right?).

Could we just save something like inProgressPlacementStale at the top and set it at the bottom? It might be clearer.

Or would it make sense to simplify this just by always committing the pausable placement? Do we expect there's some performance win from keeping the original placement object unmodified in the case nothing changes?

ChrisLoer · 2018-01-09T18:21:00Z

src/symbol/placement.js

+class JointPlacement {
+    text: boolean;
+    icon: boolean;
+    // offscreen = outside viewport, but within CollisionIndex::viewportPadding px of the edge


By the time it gets here "offscreen" now means something like "skip fade animation" since it's the combination of "offscreen" and "justReloaded". We should come up with a new name to account for that once the two concepts are combined.

The index relied on a source layer and index to identify a feature. If a data layer gets updated a new bucket is formed where these identifiers probably don't match the previous bucket. Since there is a gap between a bucket being loaded and the index being updated (during the next placement) it was possible for a query to return wrong results. This prevents it from returning incorrect features but it does not return the correct features. This might cause it to return empty results.

ansis requested review from jfirebaugh and ChrisLoer December 19, 2017 01:17

ChrisLoer mentioned this pull request Dec 19, 2017

symbol layer fades in upon setData #5716

Closed

ChrisLoer mentioned this pull request Dec 19, 2017

Global collision detection (>0.42.0) regresses performance of rendering very dense symbol layers #5891

Open

ansis force-pushed the collision-port-back branch from aac1687 to 0570b39 Compare December 19, 2017 17:50

ChrisLoer reviewed Dec 19, 2017

View reviewed changes

ansis mentioned this pull request Dec 20, 2017

port minor collision changes from -js mapbox/mapbox-gl-native#10764

Merged

ChrisLoer approved these changes Jan 8, 2018

View reviewed changes

ansis force-pushed the collision-port-back branch from b0aa49b to 98dbc7f Compare January 8, 2018 21:02

ChrisLoer reviewed Jan 8, 2018

View reviewed changes

ChrisLoer reviewed Jan 9, 2018

View reviewed changes

ansis added 3 commits January 9, 2018 14:53

port CrossTileSymbolIndex changes back from -native

557e269

fix symbol fading bugs

6dd9f66

ansis force-pushed the collision-port-back branch from 6f8caa4 to 6dd9f66 Compare January 9, 2018 19:54

ansis merged commit 3963d94 into master Jan 9, 2018

ansis mentioned this pull request Jan 11, 2018

Wobbly/shaky/dancing point icons on hover with many points #5980

Closed

jfirebaugh deleted the collision-port-back branch February 22, 2018 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

port CollisionIndex and Placement changes from -native #5890

port CollisionIndex and Placement changes from -native #5890

ansis commented Dec 19, 2017 •

edited

Loading

ChrisLoer commented Dec 19, 2017

ansis commented Dec 19, 2017

ChrisLoer Dec 19, 2017

ChrisLoer commented Dec 19, 2017

ChrisLoer Dec 19, 2017

ChrisLoer Dec 19, 2017

ansis Dec 20, 2017

ansis commented Dec 19, 2017

ChrisLoer Dec 19, 2017

ChrisLoer Dec 19, 2017

ansis Dec 20, 2017

ansis Dec 20, 2017

ChrisLoer Dec 20, 2017

ansis Dec 20, 2017

ChrisLoer Dec 20, 2017

ChrisLoer commented Dec 19, 2017 •

edited

Loading

ChrisLoer commented Dec 20, 2017

ChrisLoer commented Dec 20, 2017

ChrisLoer left a comment

ansis commented Jan 8, 2018

ChrisLoer Jan 8, 2018

ChrisLoer Jan 9, 2018

port CollisionIndex and Placement changes from -native #5890

port CollisionIndex and Placement changes from -native #5890

Conversation

ansis commented Dec 19, 2017 • edited Loading

Launch Checklist

ChrisLoer commented Dec 19, 2017

ansis commented Dec 19, 2017

Choose a reason for hiding this comment

ChrisLoer commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansis commented Dec 19, 2017

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

ChrisLoer commented Dec 19, 2017 • edited Loading

ChrisLoer commented Dec 20, 2017

ChrisLoer commented Dec 20, 2017

ChrisLoer left a comment

Choose a reason for hiding this comment

ansis commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansis commented Dec 19, 2017 •

edited

Loading

ChrisLoer commented Dec 19, 2017 •

edited

Loading