Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Filter out child tiles that overlap with parent tile #13963

Closed
wants to merge 2 commits into from

Conversation

alexshalamov
Copy link
Contributor

@alexshalamov alexshalamov commented Feb 21, 2019

Whenever map is panned or zoomed and some of the tiles at the current zoom level are not loaded, cached parent tile may be used until missing tile is loaded. The problem is that whenever circle layer is non-opaque or has blur, parent tile will be drawn together with children (overdraw) causing flickering, as the non-opaque pixels are blended together.

This PR adds filter for circle layer in order to avoid rendering parent and children tiles covering same area.

Checklist

  • briefly describe the changes in this PR
  • add unit tests

@alexshalamov alexshalamov changed the title [core] Filter out child tiles that overlap with parent render tile [core] Filter out child tiles that overlap with parent tile Feb 21, 2019
@asheemmamoowala
Copy link
Contributor

asheemmamoowala commented Feb 21, 2019

Related GL-JS issue: mapbox/mapbox-gl-js#6768

@alexshalamov alexshalamov force-pushed the alexshalamov_filter_circle_layer_tiles branch 2 times, most recently from db88510 to 057cba2 Compare February 25, 2019 08:18
@alexshalamov alexshalamov force-pushed the alexshalamov_filter_circle_layer_tiles branch from 057cba2 to 6a2764b Compare February 25, 2019 11:39
Copy link
Contributor

@pozdnyakov pozdnyakov left a comment

Choose a reason for hiding this comment

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

LGTM % nit

@@ -187,4 +200,33 @@ bool RenderCircleLayer::queryIntersectsFeature(
return false;
}

RenderLayer::RenderTiles RenderCircleLayer::filterRenderTiles(RenderTiles tiles_) const {
RenderTiles tiles = RenderLayer::filterRenderTiles(tiles_);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: RenderLayer::filterRenderTiles(std::move(tiles_));

@ansis
Copy link
Contributor

ansis commented Feb 26, 2019

Could this approach introduce flickering in existing tiles? I'm trying to understand what would happen in this case.

  • you load and display the z0/x0/y0 tile centered on the northwest corner of the world
  • you zoom in and the z1/x0/y0 tile is loaded
  • now you pan east and eventually you get to an area that needs both z1/x0/y0 (already loaded) and z1/x1/y0 (needs to be loaded)
  • as you are loading z1/x1/y0 the cached z0/x0/y0 is rendered

Does the z1/x0/y0 disappear for a bit while z1/x1/y0 loads? Does this introduce flickering if the z1 tile had data that the z0 tile didn't? (due to simplification and data thinning)


If it does introduce flickering it could still be much better than the current flickering!

I'm not sure how else this could be fixed. Maybe a uniform could be passed to the vertex shader that specifies which parts of the tile are covered by child tiles? It could then use that to drop features from the parent tile that are in areas covered by child tiles.

@alexshalamov
Copy link
Contributor Author

@ansis

Does the z1/x0/y0 disappear for a bit while z1/x1/y0 loads? Does this introduce flickering if the z1 tile had data that the z0 tile didn't? (due to simplification and data thinning)

Yes, it does. Also, I think there is a problem related to cached tiles. One missing child tile should not make rest of loaded / cached tiles non-renderable.

I need to rewrite PR and approach the issue differently, options:

  • Drop vertices in vertex shader (proposed by @ansis)
  • Drop features. Something like 'cross-tile-geometry-index' that can be used by circle and heatmap layers

@alexshalamov
Copy link
Contributor Author

Need to rewrite PR.

@alexshalamov alexshalamov deleted the alexshalamov_filter_circle_layer_tiles branch March 20, 2019 20:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants