Mapbox-gl-native: Edge padding should not be persisted as content insets

[1ec5]

As of #14664, if a padding (CameraOptions.padding) is explicitly passed into either easeTo() and flyTo(), mbgl persists that padding as the content insets (TransformState.edgeInsets):

https://github.com/mapbox/mapbox-gl-native/blob/b6dd995813625a89bba8937c512cdf1175827d66/src/mbgl/map/transform.cpp#L146-L151 https://github.com/mapbox/mapbox-gl-native/blob/b6dd995813625a89bba8937c512cdf1175827d66/src/mbgl/map/transform.cpp#L310-L315

Content insets are supposed to be persistent, but edge padding is supposed to be transient for the duration of a single camera change operation. In other words, edge padding is supposed to only affect the frame of reference for a camera, not directly influence the map’s overall state.

Because mbgl conflates edge padding with content insets, it isn’t possible to work around #15232 by manually passing in the content inset plus a padding. Now the developer has to reset the content inset in the camera change operation’s completion handler.

/cc @astojilj @mapbox/maps-ios @mapbox/navigation-ios @d-prukop

[astojilj]

@1ec5 ,

Content insets are supposed to be persistent, but edge padding is supposed to be transient for the duration of a single camera change operation. In other words, edge padding is supposed to only affect the frame of reference for a camera, not directly influence the map’s overall state.

14664 made CameraOptions.withPadding(const optional& p) persistent, as all other camera options (pitch, bearing, zoom...).

The behavior of implementation before #14664, threating CameraOptions padding as transient was problematic for cases where user specifies value for CameraOptions.withCenter, but supplied padding alters position (bakes padding to new center) to something else and reset padding to 0.

After #14664, it is not feasible to have it transient because of asymmetric viewport: if there is pitch on map, when padding animation (e.g. easeTo) animates perspective center to new position, we cannot just jump center of perspective back. Neither we should to it - API defined that easeTo() should lead to specified camera padding (perspective center).

This is related to https://github.com/mapbox/mapbox-gl-native/issues/15232#issuecomment-515677390.

[1ec5]

I think this is a misinterpretation of the term “edge padding” as used on iOS up to now: edge padding was never supposed to affect the vanishing point; only the content insets would do that. If edge padding must affect the vanishing point and be persisted, then the iOS map SDK needs to move off that parameter in favor of a separate parameter that only affects how the center point is calculated.

In short, edge padding is a way to say “this screen coordinate should correspond to this geographic coordinate”. It just happens to be defined in terms of padding, but in a future major release, perhaps we could express it as an anchor instead, like we do internally for zooming and rotating.

[astojilj]

@1ec5

If edge padding must affect the vanishing point and be persisted, then the iOS map SDK needs to move off that parameter in favor of a separate parameter that only affects how the center point is calculated.

“this screen coordinate should correspond to this geographic coordinate”
Thank you - this clarifies the use case.

Let's discuss on about separate method.

Padding is used for iOS's MGLMapView.contentInset (non user tracking mode) and Android's setPadding. Those use jumpTo/flyTo and it has persistent nature.

Would CameraOptions.withAnchorAt(ScreenCoordinate, LatLng) implement the desired behavior?
But then, should we would keep it persistent?
Or, it is enough to implement calculateMapCenterForScreenCoordinateAt and use existing methods?

[1ec5]

Would CameraOptions.withAnchorAt(ScreenCoordinate, LatLng) implement the desired behavior?
But then, should we would keep it persistent?

Yes, I think withAnchorAt() would be suitable, but I want to clarify that the map SDK’s public API shouldn’t necessarily change as a result of fixing this regression. Ideally we’d be able to calculate the right anchor respecting any existing content insets. In the future, we could expose something like a centeredUponPoint: parameter so it’s more intuitive.

Or, it is enough to implement calculateMapCenterForScreenCoordinateAt and use existing methods?

Would that work correctly even if the camera changes the pitch, rotation, or zoom level?

[1ec5]

This issue also affects some operations internal to MGLMapView. For example, -[MGLMapView showAnnotations:animated:] adds a padding to keep the annotations from being flush against the edges of the map, but this padding shouldn’t affect the vanishing point or be persisted:

https://github.com/mapbox/mapbox-gl-native/blob/b6dd995813625a89bba8937c512cdf1175827d66/platform/ios/src/MGLMapView.mm#L5055-L5059

When course tracking is turned on, -[MGLMapView didUpdateLocationIncrementallyAnimated:completionHandler:] applies a shifted padding on each location update, as discussed in https://github.com/mapbox/mapbox-gl-native/issues/15165#issuecomment-513335720.

