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

Relayout after recalculating to ensure visibility changes are handled correctly #7242

Merged
merged 3 commits into from
Dec 13, 2016

Conversation

ivovandongen
Copy link
Contributor

@ivovandongen ivovandongen commented Nov 30, 2016

Fixes #7241
Fixes #7225
Fixes #6758

Re-ordering relayout() after recalculate() ensures that sources are enabled before checking if tiles need to be reloaded on them.

@ivovandongen ivovandongen added the Core The cross-platform C++ core, aka mbgl label Nov 30, 2016
@ivovandongen ivovandongen self-assigned this Nov 30, 2016
@mention-bot
Copy link

@ivovandongen, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @1ec5 and @incanus to be potential reviewers.

@ivovandongen ivovandongen added ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold and removed ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Nov 30, 2016
@ivovandongen ivovandongen mentioned this pull request Nov 30, 2016
@ivovandongen
Copy link
Contributor Author

@kkaefer @brunoabinader this pr fixes the visibility issue without revering the optimisation in 9127299

A review would be welcome so we can cherry pick it into the android 4.2.0 release

Copy link
Member

@brunoabinader brunoabinader left a comment

Choose a reason for hiding this comment

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

@ivovandongen test case looks sounds. This actually fixed the same issue I had with the runtime style example in our Qt demos. Awesome!

@1ec5
Copy link
Contributor

1ec5 commented Nov 30, 2016

This also needs to be cherry-picked to the iOS release branch.

/cc @boundsj

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Can you make the test case in mapbox-gl-test-suite instead, so both gl-js and gl-native benefit?

@ivovandongen ivovandongen force-pushed the 7241-visibility-changes branch from 9a91366 to 00cbcce Compare December 1, 2016 07:55
@ivovandongen ivovandongen changed the title Relayout after recalculating to ensure visibility changes are handled correcty Relayout after recalculating to ensure visibility changes are handled correctly Dec 2, 2016
@ivovandongen ivovandongen force-pushed the 7241-visibility-changes branch from 00cbcce to 424c05b Compare December 7, 2016 14:43
@ivovandongen
Copy link
Contributor Author

@jfirebaugh I fixed the unit test. Not sure how to implement the same thing in a regression test: mapbox/mapbox-gl-test-suite#188

@ivovandongen ivovandongen force-pushed the 7241-visibility-changes branch from 424c05b to f631efd Compare December 7, 2016 14:57
@jfirebaugh jfirebaugh force-pushed the 7241-visibility-changes branch from f631efd to 320e1db Compare December 12, 2016 21:24
jfirebaugh and others added 3 commits December 12, 2016 13:59
This ensures that a "wait" command will always fully flush pending update flags. This was not the case with the prior conditional map.loaded() logic.
Previously, for viewport sizes less than 512 pixels in either direction, the computed size was 0.
Style::relayout uses source.baseImpl->loaded, a flag which is updated by Style::recalculate. So recalculate first, then relayout.
@jfirebaugh jfirebaugh force-pushed the 7241-visibility-changes branch from 320e1db to 904f1a3 Compare December 12, 2016 21:59
@jfirebaugh jfirebaugh merged commit 28be37f into master Dec 13, 2016
@jfirebaugh jfirebaugh deleted the 7241-visibility-changes branch December 13, 2016 00:49
boundsj added a commit that referenced this pull request Dec 14, 2016
This ports #7242
commits/2d323211af54499d5c822b8e45d7415bf92112f0 to the iOS 3.4.0
release branch.
boundsj added a commit that referenced this pull request Dec 14, 2016
This ports #7242
commits/2d323211af54499d5c822b8e45d7415bf92112f0 to the iOS 3.4.0
release branch.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants