Region change notifications should include camera delta

[1ec5]

Calls to mbgl::View::notifyMapChange() should include a “camera delta” struct alongside the mbgl::MapChange that indicates how far the camera has moved in each degree of freedom.

The delta would allow the “should change” delegate callback in #2457 to provide enough context for the delegate to make an intelligent decision. It would also allow us to implement view-based annotations (#1784) without requiring a round trip to mbgl to convert from geographic coordinates to screen points. (This would be much more of a performance issue on Android due to JNI round tripping.)

/cc @tobrun @friedbunny @brunoabinader

[tobrun]

This would make not only the View Marker API #3276 so much more feasible, it will also improve current implementation of Projection.java class. Thank you @1ec5 for ticketing this out.

[jfirebaugh]

Do we know that a round trip to mbgl to convert from geographic coordinates to screen points would actually be a performance issue?

[jfirebaugh]

The concern with summation of deltas is that it is subject to drift due to accumulated imprecision.

[1ec5]

A more direct way of getting what we need for view-based annotations would be to proactively send the screen coordinates of on-screen annotations on RegionDidChange and RegionIsChanging. But I suggested a delta because sending the screen coordinates for all the relevant annotations on each RegionIsChanging sounded like a performance hit.

[1ec5]

Along these lines, it may be useful to allow an mbgl::View to register for additional context to accompany RegionDidChange and RegionIsChanging notifications, such as the result of Map::getPointAnnotationsInBounds().

[1ec5]

Filed #4694 for automatically getting annotations in the new viewport.

[1ec5]

Deferring to ios-v3.4.0 because we aren’t comfortable with making the necessary mbgl changes this close to a release.

[brunoabinader]

With #8377 we have now means of passing additional information (such as CameraOptions) to MapObserver camera notifications.

However, Transform object does not store the edge insets information - which is always sent by the client upon requesting e.g. the camera options. This could potentially cause the notification to send an incorrect camera options that did not take the padding informed by the edge insets in consideration.

Changing this behavior implies in modifying our Map API to add a specific getter/setter pair for the edge insets and thus removing it from function calls such as getCameraOptions, setLatLng and etc. This way Transform would know which edge inset to take in consideration when notifying on camera changes.

/cc @1ec5 @jfirebaugh

[1ec5]

With #8377 we have now means of passing additional information (such as CameraOptions) to MapObserver camera notifications.

Note that this issue originally requested the previous camera or a delta from the previous camera, not the current camera, which the SDK code can easily obtain from the map.

However, Transform object does not store the edge insets information - which is always sent by the client upon requesting e.g. the camera options. This could potentially cause the notification to send an incorrect camera options that did not take the padding informed by the edge insets in consideration.

I still think it’s appropriate that, from mbgl’s perspective, the map has no intrinsic padding. The iOS SDK, for example, takes advantage of this fact to allow for different paddings simultaneously: one that affects the centerCoordinate getter and some gestures, and another that affects the placement of the user dot and movement of the camera during user tracking mode.

[brunoabinader]

Note that this issue originally requested the previous camera or a delta from the previous camera, not the current camera, which the SDK code can easily obtain from the map.

Yes, though I agree with @jfirebaugh that dealing with deltas on core side can cause precision loss.

To avoid that, the delta could be calculated from client side - we could e.g. add a CameraOptions CameraOptions::compareTo(const CameraOptions& other) to make that step easier.

[1ec5]

Actually, I’m less sure that we need camera deltas at all, given the discussion above and the way #2457 was implemented. What’s most important for public notifications is indicating the final camera at the beginning of an animation. The initial camera at the beginning of an animation could be helpful, too, but the SDK can already provide this information by capturing the camera ahead of time.

[jfirebaugh]

What’s most important for public notifications is indicating the final camera at the beginning of an animation.

@1ec5 Can you open a fresh ticket for that?

Closing here since it sounds like we have consensus that deltas aren't necessary.

[1ec5]

What’s most important for public notifications is indicating the final camera at the beginning of an animation.

The saga continues in #8499.