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

[WIP] viewport label placement #4972

Closed
wants to merge 66 commits into from
Closed

[WIP] viewport label placement #4972

wants to merge 66 commits into from

Conversation

ansis
Copy link
Contributor

@ansis ansis commented Jul 11, 2017

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:

  • More reliable collision detection between tiles (buffering/avoid-edges are no longer necessary)
  • Collision detection now works on labels from different sources (e.g. a GeoJSON set of labels added on top of a base map)
  • Calculating collision boxes at (or near) render time is much more accurate for pitched maps, line labels, sizes based on zoom functions, etc.
  • Fade in/out animations are no longer limited to zoom operations (i.e. collisions related to pitch/pan/rotate will all be animated now)

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.
  • The "placement" step no longer happens in the background thread, and static buffers for symbols are created during the "prepare" step.
  • The new foreground "placement" step is primarily responsible for updating a dynamic array of opacities that are used to animate collision states. For each vertex, the opacity state is expressed as an "opacity at time of last update" and a "target opacity". The symbol shaders animate towards the "target opacity" based on an animation start time set for each bucket.
  • It's now necessary to place a hard time limit on how long an individual placement call can take, to avoid frame stutter -- so full placement can now be split across multiple frames.
  • Collision features for lines are now represented in the collision grid as an array of circles, instead of as an array of boxes. This makes their behavior more stable under pitch/rotation operations.
  • Because fading is now tied to "collision time" instead of to zoom level, we need a way to tell when two symbols in tiles of two different zoom levels are "the same symbol", so that we can avoid triggering a fade in/fade out when we cross between zoom levels. This is primarily handled by the new CrossTileSymbolIndex.

TODO:

  • Handle re-ordering for overlapping symbols (requires re-generating static buffers on rotation)
  • Re-implement symbol-avoid-edges support (?)
  • Fix remaining test failures/update tests to reflect new behavior
    • Render tests
    • Query tests
    • Unit tests
  • Performance improvements:
    • 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.
    • Allow progressive placement to pause at the bucket level. (Currently placement pauses at the layer level, where a single dense layer of street labels on a large viewport can take ~8ms on my MacBook Pro)
  • Implement anti-aliasing for collision circle debugging (need them to look good for Studio)
  • Update comments, rationalize class names (e.g. CollisionTile), remove dead code
  • Simplify representation of collision boxes/circles used on foreground
  • Rebase on master
  • Flow-ify changes
  • Handle wrapped tiles in CrossTileSymbolIndex
  • Figure out home for 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.
  • Port to gl-native
    • Figure out how to continue doing tile-based collision detection in api-gl

This PR will enable further changes that we're not tackling yet:

@ansis
Copy link
Contributor Author

ansis commented Jul 11, 2017

So far I've switched CollisionTile to use viewport units and I've made it possible to switch the tile-specific parts of CollisionTile (matrix, collisionBoxArray). Right now I'm just trying to get to a hacky version of global viewport label placement so that we can unblock other work.

I think steps are:

  • Switch to global label placement by using one CollisionTile for all tiles. Create the CollisionTile in Style._redoPlacement and pass it to the source.redoPlacement and tile.redoPlacement functions. @mollymerp want to take this one? I think at a high level it should look like this:
for each symbol layer
    for each source
        for each tile
            if the tile has a bucket for that layer
                collisionTile.setMatrix(matrix); // switch the transform to the current tile
                collisionBox.setCollisionBoxArray(collisionBoxArray);
                bucket.place(...)

I'll work on:

  • Only creating the collision tile on the main thread (never doing it in the worker).
  • Removing unnecessary code from CollisionTile

ansis and others added 14 commits July 11, 2017 11:52
and do placement every frame
…ecessary as long as we keep this all synchronous)
 - Doesn't account for edges
 - Doesn't account for labels that rotate with the map
 - Shaders are still trying to account for perspective (needs to be removed)
 - Use of the collision grid got much simpler: we just put in projected coordinates and test for collisions... no more looking up the bounding boxes and calculating collision scales.
 - Collision grid dimensions/column count pretty much chosen at random
 - Get rid of no-longer-necessary placementThrottler
 - Get rid of (some) of "avoidEdges" logic (it doesn't play well with global collision detection)
 - Get rid of unused serialization code
Performance improvement: don't recalculate matrices for each layer
…sed for a label even if we already know that the label is blocked.
…ce mathematical operations in circle/rect comparisons.
Reload tiles when debug collision boxes are turned on so that buffers get generated.
…ol_index, with opacities stored in the symbolInstances).