Meanwhile, I think -[MGLMapView setCenterCoordinate:zoomLevel:direction:animated:completionHandler:] and -[MGLMapView flyToCamera:withDuration:peakAltitude:completionHandler:] have the incorrect fallback too:

https://github.com/mapbox/mapbox-gl-native/blob/6f0cda8c8b8e7831f04ae5e00b13387107dcad9b/platform/ios/src/MGLMapView.mm#L3162 https://github.com/mapbox/mapbox-gl-native/blob/6f0cda8c8b8e7831f04ae5e00b13387107dcad9b/platform/ios/src/MGLMapView.mm#L3582

/cc @fabian-guerra @friedbunny @captainbarbosa

[astojilj]

@1ec5 , @fabian-guerra

I debugged the code and it is also induced by expanded top banner - https://github.com/mapbox/mapbox-navigation-ios/issues/2165

The attached video shows the behavior:
RPReplay_Final1565363696.mp4.zip

In code, this causes relayout and then a change in content insets in RouteMapViewController viewDidLayoutSubviews

For a brief period, instructionBannerHeight gets changed, increasing and then decreasing content insets.

Note that, as of #14664, content insets also became animatable property... and that is what we see with puck position animated back. This is why I'm thinking that the best solution for this is to fix content insets in mapbox-navigation-ios during navigation - if you're fine with it, could work on proposing a patch.

[1ec5]

I debugged the code and it is also induced by expanded top banner - mapbox/mapbox-navigation-ios#2165

Note that the issue reproduces even if the user never interacts with the top banner and the top banner never expands.

[astojilj]

I debugged the code and it is also induced by expanded top banner - mapbox/mapbox-navigation-ios#2165

Note that the issue reproduces even if the user never interacts with the top banner and the top banner never expands.

If you have a video prepared, please attach.

I used println for analysis, and noticed a short change in instructionBannerHeight, even though the "turn left" or "turn right" banner expansion is not visible. I'm working on patch for it and let's verify it after...

[astojilj]

@1ec5

Please check PR to navigation-ios Simplify anchor point calculation and puck centering #2211 .

This is a change of anchor and padding handling in navigation-ios.
This should void the need for change in gl-native as I'd like to keep the current implementation here and close this issue once client code adapts to it.

You also mentioned that there is a need to inform another customer potentially copying the same anchor point - padding approach as it is in navigation-ios.

[1ec5]

If you have a video prepared, please attach.

Here’s a screen recording of navigation SDK v0.36.0 with map SDK v5.2.0 in an iPhone 8 / iOS 12.4 simulator simulating a route from 39.10128°N, 84.51134°W to 39.09784°N, 84.51383°W along Walnut, Fourth, and Race streets.

The puck is in the correct place at all times, but the camera is positioned incorrectly with respect to the puck. Watch as the camera pivots around the intersection while the puck is floating a certain screen distance above it. From the point of view of a consumer of the map SDK, there should be nothing wrong with positioning a view at a screen coordinate and independently centering the camera over that same screen coordinate using a padding.

I debugged the code and it is also induced by expanded top banner - mapbox/mapbox-navigation-ios#2165

mapbox/mapbox-navigation-ios#2165 is a red herring – please disregard. The only reason it came up was because the developer thought it was related to a bug in the step list, which is wholly independent of the map view.

For a brief period, instructionBannerHeight gets changed, increasing and then decreasing content insets.
I used println for analysis, and noticed a short change in instructionBannerHeight, even though the "turn left" or "turn right" banner expansion is not visible.

When a “then” banner appears and disappears, the combined height of the top bars does change, and the map view’s visual center and the puck’s position should change as a result. Admittedly it is suboptimal that the map only gets recentered upon the next location update, not immediately when the content insets change, but that’s orthogonal to this issue.

This should void the need for change in gl-native as I'd like to keep the current implementation here and close this issue once client code adapts to it.

To reiterate: from the perspective of the iOS/macOS map SDK’s public API, ignoring mbgl, the edgePadding parameter of -[MGLMapView setCamera:withDuration:animationTimingFunction:edgePadding:completionHandler:] should be transient, only applicable to the camera change operation. That is, MGLMapView should use the edge padding to translate the centerCoordinate property to a screen coordinate – nothing more.

Taking a step back, the rationale for an edge padding property is to ensure that the centerCoordinate is offset from the map view’s actual center. It happens to be expressed as an edge padding because edge padding tends to remain constant as a view resizes, unlike a hypothetical anchor property relative to an origin at the top-left corner. The thinking was that the developer would be able to plug in the same banner view height as an edge padding rather than have to recalculate a visual center point each time they move the camera.

