-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Viewport collision detection #5150
Conversation
99fdb47
to
dbb54da
Compare
@ChrisLoer A few notes based on my visual review of this branch today:
|
These are all good questions, and I don't know the answer. We should experiment with different values to find the best stability/performance tradeoff. I can try to make the padding a configurable value just so we can experiment more quickly.
Actually, I think that's just a bug -- there are two failing test cases for that too. It's on my list to look into, I've just been mentally filing it under "fix tests".
👍 |
OK, the query tests are now passing and the ten remaining render test failures are all caused by the known avoid-edges/tile-clipping/overlapping-symbol-sorting issues. If we removed support for symbol-avoid-edges and stopped doing tile clipping for overlapping symbols, we'd be down to just two failures, both in
I was too optimistic here. The cause of those test failures was a bug in handling collision boxes for single-glyph labels. Actually handling text-offset correctly in collisions for line labels isn't included in this PR, but it should be much more tractable after this PR goes in. I've added it to "further work". |
Benchmark results overall look pretty good, but
|
I have a local stash that replaces the "flattened" arrays of collision boxes and collision circles with arrays of javascript objects. It's much easier to read. However, it also seems to be slower. My test methodology was to rotate a pitched full screen map with lots of road labels while recording with the CPU profiler, and then look at the percent of render time spent on the relevant code (looking at ratios is more reproducible than looking at absolute render time): Before:
After:
I don't have a good explanation for why this makes such a noticeable difference, but it looks like in this particular example it pushes up placement time by at least a millisecond. |
I did four more test runs, this time with a build that represented circles/boxes as javascript objects, but with only number data members (i.e. instead of containing a |
@ChrisLoer I also noticed that working with flat number arrays generally gives a huge boost to performance compared to arrays of objects. V8 treats them differently. Earcut and Delaunator both take advantage of this. |
D'oh. I found that at least part of the cause of the flattened array being faster was that it had a bug that caused it not to place some collision circles that it should have (of course the bug derived from the awkwardness of the code!). After fixing the bug, the difference is looking narrower, but the "flattened" approach is still reliably faster... Before:
After:
|
@nickidlugash I pushed a change that extends collision detection past the edge of the viewport by 100 pixels in all directions. Unfortunately, there's no getting around that doing collision over a broader area makes it significantly more expensive (the cost scales at least linearly with the number of symbols included in the detection area). I ended up at padding of 100px because the hit to collision times wasn't that bad, and it seemed to eliminate most of the notable cases of label instability at the edge of the viewport. Maybe you could evaluate if it's good enough? If we can get away with an even lower value, that'd be great, too (just change the |
@nickidlugash I just pushed a change that draws the collision circles with anti-aliasing, so they should be "ready" for use in Studio. I could use your help figuring out the color strategy though. The current black and blue (with fading to indicate unused collision boxes/circles) works well for me, and I think it looks reasonable. We could go back to green and red, but those are really difficult for my kind to figure out. Overall, I think the decision is a lot easier because we used to have four colors for the various states, but now there are just two colors. |
a698939
to
4fd25dc
Compare
Thanks @anandthakker for making it easy to do a density plot (although I don't have the R skills you have, were you using ggplot?). Overall, frame durations are looking pretty similar, but there's definitely a bit of a long tail that's been introduced with these changes. (steeper/taller line is |
I pushed a change to allow placement to pause per-bucket instead of just per-layer (although the pause/restart logic is a bit ugly 😬 ). On my machine even the most expensive/dense buckets don't take much more than a millisecond to place, so we can pretty effectively constrain how long placement takes. I've updated the benchmark at the top of the ticket, along with a density plot showing that the behavior is now looking pretty close to The remaining flat-lined long tail represent the frames on which a new tile loads, in which case full placement has to take place (in this run, the longest frame-duration came out to 16.035ms, so that probably would have been a one frame stutter). We can't do a partial placement when new tiles come in because they may be higher or lower-zoom versions of tiles we're already showing and to avoid flicker we need to transfer opacities from the previous zoom level. What we could potentially do to solve the tile loading problem is:
|
@kkaefer (or @mourner), do you think we can get away with doing a full placement in those two (relatively uncommon) cases? |
@ansis When you get back, I'd like to brainstorm with you about how to solve the overlapping-symbol-ordering problem, which I think is the only big thing left on the JS side. Ideally, I'd like to fix #2706 and not reopen #470.
My inclination is to do "per-tile re-sort on foreground, without tile-clipping". I think the problems in #2706 are overall more severe than the potential for overlapping symbols to have their order messed up at tile boundaries -- but I may not have enough context for why that would be a serious regression. I think "combine and sort buffers on the foreground" is promising, but it would be a big change and this PR is already a lot to digest. |
@ChrisLoer I reviewed this in one of the areas of the Mapbox Streets style with the most collisions (Manhattan at z16), with a 60deg pitch, with the default collision fade duration of 300ms, and I think it looks good enough 👍 I only tested it with manually panning/zooming/rotating/pitching the map, but for all practical speeds of doing these actions, the map handled this issue pretty well. I haven't tested camera animations, but as we discussed offline, fly to animations where the zoom level + center is changing rapidly probably won't load new tiles fast enough to be very affected by this (but I guess we should test this?). The nature of animation used for turn-by-turn guidance seems like it is slow enough (and typically uses styles that have sparse enough labels) for this to not be an issue, despite the extreme pitch typically used. There may be use cases we haven't discussed that need very fast panning on pitched maps, but I think this viewport padding solution is fine to proceed with for now. If performance is a big concern, we could decrease the padding a bit – a 50px padding was noticeably worse than 100px, but I thought 75px was still pretty reasonable. |
@nickidlugash Thanks for taking the time to run those tests! Sounds like your impressions were broadly similar to mine. I think I'll stick with 100px vs 75px for now just to give the padding a little padding. ;) |
My latest change tries to be more methodical about choosing an optimal number of cells for the grid-index on the size of the viewport. The results at the end of all the tweaking don't come out all that different on my device/viewport from what I was seeing with the earlier choice of "make a 20x20 grid", but they should be a little more robust to different viewport sizes. I evaluated choices by collecting the average time to complete a full placement (i.e. one placement split across multiple frames) during a 10 second 180 degree rotation in a 60 degree pitched map full of line labels using Nicki's label-placement-debug style ( Basically anything between 15px and 40px seemed to work pretty similarly, so I went with a 25px cell size. |
6b0cd2e
to
499c215
Compare
After rebasing on master and combining with the expressions PR (#4777), the benchmark results look problematic. 😞 In both density plots, the flatter line is Both branches seem to have a longer tail than we'd like -- I'm not sure what we expect is causing that on the expressions side (I'm assuming the changes on master are because of #4777, that could be wrong). On the Before expressions
After expressions
/cc @anandthakker |
I experimented with trying to cut the time spent on updating the dynamic opacity buffer by reducing 8 uint16s per glyph (opacity + target opacity for each of the four corners) to one uint32 per glyph. I packed the opacity information into one uint8 per vertex, and then I packed all four vertices into one uint32 (and just cast to uint8s when I bound the buffer), so that there'd be fewer assignments/multiplications on the javascript side. It worked just fine, but it didn't make a noticeable dent in the time spent updating the buffers (watching in the profiler). Also, if making the buffers smaller improved rendering times, it was too small of a difference for me to be able to reliably pick up in the benchmarks. |
Fixes issue #6548. Restores behavior from before PR #5150 -- all layers that share the same bucket (and thus same layout properties) share the same placement. Before this fix, two layers with the same layout properties could collide against each other, and because they shared CrossTileIDs, _both_ layers could end up hidden.
Fixes issue #6548. Restores behavior from before PR #5150 -- all layers that share the same bucket (and thus same layout properties) share the same placement. Before this fix, two layers with the same layout properties could collide against each other, and because they shared CrossTileIDs, _both_ layers could end up hidden.
Fixes issue #6548. Restores behavior from before PR #5150 -- all layers that share the same bucket (and thus same layout properties) share the same placement. Before this fix, two layers with the same layout properties could collide against each other, and because they shared CrossTileIDs, _both_ layers could end up hidden.
Fixes issue #6548. Restores behavior from before PR #5150 -- all layers that share the same bucket (and thus same layout properties) share the same placement. Before this fix, two layers with the same layout properties could collide against each other, and because they shared CrossTileIDs, _both_ layers could end up hidden.
Re-implement basic collision group support based on a global "crossSourceCollisions" map option that replicates pre-#5150 behavior. Render tests maintain the same structure/results, but are now based on grouping-by-source.
Re-implement basic collision group support based on a global "crossSourceCollisions" map option that replicates pre-#5150 behavior. Render tests maintain the same structure/results, but are now based on grouping-by-source.
Re-implement basic collision group support based on a global "crossSourceCollisions" map option that replicates pre-#5150 behavior. Render tests maintain the same structure/results, but are now based on grouping-by-source.
Re-implement basic collision group support based on a global "crossSourceCollisions" map option that replicates pre-#5150 behavior. Render tests maintain the same structure/results, but are now based on grouping-by-source.
Re-implement basic collision group support based on a global "crossSourceCollisions" map option that replicates pre-mapbox#5150 behavior. Render tests maintain the same structure/results, but are now based on grouping-by-source.
Switching from tiled label placement to global viewport label placement. #4704 WIP.
This PR dramatically changes our approach to collision detection. Instead of trying to pre-calculate collisions across a range of zooms in the background, we now calculate collisions synchronously in the foreground. Fade animations are no longer tied to zoom level, but instead based on time elapsed since collision occurred. This approach helps solve many problems:
The main structural changes are:
CollisionTile
is now a global object that lives only in the foreground. Insertions/queries into the collision index are now done entirely in viewport coordinates instead of tile coordinates.CrossTileSymbolIndex
.TODO:
Don't require full placement when a new tile is added -- instead, start progressive placement for the tile as soon as it's added, but don't start rendering until the first full placement has completed.CollisionTile
), remove dead codeCrossTileSymbolIndex
grid_index_experimental
(adds support for circle geometry queries; removes support for cross-thread serialization): either update https://github.com/mapbox/grid-index, make a new library, or just continue to include the code directly.gl-native
api-gl
This PR will enable further changes that we're not tackling yet:
text-offset
applied to collision detectionLatest Benchmark
/cc @ansis @nickidlugash @mollymerp