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 bug causing overzoomed tiles to disappear #4567

Merged
merged 3 commits into from Apr 11, 2017

Conversation

mollymerp
Copy link
Contributor

@mollymerp mollymerp commented Apr 11, 2017

Fixes #4564

For overzoomed tiles, TileCoord#cover returns TileCoords with incorrect z, so they dont appear to be within TileJSON.bounds for vector sources with that property, and thus (as of #4556 ) the SourceCache doesn't consider them part of visibleCoords (they get filtered out here) and so the source disappears if it's over its maxzoom

For instance in the case of the test case provided in #4564, the source has a maxzoom of 11, and the tiles within its bounds are [11/583/782, 11/584/782, 11/583/783, 11/584/784] but when you zoom to ZL 12, transform.coveringTiles returns [12/583/782, 12/584/782, 12/583/783, 12/584/784] which are not the correct tiles, and I guess doesn't cause problems besides this? Or maybe it's like this for a reason?

This PR doesn't fix that bc I'm scared to change TileCoord without understanding what's going on there. If the behavior of TileCoord is indeed a bug I can modify this PR to fix that. This PR is just a workaround to prevent over-zoomed vector sources with bounds from disappearing.

@anandthakker @jfirebaugh

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page

@mollymerp mollymerp changed the title fix overzoomed tiles when source has fix bug causing overzoomed tiles to disappear Apr 11, 2017
@mourner
Copy link
Member

mourner commented Apr 11, 2017

That's a known gotcha we've been meaning to fix — I attempted at some point but got stuck because it affects a lot of code throughout Mapbox GL. There should be an open ticket about this, trying to find it...

As for the fix, I think we should pass source.maxzoom to hasTile method and then just do Math.min(maxzoom, coord.zoom) inside — it's handled like this in other places currently. It's confusing, but will fix the issue while retaining the bounds filtering logic on overscaled zooms.

@mourner
Copy link
Member

mourner commented Apr 11, 2017

@mollymerp found the relevant tickets: #1912 and #2868

@mollymerp
Copy link
Contributor Author

Thanks for the background @mourner - i've update the PR and will add a regression test next

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Looks good! Can we use a smaller test though, e.g. 64x64? It should probably be enough to test the presence of an overzoomed tile.

@mollymerp mollymerp merged commit 30d09af into master Apr 11, 2017
@mollymerp mollymerp deleted the render-overzoomed-tiles branch April 11, 2017 22:36
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.

Layer Not Shown Past Tileset Max Zoom
2 participants