What we’ve learned since #3583, particularly bf87eaa7b8aa049358559a96f290603e13ac736b, is that developers normally want the ability to specify an anchor point (in screen coordinates) for the center (geographic) coordinate. Expressing it as edge padding is actually less direct. So if mbgl::Transform supports mbgl::CameraOptions::anchor for panning as well as zooming and rotation, then MGLMapView should translate the edge padding to anchor for now. In the future, we could replace the edgePadding parameter with an anchoredAtPoint parameter.

[astojilj]

The puck is in the correct place at all times, but the camera is positioned incorrectly with respect to the puck. Watch as the camera pivots around the intersection while the puck is floating a certain screen distance above it. From the point of view of a consumer of the map SDK, there should be nothing wrong with positioning a view at a screen coordinate and

The screenshot is from attached video. This doesn't look correct - is it supposed to be like that?

I assumed it is supposed to be like in attached screenshot (this is behavior with https://github.com/mapbox/mapbox-navigation-ios/pull/2211):

mapbox/mapbox-navigation-ios#2165 is a red herring – please disregard. The only reason it came up was because the developer thought it was related to a bug in the step list, which is wholly independent of the map view.

It is triggering issue with position (padding animation introduced with #14664) kicks in. The problem is not visible after https://github.com/mapbox/mapbox-navigation-ios/pull/2211

[astojilj]

@1ec5

Admittedly it is suboptimal that the map only gets recentered upon the next location update, not immediately when the content insets change, but that’s orthogonal to this issue.

Noticed. This is fixed by https://github.com/mapbox/mapbox-navigation-ios/pull/2211.

[astojilj]

@1ec5

Need help with this:

Because mbgl conflates edge padding with content insets, it isn’t possible to work around #15232 by manually passing in the content inset plus a padding. Now the developer has to reset the content inset in the camera change operation’s completion handler.

Padding and content inset are not fused in MGLMapView - MGLMapView holds value of specified content inset and adds additional supplied padding.

What is the use case where reset in completion handler is needed - is there an existing code I could check? Thanks.

[1ec5]

The screenshot is from attached video. This doesn't look correct - is it supposed to be like that?

The video depicts the current broken state as of navigation SDK v0.36.0 and map SDK v5.2.0, not how it’s supposed to look. The _puck_ is in the correct position but the _camera_ is wrong.

[1ec5]

Padding and content inset are not fused in MGLMapView - MGLMapView holds value of specified content inset and adds additional supplied padding.

This code modifies mbgl::TransformState::edgeInsets by side effect:

https://github.com/mapbox/mapbox-gl-native/blob/b6dd995813625a89bba8937c512cdf1175827d66/src/mbgl/map/transform.cpp#L310-L315

The next time mbgl::Transform::easeTo() is called, mbgl::TransformState::edgeInsets is ignored _unless_ mbgl::CameraOptions::padding is unset, due to #15232. So if the next call to easeTo() comes with no edge padding because the developer’s intent is to have the camera flush with the inset content rect on all sides, they get whatever mbgl::CameraOptions::padding was previously passed into mbgl::Transform::easeTo(), not the mbgl::Transform::edgeInsets that would’ve been set via -[MGLMapView setContentInsets:].

I think what you’re saying is that the padding-less call will never happen because MGLMapView always adds contentInsets to edgePadding. If so, is mbgl::Transform::edgeInsets just a temporary scratch area for use during the animation, to be ignored otherwise? That could be made more clear by removing the value_or():

https://github.com/mapbox/mapbox-gl-native/blob/b6dd995813625a89bba8937c512cdf1175827d66/src/mbgl/map/transform.cpp#L85

[1ec5]

The most straightforward fix for this issue would be for mbgl::CameraOptions to distinguish between the current definition of padding and the old one, which we can call centerOffset. Aside from adding a new field to mbgl::CameraOptions, that would mean restoring some methods like Transform::getLatLng(const EdgeInsets&, LatLng::WrapMode). I don’t know if the math in those methods remains accurate now that the viewport can be asymmetric. Basically there needs to be a way to say “consider centerCoordinate to be the coordinate at _this_ center, not the usual center”.

The iOS/macOS map SDK’s -[MGLMapView cameraOptionsObjectForAnimatingToCamera:edgePadding:] method – which returns an mbgl::CameraOptions that gets passed into mbgl::Map::easeTo() – could set mbgl::CameraOptions::centerOffset to the edgePadding parameter and mbgl::CameraOptions::padding to the value of MGLMapView.contentInsets.

If the idea of a “center offset” is anathema, then we should find a way to decouple the centerCoordinate camera parameter from the idea of a “center” at all. Rather, the developer should be able to say “this LatLng should end up at this ScreenCoordinate”.

Until then, the fact that Transform conflates the concepts of edge padding and content insets will continue to cause frustration for anyone who implements a navigation-centric map view on any platform.

/cc @chloekraw @tmpsantos @asinghal22