Flickering when crossing international date line

[kkaefer]

When panning the map across the international date line that map tiles seem to flicker:

cc: @kkaefer

[mention-bot]

@kkaefer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jfirebaugh, @brunoabinader and @1ec5 to be potential reviewers.

[kkaefer]

What's happening here is the following: We use the concept of "unwrapped" tiles. Those are tile IDs that have an additional "wrap" component that indicates the offset of the pyramid, e.g. in the situation above, we are rendering the tile 0/0/0 with two "wrap offsets" of 0 and 1 to cover the entire viewport. At some point, when you are panning beyond 180°, we wrap around and continue at −180°. However, there's a disconnect between when we calculate what tiles we need for rendering, and the actual render. Here's the timeline that produces tile flickers:

1. user is flicking/panning the map sideways; we start a transition that gradually updates the location based on the time.
2. asynchronous state update determines the tiles we need
3. rendering: we update lat/lon based on the location transition and just happen to wrap around from 180° to −180° (or vice versa), which means we now need to render different tiles, but we didn't recalculate the tiles we need to display. We were still rendering both tiles, but the second tile was rendered at the wrong (off-screen) location.

[brunoabinader]

Neat :)

[jfirebaugh]

The return value doesn't seem to be used.

[jfirebaugh]

This seems like it's changing some pretty fundamental parts of the render loop. What kind of testing have you done?

[jfirebaugh]

@kkaefer asked me to list some more specific concerns:

• Could this regress performance? Does it require doing more work on a per-frame basis, or during the frame render (as opposed to a more "idle" point in the loop)?
• Could this regress camera animations?
• Could this regress synchronization between GL canvas and iOS/Android native views?
• Could this cause a regression due to reordering when state updates take place? Style has a lot of implicit state, and it tends to depend on methods getting called in a particular order, which this change could perturb.

[jfirebaugh]

Past issues that give a sense of the subtleties: #1813, #1904, #1975, #1979, #2207, #5833.

[jfirebaugh]

It looks like the main substance of the change here is to move the majority of Map::Impl::update into a new function Map::Impl::updateState, which can be called from both update and render. This is a substantial change -- updateState potentially does a lot of things that haven't historically been done from inside render. (We can't assume that what updateState does inside render is determined only by the return value of updateTransitions. That value is or'd with updateFlags, which could contain any flags.)

Here are two alternate options that seem better to me:

• Do all updates inside render, immediately before actually rendering the frame. update becomes a function that does nothing but call backend.invalidate(). This is the rendering model that mapbox-gl-js uses, and it's worked well there. It's at least as substantial a change as the current diff here, but leads to a less complex rendering model because there's only one time point in the rendering loop at which updates happen, rather than two.
• Move updateTransitions out of render, into update, possibly adding an Update::Transitions flag if needed. It's not clear to me why updateTransitions is the one type of update that happens in render rather than update. I think this might simply be a historical accident. I like the idea of consolidating, and it seems a lot less risky than the other alternatives. I don't think this would regress GL canvas / native view synchronization because that's based on MapChange notifications, which are sent in render.

Did you try either of these approaches, or see any obvious issues with them that I'm missing?

[kkaefer]

I've tried your second suggestion, and now remembered that I had tried it but discarded: It works well on iOS + macOS, since something in its UI logic seems to guarantee that all user interactions (like panning) are dispatched into state transform calls on the map object (e.g. map.setLatLng()), then update(), and render() are called.

However, when adding logging to Map::Impl::update(), Map::Impl::render(), and TransformState::setScalePoint(), I observed that Android doesn't seem to do that. I frequently got events in the order of update(), setScalePoint(...), render(). This means that we've mutated the state after we've determined the unwrapped tiles we need to render.

This means the only safe way of how this could work is the first alternative: completely abandon the concept of "updates without render". The separation of update and render originated in the assumption that we know that the user panned/zoomed the map prior to the UI framework telling us to render, and that we could get an edge to downloading/parsing tiles before rendering. Meanwhile, I don't believe this has ever been true, since the time between two successive render attempts is extremely short anyway, and render only uses the tiles that are already completely parsed.

The other case where this was used is for static rendering: Whenever you change something about a map object, we do an update, which determines the correct tiles to load and starts loading them. However, we already have a safeguard built into our render call that doesn't render until all tiles have been loaded and parsed, so we could just replace calls to update with calls to render in the static case.

[kkaefer]

I've tried implementing the first suggestion as well, but contrary to what I wrote in my previous comment, I don't think it's viable either: While it works perfectly fine for continuous rendering (iOS/Android), it doesn't work for static map rendering: invalidate() isn't implemented for the static view (and can't be because the point of that call is to pass it through to the UI system). Instead, we'll have to use RunLoop-based version of triggering an action through the asyncUpdate call. We can't wire asyncUpdate to render(), since it might be called in a situation where we can't use OpenGL (e.g. on UI-based systems), which leads me to believe that we'll have to maintain the separation between update and render.

Another alternative to calling update on render is to persist the TransformState we produce after an update call and use that state when rendering. This means we'll ignore any state updates that trickle in between update and render. which only seems to happen on Android.

[kkaefer]

An alternative fix is in https://github.com/mapbox/mapbox-gl-native/compare/7040-dateline-flickering-alternative. It moves transition updates to update, and stores the TransformState after every update for use in render. This means that render no longer uses the most current state, since using that would require another call to update (which is what the PR in this thread is about). As for testing the alternative fix, we'd need to verify that platform markers remain in sync with the rendered map.

[jfirebaugh]

However, when adding logging to Map::Impl::update(), Map::Impl::render(), and TransformState::setScalePoint(), I observed that Android doesn't seem to do that. I frequently got events in the order of update(), setScalePoint(...), render(). This means that we've mutated the state after we've determined the unwrapped tiles we need to render.

Yeah, if the SDK code interleaves state updates in between backend.invalidate() and render(), then we have issues. But doesn't the fact that Android do that mean that there's an issue already, with the current code? Does setScalePoint(...) not mutate state in a way that would cause the same sort of flickering as transform.updateTransitions()?

The first option, doing all updates immediate prior to rendering the frame, with no opportunity for the SDK to interleave state changes, is definitely the safest and easiest to reason about. It seems like it could be made to work for static rendering: wire asyncUpdate to render, like you suggest, but then don't even use asyncUpdate in continuous mode. Instead call backend.invalidate() directly, relying on the platform SDK to do coalescing. Here's a prototype.

[kkaefer]

Prototype looks good! One minor drawback I'm seeing is that our typical static workflow of "update all transform state, then call renderStill will now also invoke style->updateTiles on every render attempt (which happen whenever a new resource or tile is loaded), so rather an calling update just once (coalesced), it'll be called multiple times (my guess would be ~5-10 times).

[jfirebaugh]

Yeah, probably we should have an Update::Tiles flag and conditional around style->updateTiles, like all the other updates.

[kkaefer]

I tested it and it seems that we've been calling updateTiles multiple times before as well. Let's not include the addition of a Update::Tiles flag into this PR, and instead do it in a separate one, along with #7536

Updated changes

[jfirebaugh]

Replaced by #7574.