-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
retain all loaded children #5671
Conversation
@mollymerp looking back at https://github.com/mapbox/mapbox-gl-native/blob/3bef7593a64a51e86dd5a2ed9fd36b4a143350b0/src/mbgl/algorithm/update_renderables.hpp, I think this is not what -native does -- will this need a port? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 , but I'd say if there's any reason we don't want to port this to native, then we might want to discuss whether we really want divergent logic.
new TileCoord(2, 0, 2), | ||
new TileCoord(2, 1, 2), | ||
new TileCoord(2, 0, 3), | ||
new TileCoord(2, 1, 3), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first, I was going to ask whether there's some other way we could preserve testing of this logic, checking for child tiles before trying to load parent... But, if I'm understanding things right, it looks like the 'prefer loaded child tiles to parent tiles'
at least partially confirms that the right thing is happening, so we're probably okay. Does that sound right to you, @mollymerp?
Do you think it would be worthwhile to add a test that's similar to
the 'prefer loaded child tiles to parent tiles'
one, but where all the child tiles are loaded, to confirm that we don't load the parent in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
71da00c
to
c095971
Compare
per chat with @kkaefer this morning – he is 👍 on restoring this behavior in JS and porting to native. apparently the native behavior was originally implemented to avoid having to render too many child tiles (if they were highly under-zoomed), but because we're only retaining already loaded child tiles, it is not likely that we'd end up with an unreasonable number of tiles to render. |
... even those at ZL more than one greater than the ideal tile.
popping this out of #5286. right now we're only checking for loaded children that are exactly one ZL higher from the ideal tile. this change makes it so we retain all loaded children of not-yet-loaded ideal tiles so that in the event of a fast zoom out we don't drop deep descendent child tiles before the parents are loaded.
Launch Checklist