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

Fix vector tile overdrawing with overlapping tiles from different zoom levels #6768

Open
jfirebaugh opened this issue May 31, 2018 · 7 comments
Assignees
Labels

Comments

@jfirebaugh
Copy link
Contributor

The tile loading logic in Mapbox GL JS attempts to load a parent tile in certain cases when a request for a child tile fails, e.g. the tile response is a 404, or a 200 with a content length of 0. This is intended to compensate for situations where tiles are not available at the current zoom level, but may be available at lower zoom levels, which could be overzoomed.

Unfortunately, this leads to overdraw in certain situations. I believe the primary situation (there may be others) involves:

  • A vector tileset that does not cover the whole world, i.e. tiles exist only for a certain bounding box. Outside that bounding box, the Mapbox tile API will respond with 404s.
  • Layers sourced from this tileset that use semi-transparent styling.
  • A map viewport that intersects the bounds of the tileset.

Under these conditions, the map succeeds in loading one or more tiles at zoom z inside the bounds, but fails on one or more tiles outside the bounds. This triggers requests for tiles at z-1 that succeed, returning tiles that overlap the child tiles. Both parent and child tiles are rendered, producing overdraw. (The fallback loading behavior was originally implemented for fully opaque raster tiles, and semi-transparent vector tiles was not taken into account.)

Multiple issues have been filed relating to this behavior:

IIRC, we originally limited the parent tile fallback behavior to raster sources. This issue probably dates to when we expanded that behavior to vector sources as well. I can think of a few ways we might fix it:

  • Revert loading parent tiles for vector sources. We'd have to go back and look at the rationale for that change and see if there's a different way that doesn't trigger this bug.
  • Don't render parent tiles for vector sources when there's any successful child tile. Probably don't want this because parent tile rendering is important to avoid showing empty areas during pans and zooms.
  • Adjust the parent tile loading behavior to be smarter around tileset bounds. Something like: don't load a parent tile when a child tile is near the bounds or when the parent tile intersects the bounds.
  • Clip child tiles out of parent tiles during rendering. We did this for raster tiles in Clip raster tiles  #5105.

@kkaefer, @mollymerp you're most familiar with tile loading / clipping behavior. What are your thoughts?

Making this high priority since it's affecting multiple customers. cc @lilykaiser

@springmeyer
Copy link
Contributor

Another option would be, for vector sources via the Mapbox API, ignore the maxzoom defined in the tilejson and simply request tiles at a higher zoom. So, let the server overzoom: In this case the Mapbox API will do the 404 check internally and will clip out a new tile for you.

@mollymerp
Copy link
Contributor

mollymerp commented Jun 6, 2018

Thanks for summarizing the issue in this ticket @jfirebaugh!

The original reason for the change in parent tile loading was to support "sparse" raster tilesets in JS, so in the case where a parent tile had data for a region with no corresponding child tile in one or more quadrant of the parent, we display the data in that area by retaining the parent and using a modified vertex buffer as a "stencil buffer". It also was meant to bring JS in line with the Native update_renderables tile retention logic which changed to "required" loading of parent tiles (from "optional" loading/retention) to deal with a bug in the node bindings (I think) mapbox/mapbox-gl-native#8769

Revert loading parent tiles for vector sources. We'd have to go back and look at the rationale for that change and see if there's a different way that doesn't trigger this bug.

I don't think this will actually fix the issue for sparse tilesets because we will still need to retain the loaded parent if a child is not yet available to avoid a flickering when zooming, and even if we don't actively load new parent tiles, previously cached tiles will still overdraw.

Here is an example from the release prior to the tile loading changes (you need to zoom in and out a bit to reproduce)

http://jsbin.com/donugareji/edit?html,output

Don't render parent tiles for vector sources when there's any successful child tile. Probably don't want this because parent tile rendering is important to avoid showing empty areas during pans and zooms.

@ryanhamley and I tested this out and, yep, this solves the overdraw issue but causes flickering on zoom 👎

Adjust the parent tile loading behavior to be smarter around tileset bounds. Something like: don't load a parent tile when a child tile is near the bounds or when the parent tile intersects the bounds.

I don't think this will solve the issue for all users, because not all tilesets have TileJSON bounds, and for some reason, the tileset in the example above TileJSON bounds are the mercator projection bounds while the data is only in a very small area– this could be an issue with API maps/unpacker?

Clip child tiles out of parent tiles during rendering. We did this for raster tiles in #5105.

I'm thinking this might be the only fool-proof option, but we won't be able to use the same technique as we did with raster tiles (because the raster tile vertex buffer only represents the bounds of the tile, we generate custom vertex buffers on the fly to mask out areas of overdraw). To use the same technique with vector layers, I think we'll have to render clipping masks to the stencil buffer for each tile's mask VAO (instead of from the general tile extent VAO). No sure what performance implications this will have, but it's worth a try I think given the above.

@springmeyer thanks for the suggesstion!

Another option would be, for vector sources via the Mapbox API, ignore the maxzoom defined in the tilejson and simply request tiles at a higher zoom. So, let the server overzoom: In this case the Mapbox API will do the 404 check internally and will clip out a new tile for you.

I don't think the current issue is a problem with overzooming – its mainly an issue with sparse tilesets where some children of a parent tile have data and others do not, so the parent is retained and overdraws on the child tile.

@springmeyer
Copy link
Contributor

I don't think the current issue is a problem with overzooming – its mainly an issue with sparse tilesets where some children of a parent tile have data and others do not, so the parent is retained and overdraws on the child tile.

@mollymerp I'm proposing not retaining/overdrawing but rather requesting tiles from the server and letting it overzoom. Or am I missing something?

@kkaefer
Copy link
Contributor

kkaefer commented Jun 8, 2018

I don't think we should rely on server-side overzooming, or ever request tiles that go beyond the TileJSON's maxzoom. Overzooming tiles is one of the hallmarks of GL and allows us to keep data usage low, and render high-res maps from offline tiles as well. Even if we did request server-side overzoomed tiles, we'd still have to add this logic to GL to support the use cases where those tiles haven't loaded yet, or will never load due to connectivity loss.

@mollymerp
Copy link
Contributor

mollymerp commented Jun 15, 2018

update here from some more investigations @ryanhamley and I did:

  • we tried the clipping via stencil buffer option, but unfortunately it won't work for unclipped layers (circle, heatmap, etc) because those features need to be allowed to cross tile boundaries.
  • turns out gl-native treats 404 as valid vector tiles with a "nocontent" flag, so the issue is not as prevalent because it only overdraws parent tiles for a frame or two until the children error out – we thought we could use this approach (6768 overdraw fix #6803) but it prevents valid overzooming when you go past a tileset's maxzoom. (note that overzooming when tiles 404 at zoom level Z, but Z < source maxzoom does not work the same way in native because of this)
  • @jfirebaugh suggested a check where we don't retain parent tiles if at least one child has failed && at least one child has errored (indicating a sparse tileset). this is the best solution we've tried so far, but it does not completely eliminate overdraw. In the case of a "grandparent" tile that has at least one empty quadrant, if we zoom in to the grandchild ZL, at least one parent tile will have all siblings error (because they're empty) and so we'll go up and find the grandparent, which does not 404, so we retain it causing overdraw (my attempt at an illustration below).

img_5828

I have two more ideas:

  • revert to only retaining parents 1 ZL up from the ideal tile for all vector tilesets
  • add a 'sparse' option for vector tile sources, and when that option is true
    • don't retain parent if any of the current tile's siblings have successfully loaded (causes a flicker in between zoom levels)
    • only retain parents 1 ZL up, and don't retain parent tiles if at least one child has failed && at least one child has errored

I'm open to other ideas if anyone has thoughts!

@peterqliu
Copy link
Contributor

🚗 drive-by thought: in cases where only some child tiles fail, can we iterate through the features inside each successful child tile, identify the same feature in the parent tile, and cull those out of the parent, to avoid overdraw?

Only thing is, I can't say how reliable the deduping step is, without some unique feature id.

@mollymerp
Copy link
Contributor

we fixed this 90% for circle layers in #6803 – persistent overdraw is no longer present, but there is flashed overdraw when changing zoom levels (loading tiles from the network). A more permanent fix will require some kind of intelligent vertex culling and would be a more significant lift.

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

No branches or pull requests

7 participants