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

Transitions fail to complete when zooming #1548

Closed
wants to merge 0 commits into from

Conversation

brunoabinader
Copy link
Member

When zooming with a scroll wheel or with pinch gestures, we sometimes fail to finish the fade transition (both fading in and fading out):

image

@jfirebaugh
Copy link
Contributor

@brunoabinader, want to take a crack at this? Basically, we need to thread some more state through so that MapContext::renderSync can return true for needsRerender while label fading is happening.

@brunoabinader
Copy link
Member

@jfirebaugh yes, I'll take a look at this.

brunoabinader added a commit that referenced this pull request Jul 7, 2015
Not only it fixes #1548, but also makes the function `O(1)` instead of
`O(n)` (caused by usage of `size()`).
@brunoabinader
Copy link
Member

Review @kkaefer @jfirebaugh?

@kkaefer
Copy link
Contributor Author

kkaefer commented Jul 8, 2015

While this fixes the bug, it also means that the map is continuously rendering at 60fps, using system resources and battery. Instead, we need to figure out in what circumstances we need to rerender (= when fading in/out), and not render otherwise.

@mourner mourner removed the ready label Jul 9, 2015
@brunoabinader brunoabinader reopened this Jul 9, 2015
@brunoabinader brunoabinader force-pushed the 1548-transition-fail-zoom branch 2 times, most recently from 3bab65e to 8163956 Compare July 13, 2015 13:00
@brunoabinader brunoabinader changed the title Transitions fail to complete when zooming [WIP] Transitions fail to complete when zooming Jul 13, 2015
@brunoabinader brunoabinader added the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jul 13, 2015
@brunoabinader brunoabinader changed the title [WIP] Transitions fail to complete when zooming Transitions fail to complete when zooming Jul 14, 2015
@brunoabinader brunoabinader added ready and removed in progress ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Jul 14, 2015
@brunoabinader
Copy link
Member

I've cracked my head a bit these past days trying to figure out why this was not working. It turns out that we're missing checking if Painter::needsAnimation() along with Style::hasTransitions() in MapContext::renderSync(). I've learned a lot from the code while resolving this bug, and thus got the chance to do some other fixes and cleanups along the way:

Relevant changes:

  • Unify default transition values in MapData:
    • Added defaultFadeDuration and defaultTransitionDelay to MapData;
    • Painter & StyleParser now receives a reference to MapData;
    • As previously seen on the code:
      • 300ms is the default fade duration;
      • 0ms is the default transition duration;
    • We no longer pass the current time point to Style, since it now uses MapData.animationTime, which gets updated in MapContext::update().
    • Updated StyleParser check to use a mock MapData;
  • Pass {Duration,TimePoint} by const ref whenever possible;
  • Unify time point for Mapcontext::update:
    • Let style classes cascade use the same time point as the one used to
      recalculate style.
    • Cleaned up MapContext::update() to return early whenever possible.
    • Cleaned up MapContext::loadStyleJSON() to avoid redundant calls to style
      classes cascade.

/cc @jfirebaugh @kkaefer @tmpsantos @tmcw

@brunoabinader
Copy link
Member

What about moving these commits not exactly related to the fix to a separated PR?

👍 Moved related (but not necessary to fix the issue) changes to #1888. Thanks!

@brunoabinader
Copy link
Member

Build failure cause is not related to these changes. See #1887 for details.

@brunoabinader
Copy link
Member

This should fix #779 as well.

@mourner mourner removed the ready label Jul 20, 2015
@brunoabinader brunoabinader deleted the 1548-transition-fail-zoom branch July 20, 2015 11:39
brunoabinader added a commit that referenced this pull request Jul 20, 2015
…syncing"

This reverts commit b838816.

It seems 'nudgeTransitions()' is no longer necessary as #1548 has fixed the update issues.
brunoabinader added a commit that referenced this pull request Jul 23, 2015
We're now using nudgeTransitions() to tell the Map view that we want to
update it. However, if we're on a gesture movement, the update() call
can get called too fast and causing general slowdown. This check ensures
we only call for nudgeTransitions() after all gesture events are
finished. Fixes the issue pointed out by #1548 on Android.
brunoabinader added a commit that referenced this pull request Jul 27, 2015
Following the same steps from #1548 and #1912 for GLFW and iOS ports.
Also removed a redundant check if transform state is changing since
we're already dealing with that.
AndwareSsj pushed a commit to AndwareSsj/mapbox-gl-native that referenced this pull request Nov 6, 2015
…syncing"

This reverts commit b838816.

It seems 'nudgeTransitions()' is no longer necessary as mapbox#1548 has fixed the update issues.
AndwareSsj pushed a commit to AndwareSsj/mapbox-gl-native that referenced this pull request Nov 6, 2015
We're now using nudgeTransitions() to tell the Map view that we want to
update it. However, if we're on a gesture movement, the update() call
can get called too fast and causing general slowdown. This check ensures
we only call for nudgeTransitions() after all gesture events are
finished. Fixes the issue pointed out by mapbox#1548 on Android.
AndwareSsj pushed a commit to AndwareSsj/mapbox-gl-native that referenced this pull request Nov 6, 2015
Following the same steps from mapbox#1548 and mapbox#1912 for GLFW and iOS ports.
Also removed a redundant check if transform state is changing since
we're already dealing with that.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants