Avoid animating to the same camera options

[1ec5]

If you attempt to change the camera to an mbgl::CameraOptions that’s identical to the current CameraOptions, mbgl::Transform still goes through the entire animation machinery. We do avoid modifying the TransformState in that case, but by then it’s too late: we’ve already kicked off an animation timer and all the calculations for each step of the animation. The animation timer updates the map on a schedule regardless of whether the transform state changes.

There are multiple places where we can short-circuit the animation, and we should consider all of them to avoid severe energy consumption in navigation use cases, which require constant animation.

Incidentally, when we bail for a no-op animation, should the completion callback by called immediately, or still on a delay?

/cc @kkaefer @friedbunny

[tobrun]

Incidentally, when we bail for a no-op animation, should the completion callback by called immediately, or still on a delay?

I think from end user perspective immediately would make the most sense.

[incanus]

I am going the route here of:

There are multiple places where we can short-circuit the animation

I'm doing this with a few reusable viewport and camera equality check routines and some other slight refactoring to ensure that:

1. We always bail early while still possibly running any completion block (immediately, per above).
2. In future code we don't introduce new paths which circumvent these checks.

Starting with iOS to hit some immediate needs, but will bring this to Android as well.

[incanus]

Beginning in 4ef336e I am gating each viewport change method on MGLMapView (center, zoom, direction, bounds, camera, etc. with or without padding) by checks to bail early (possibly immediately running a passed completion callback in the main thread) before attempting any further mutation of the viewport state.

I also abstracted some routines out that we use more than once and created a set of convenience handlers for checking the viewport changes by passing some or all of these parameters.

This is just iOS to start.

/cc @1ec5 @friedbunny

[1ec5]

I think this is the cruxt of the problem that’s currently affecting user tracking use cases and causing #5833.

The fix in 4ef336e will harden us against this kind of issue, but it should be much more tightly scoped to take advantage of the equality functions we already have in the SDK. Every round trip between MGLMapCamera and mbgl::CameraOptions loses accuracy when the map is tilted, due to #2259.

[incanus]

@1ec5 @friedbunny @boundsj I'm trying to think ahead to how I might write tests for these gates and coming up blank. Basically, we want to ensure that the setters are bailing early if the viewport is unchanged. By virtue of the viewport not getting changed, there isn't really anything to measure concretely.

Any ideas?

🤔

[incanus]

Specifically, it would be nice to be able to determine that either a Core Animation or a core GL transition aren't happening.

[incanus]

Currently trying something around inspecting _mbglMap->transform.inTransition().

[1ec5]

If you take my advice in 4ef336e#commitcomment-18666523, you could call -setCamera:… twice, the second time with a two-second-long animation and a completion handler that satisfies an expectation. Give the expectation only one second to be satisfied.

[incanus]

Nice, I like that idea.

[incanus]

The test approach doesn't work for all of the setters, but it does for a basic camera set with a completion handler, which is a pretty good start. Confirmed that it fails on current master, too.

[jfirebaugh]

Incidentally, when we bail for a no-op animation, should the completion callback by called immediately, or still on a delay?

On a delay. Callback APIs should be async always, not sometimes async and sometimes immediate.

In this case, calling the callback immediately sets users up for a stack overflows reminiscent of #5833 in the case that a completion handler:

a) re-triggers the animation
b) expected the animation not to be a no-op

[jfirebaugh]

Is #2259 the reason #6060 is attempting to fix this at the SDK level, rather than in core? It seems to me that this should really be fixed in core, so that the behavior is guaranteed across all SDKs. If we need to fix #2259 first, so be it.

[1ec5]

I agree #2259 should be fixed in core, but at the same time, I don’t agree that a call to -setDirection: should involve a round trip through mbgl::Map::cameraForLatLngs() just to check whether the passed-in direction is the same as the current direction. Even for -setCamera:, we should make use of (and correct) the -[MGLMapCamera isEqual:] method we already have (and must have for value class semantics and MapKit parity). Within the scope of this ticket, here are the three things (and only three things) that should change:

1. Guarding on -[MGLMapCamera isEqual:] before calling mbgl::Map::cancelTransitions(). (Or getting rid of the call to cancelTransitions() altogether, as proposed in Transition loop and eventual crash at flyTo #5833 (comment).)
2. Correcting -[MGLMapCamera isEqual:] to allow for some imprecision when comparing floats. The existing checks in -setDirection: et al. should similarly allow for some imprecision.
3. Fixing the root cause in core, approximately here, by performing essentially the same comparison as in -[MGLMapCamera isEqual:].

Even if we implement (3), we still need (1) because setters like -[MGLMapView setDirection:] do more than merely move the viewport: they also affect the user tracking mode and implement platform-specific snapping behavior.

[incanus]

Some of what @1ec5 says is why I was inclined to first tackle this in iOS (making sure it solves our downstream issues), then apply the principles to core (while also possibly removing some of the iOS code). Things exactly like user tracking mode, or UI element coordination, are why this needs to happen at least in some part at the platform level.

[incanus]

It's not entirely complete, since I have some other pending refactoring work which also makes use of this, but I introduced a convenience function called MGLPerformAsyncOnMainThread() in a20fefb to address @jfirebaugh's concern in #5983 (comment).

This also moves instances of the always lovely void (^)(void) to dispatch_block_t types for ease of typing and readability. This will get a lot of use since our animation-related callbacks are always return-less, argument-less closures.

Feedback welcome @jfirebaugh @1ec5.

[1ec5]

#7125 is a more tightly scoped take to this problem. It pushes the redundancy checks further down to the three bottlenecks where we call into mbgl to modify the viewpoint. I think my approach is justified because:

• The only additional work that occurs compared to fix #5983: ignore viewport no-ops #6060 is a cheap conversion from MGLMapCamera to mbgl::CameraOptions that has no side effects.
• You can clearly see how we’d push this logic down to mbgl::Transform in the future.
• We don’t round-trip from MGLMapCamera to mbgl::CameraOptions back to MGLMapCamera, so cameraForLatLngs fits to untilted bounds while in perspective view #2259 isn’t a problem because we’re internally consistent.
• It means we don’t have to be more disciplined about adding redundancy checks to any method that under the hood could potentially affect the viewpoint.

Additionally, #7125 triggers the completion handler on a delay in the case of a redundant camera change, per #5983 (comment).

[kkaefer]

You can clearly see how we’d push this logic down to mbgl::Transform in the future.

Why not now?

[mikemorris]

Just flagging that we should make sure to avoid a repeat of #7098 here if we're pushing this deeper into core - the Node.js bindings rely on a stateless camera that resets to default values for consecutive calls to renderStill on the same mbgl::Map object.

[1ec5]

Why not now?

Because there would still be some knots (like the KVO willChange/didChange calls) to untangle so close to a release, with the risk of regressing delegate method coalescing that we've been unable to UI test reliably. Also because the right fix in mbgl has implications for the Android SDK, which is even closer to release. See #5833 (comment).

[1ec5]

This issue – insignificant camera changes that trigger animations in-place – is most likely an edge case that only occurs for instance when the user is stopped at a traffic light and GPS readings bounce around slightly.

#7236 is related and much more acute. We’ll have to do a bit more profiling to know exactly where the issue lies.

[1ec5]

Fixed for iOS and macOS in #7125 on the release-ios-v3.4.0 branch. #7295 tracks a similar issue on Android.