Base fadeChange shader uniform on the last time the opacities for a bucket were updated.
…target opacity" for a symbol.

Previous approximation was to just render for 300ms whenever there was a move event, but that could leave some fade animations incomplete until the next move (for instance, on a new tile load).
…ys become strings under the hood.

(This would cause traversal of tile index in incorrect order, breaking label duplication assumptions)
 - clone opacityState when we copy it -- otherwise, different tiles end up sharing the same opacity state object with undefined results
 - Don't copy opacityState from parent to child if the parent is the one being added to the index
 - TileCoord.isChildOf() needs sourceMaxZoom
 - Remove 'assert(symbolInstance.isDuplicate !== false)' on blockLabels. This assertion was saying that a child should only have to block one parent (because the others should already be blocked). But this wasn't true. Consider a label at zoom 10 blocking a label at zoom 9. If the same label is then added at zoom 8, the zoom 10 label will "block" a second parent (the one at zoom 8).
 - Remove 'assert(parentSymbolInstance.isDuplicate !== false)' on unblockLabels. The assumption here is that any duplicates we find on unblocking should have already been marked as duplicate. While it is the hoped-for behavior, it's not currently guaranteed, because it's possible for multiple symbols in one tile to show up as duplicate for the same symbol in a different tile. Added comment explaining the case.
…symbol duplication is causing targetOpacity to be set to 0).
…00ms and 0ms providing instant collisions.

Use 0ms/instant collisions for test suite.
Also, fix bug in logic that tries to avoid line placement if a label is fully faded -- we need to make sure the label is fully faded (opacity == 0) AND we want the label to stay faded (targetOpacity == 0).
See distance_ratio comment in symbol_sdf: collision boxes are laid out in viewport space.
Previous code looked like it inverted both terms, but: 1/(.5 + .5 * x/y) != .5 + .5 * y/x
Update Chinese tests to use new collision algorithm (not behavior preserving)
@ChrisLoer
Copy link
Contributor

From discussion with @nickidlugash, our current plan is to continue supporting symbol-avoid-edges even though it won't be necessary outside of api-gl. The reasoning is roughly: maintaining consistency between api-gl and gl-native/gl-js is good whenever possible, even as we add features that break consistency (e.g. 3d buildings, viewport collision detection,...)

@ChrisLoer
Copy link
Contributor

Here are the current known caveats for cross-tile symbol fading:

  • Symbols with the same name very close to each other at high res won’t always get opacity updates transferred correctly on going to low res (or coming back to high res)
  • Symbols that are right at tile boundaries may end up split into two different parent/child hierarchies, in which case updates don’t work correctly
  • The “same anchor point” logic depends on rounding anchor locations to a grid that’s normalized between different zoom levels. Anchor points that are right at that grid line can end up rounding to different locations at different zoom levels.
  • If you’re zooming in or out quickly, the above issues are more likely to show up because we end up crossing multiple zoom levels at once and the rounding effects are stronger.

When the logic fails, you should just see a label fade out and then fade back in. I think it’s starting to look pretty acceptable… but we can definitely still improve. All of the above caveats can be fixed or at least mitigated (but at the cost of adding code complexity and some computation cost), although ultimately it’s not going to be 100% because “same label text and same anchor point” is just a heuristic.

I'm not at all certain that the four points above account for all of the remaining flicker. My debugging approach so far has been to try to keep an internal data structure logging every cross-tile opacity update operation (keyed by label text), and then when I see flicker for a given label, looking it up in the data structure to see how it arrived at the flickering state.

@ChrisLoer ChrisLoer mentioned this pull request Aug 15, 2017
18 tasks
@ChrisLoer ChrisLoer closed this Oct 5, 2017
@ChrisLoer ChrisLoer deleted the start-collision branch October 5, 2017 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants