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

Don't break ascent in the overscaled tile phase #12931

Merged
merged 1 commit into from
Sep 21, 2018

Conversation

kkaefer
Copy link
Member

@kkaefer kkaefer commented Sep 20, 2018

We optimize our updateRenderable algorithm by breaking ascent when we've already checked a certain tile. So far, we've compared the UnwrappedTileIDs, but they don't include the overscale component. When ascending through overscaled tile IDs, we've stopped the ascent too early, when we should've kept the search going. This resulted in tiles not getting rendered even though they were available.

We optimize our updateRenderable algorithm by breaking ascent when we've already checked a certain tile. So far, we've compared the UnwrappedTileIDs, but they don't include the overscale component. When ascending through overscaled tile IDs, we've stopped the ascent too early, when we should've kept the search going.
@kkaefer kkaefer added bug Core The cross-platform C++ core, aka mbgl labels Sep 20, 2018
@kkaefer kkaefer requested a review from ChrisLoer September 20, 2018 11:20
Copy link
Contributor

@ChrisLoer ChrisLoer left a comment

Choose a reason for hiding this comment

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

As always with update_renderables logic it took me a while to feel like I understood what's going on, but this looks good to me. Also looks like GL JS was already using overscaled tile IDs in its equivalent logic:

https://github.com/mapbox/mapbox-gl-js/blob/a1233cb33e3b55cde90b52b908cb4d8f4b6864e6/src/source/source_cache.js#L373-L386

@kkaefer kkaefer merged commit 2ea0833 into master Sep 21, 2018
@kkaefer kkaefer deleted the updaterenderables-tile-ascent branch September 21, 2018 10:12
@kkaefer
Copy link
Member Author

kkaefer commented Sep 21, 2018

Thanks for checking that JS works correctly!

kkaefer added a commit that referenced this pull request Oct 2, 2018
kkaefer added a commit that referenced this pull request Oct 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants