Strict render step to Map::render{Sync,Still}

[brunoabinader]

First, a little background: Map context thread is responsible for two main tasks:

1. Update render objects.
2. Render objects to screen.

Such tasks are independent and can be pipelined. However, only task 1 can be called asynchronously, while task 2 needs to be done synchronously. Currently we have the following issues:

a. Task 1 causes 2 to be done outside of view invalidation call. Instead, we should always render inside view invalidation call.
b. Task 1 has to be manually triggered via Map::nudgeTransitions() when a transition/animation is in progress. Instead, map context should be smart enough to trigger task 1 asynchronously after completion of task 2.

With that in mind, I propose the following fixes:

• Get rid of Map::nudgeTransitions; Render updates (step 1) are now asynchronously triggered by MapContext::renderSync whenever required.
• Get rid of MapData::{get,set}NeedsRepaint; This synchronisation flag is no longer required as MapContext should be robust enough to asynchronously trigger a view invalidation (step 2) if a transition/animation is in progress.
• Provide an asynchronous handler for view invalidation in MapContext. This is handy because while step 2 needs to be done synchronously between Map and MapContext threads, view invalidation itself can happen asynchronously.
• Because transform transition updates only in Map thread, Map::renderSync may call for Map::update to trigger an asynchronous update if Transform contains a transition in progress.

/cc @tmpsantos @jfirebaugh @kkaefer @incanus

[brunoabinader]

Tested on Android (Samsung Galaxy S5) and iOS (iPad mini) 👍

[friedbunny]

This looks fine to me on iPhone 5 — user dot appeared to behave properly and update as expected.

[incanus]

I don't think this is workable yet on iOS @brunoabinader. While I don't see a hard lag between native view draw location and GL frames, there still is some sort of out-of-sync-ness as the user dot "floats" a bit despite not changing actual geo location during animated pans.

I just don't think that it is possible to have a satisfactory experience without this sort of flow:

1. GL frame draw.
2. Sync native view location to same TransformState as frame draw.
3. Update TransformState.

This is why in master we currently updateUserLocationAnnotationView() between renderSync() and nudgeTransitions() (and is why nudgeTransitions() exists separately).

Backstory: #1813

Last regression: #1904

[jfirebaugh]

Yeah, +1 to what @incanus said. This change has the effect of changing the sequence to:

1. GL frame draw
2. Update TransformState (via renderSync ⇢ update(Update::Nothing) ⇢ updateTransitions)
3. Sync native view location to different TransformState as frame draw

This will result in desynced native views.

It's possible that the following sequence would work:

1. Update TransformState
2. GL frame draw
3. Sync native view location to same TransformState as frame draw

This order makes more intuitive sense than updating TransformState post-draw. There was some reason we didn't do it this way to begin with, but I can't remember what it was. If you can make it work, I'm 👍.

[incanus]

I am still seeing stuttering render problems and an extremely jittery user dot as of 981de40. The panning is rock solid now, but pinch-zooming with inertia ("pinch fling") causes the user dot to jitter a ton.

The best way to replicate this class of problems is to use the Xcode simulated city locations, since they don't have any GPS variance and are hardcoded single geo points.

This applies to device as well, so be sure to run on device from Xcode once without geo simulation if you want your original location and timezone back! 😉

[1ec5]

I’m seeing this jitter without inertial zooming – just plain zooming. It also appears to be jittering by a fixed amount, so there’s no perceptible jitter at say z14.

[incanus]

Confirmed the jitter is in master (ticketed at #2230).

I am otherwise 👍 here @brunoabinader as of 981de40 pending tests.

I don't yet fully understand, but it sounds like the gist is that we swap the TransformState update and the renderSync per #2207 (comment), correct?

Nice work!

[friedbunny]

You missed the opportunity to say "jitter bug" here, @incanus.

[brunoabinader]

I don't yet fully understand, but it sounds like the gist is that we swap the TransformState update and the renderSync per #2207 (comment), correct?

Yes :-) TransformState is updated when we run a step of TransitionFrameFn with the current clock time. After that we synchronously call for the MapContext::renderSync which will draw all the things and then finally, if an update is still required (eg. due to an ongoing transform transition, let's say a rotateBy() with 500ms duration), we call for an asynchronous update in Map thread (done in Map::update()).

[incanus]

You missed the opportunity to say "jitter bug" here

[jfirebaugh]

@brunoabinader node branch is merged in. Providing @incanus still has appetite for this change WRT the mobile releases this week, feel free to rebase and merge.