Short-circuit redundant camera changes #7125
Conversation
1ec5
added
iOS
| XCTAssertEqualObjects(tester.mapView.camera, camera); | ||
|
|
||
| [tester.mapView setCamera:camera withDuration:10 animationTimingFunction:nil completionHandler:^{ | ||
| XCTAssert(NO, @"Camera animation should not be canceled by redundantly setting the camera to the current camera."); |
There was a problem hiding this comment.
This is the best I could come up with for a test of the redundancy check. It essentially only verifies that cancelTransitions() isn’t called in the case of a redundant camera change. The test in #6060 is infeasible because the completion handler is eventually called even for redundant camera changes.
I verified via breakpoints that we do short-circuit mbgl for redundant camera changes and that significant camera changes still work.
include/mbgl/map/camera.hpp
Outdated
| @@ -34,6 +34,31 @@ struct CameraOptions { | |||
| /** Pitch toward the horizon measured in radians, with 0 rad resulting in a | |||
| two-dimensional map. */ | |||
| optional<double> pitch; | |||
|
|
|||
| bool operator==(const CameraOptions& o) const { | |||
| if ((!center && o.center) || (center && !o.center) || | |||
There was a problem hiding this comment.
Why isn't this defined simply as memberwise ==?
There was a problem hiding this comment.
Due to the float casts, which are needed due to slight imprecision – comparing doubles never returns true in practice. Or should a fuzzy equals be a separate method for clarity?
There was a problem hiding this comment.
I should probably bring over the six-decimal-place rounding from #6060.
1ec5
force-pushed
the
1ec5-camera-noop-5983
branch
2 times, most recently
from
8259e2c
to
ec6c1a1
Compare
|
I’ve pared back the change to touch only SDK code and also leave the normal |
1ec5
force-pushed
the
1ec5-camera-noop-5983
branch
from
ec6c1a1
to
a3a8a74
Compare
platform/ios/src/MGLMapView.mm
Outdated
| @@ -2415,9 +2427,7 @@ - (void)setVisibleCoordinates:(const CLLocationCoordinate2D *)coordinates count: | |||
|
|
|||
| - (void)_setVisibleCoordinates:(const CLLocationCoordinate2D *)coordinates count:(NSUInteger)count edgePadding:(UIEdgeInsets)insets direction:(CLLocationDirection)direction duration:(NSTimeInterval)duration animationTimingFunction:(nullable CAMediaTimingFunction *)function completionHandler:(nullable void (^)(void))completion | |||
| { | |||
| _mbglMap->cancelTransitions(); | |||
|
|
|||
There was a problem hiding this comment.
Nit: Inconsistent line break.
Avoid canceling transitions (and triggering preexisting completion handlers) until we get a chance to ensure that a new transition really does have to begin. Consistently avoid mbgl transitions for redundant camera changes. Upon bailing, schedule the completion handler to run asynchronously on a delay equivalent to the requested animation duration. Added a “functional” equality method to MGLMapCamera that avoids trivial differences. Fixed invocations of XCTAssertEqualWithAccuracy() that incorrectly expressed the accuracy as a number of digits rather than a scalar tolerance.
1ec5
force-pushed
the
1ec5-camera-noop-5983
branch
from
a3a8a74
to
d790132
Compare
Avoid canceling transitions (and triggering preexisting completion handlers) until we get a chance to ensure that a new transition really does have to begin. Consistently avoid mbgl transitions for redundant camera changes. Upon bailing, schedule the completion handler to run asynchronously on a delay equivalent to the requested animation duration.
MGLMapCamera now performs a float-precision equality check to avoid trivial differences. Added an equality operator to CameraOptions that similarly uses float-precision.Added a “functional” equality method to MGLMapCamera that avoids trivial differences.Having made these changes, I very much want to move these redundancy checks into
mbgl::Transform. However, that depends on eliminating the numerous SDK-sidecancelTransitions()calls: #5833 (comment). This PR is less risky but should address the performance issues the iOS SDK is experiencing.This change also fixes invocations of
XCTAssertEqualWithAccuracy()that incorrectly expressed the accuracy as a number of digits rather than a scalar tolerance.Fixes #5983.
/cc @incanus @jfirebaugh