Simplify anchor point calculation and puck centering #2211

astojilj · 2019-08-12T15:26:05Z

Decouple dependency content insets -> anchor point -> padding/content insets broken after mapbox/mapbox-gl-native#14664 introduced animated interpolation for padding change.

Remove the need to specify center for puck view - use the approach where content inset is keeping it centered.

Take safeArea into account when calculating contentInsets.

Link to video showing current state.

EDIT 9/9/2019:
Work around mapbox-gl-native/15574. Panning fix.

Fix CarPlay puck positioning, panning away from navigation, navigation -> overview transitions.
Remove syncing safeArea to contentInset, contentInset is to be used to position puck and this causes map position sudden change when panning.

Work around mapbox-gl-native/15574: should be future proof as the mapbox/mapbox-gl-native#15574 exists only with MGLMapView.contentInset.

Addresses: mapbox/mapbox-gl-native#15232, mapbox/mapbox-gl-native#15233

Related to: #2165,

Fixes: #2145, #2190

1ec5 · 2019-08-14T04:12:54Z

MapboxNavigation/RouteMapViewController.swift


        if overviewing {
            insets += NavigationMapView.courseViewMinimumInsets

            let routeLineWidths = MBRouteLineWidthByZoomLevel.compactMap { $0.value.constantValue as? Int }
            insets += UIEdgeInsets(floatLiteral: Double(routeLineWidths.max() ?? 0))
+        } else if mapView.tracksUserCourse {


RouteMapViewController is used by NavigationViewController but not CarPlayNavigationViewController. Developers are also responsible for implementing their own preview map or free-driving map using a standalone NavigationMapView. So this code would need to be duplicated in CarPlayNavigationViewController and application code. It would be better to keep the code in NavigationMapView if possible.

After 98551dd

CarPlayNavigationViewController and NavigationViewController contentInset() functions are very close and seems feasible to extract common part to NavigationMapView.
As I don't have enough understanding of navigation-ios API surface. it is better if someone else from navigation-ios team continue on formulating the API, that is if the workarounds taken here are fine.

We’re going to address duplication between CarPlayNavigationViewController and NavigationViewController overall in a separate PR ahead of v1.0.

1ec5 · 2019-08-14T04:19:31Z

MapboxNavigation/NavigationMapView.swift

@@ -194,8 +183,6 @@ open class NavigationMapView: MGLMapView, UIGestureRecognizerDelegate {
            if let userCourseView = userCourseView {
                if let location = userLocationForCourseTracking {
                    updateCourseTracking(location: location, animated: false)
-                } else {
-                    userCourseView.center = userAnchorPoint


Remove the need to specify center for puck view - use the approach where content inset is keeping it centered.

We need to check whether undoing #2047 ends up regressing #1731 and #1949. Also, does the puck consistently remain at userAnchorPoint when simulation is sped up?

/ref @frederoni

Also, does the puck consistently remain at userAnchorPoint when simulation is sped up?

Yes, it remains. It is always centered to padding area and no need to reposition it. Attached video to show:
https://www.dropbox.com/s/wori62tfhp69feg/RPReplay_Final1565767435.mp4?dl=0

Also, the issue here is fixed: mapbox/mapbox-gl-native#15233 (comment)

1ec5 · 2019-08-14T04:20:43Z

Addresses: mapbox/mapbox-gl-native#15232, mapbox/mapbox-gl-native#15233

This is at most a workaround. A downstream change does not fix an upstream regression.

… safeArea. Panning fix. Fix CarPlay puck positioning, panning away from navigation, navigation -> overview transitions. The workaround should be future proof as the mapbox/mapbox-gl-native#15574 exists only with MGLMapView.contentInset. Additionally, it simplifies the code as it doesn't need to ensure that safeArea and contentInset are kept in sync: according to work in #2211, contentInset is to be used to position puck and this causes map position sudden change when panning. Fixes: #2190

noway · 2019-09-26T13:37:31Z

When this will be merged? We can no longer use mapbox navagtion 0.34 with mapbox ios sdk 5.0.0 because it fails to compile with Swift 5.1 (which is forced with Xcode 11.0). Having to update to 0.37, this is a critical bug which blocks any releases we can do to our app. 😣

Having this bug in production is not an option because it critically deteriorates the user experience which was working well before.

If there is any work around, or if this issue can be prioritised, that would be greatly appreciated.

astojilj · 2019-09-26T13:47:13Z

When this will be merged? We can no longer use mapbox navagtion 0.34 with mapbox ios sdk 5.0.0 because it fails to compile with Swift 5.1 (which is forced with Xcode 11.0). This is a critical bug for us which blocks any releases we can do to our app. 😣

Having this bug in production is not an option because it critically affects the user experience

It is currently in review and some manual verification is needed. Did you try if it fixes the issues in your application?
Thanks.

cc @frederoni @1ec5 @fabian-guerra

noway · 2019-09-26T14:17:16Z

WORKS FABULOUSLY! Even closing the top nav bar stopped crashing the app!

For anyone who's coming across this, the commit id for this PR's branch is:

github "mapbox/mapbox-navigation-ios" "98551ddcbf5934226702dc0ce710266d86f809ce"

Thanks!

1ec5 · 2019-09-27T17:44:11Z

This PR will go in as soon as we’re able to verify that it doesn’t break navigation on a CarPlay device. In the meantime, if you don’t use CarPlay functionality, feel free to use this branch as described above.

Decouple dependency content insets -> anchor point -> padding/content insets broken after mapbox/mapbox-gl-native#14664 introduced animated interpolation for padding change. Remove the need to specify center for puck view - use the approach where content inset is keeping it centered. Take safeArea into account when calculating contentInsets. Addresses: mapbox/mapbox-gl-native#15232, mapbox/mapbox-gl-native#15233 Related to: #2165, Fixes: #2145

… safeArea. Panning fix. Fix CarPlay puck positioning, panning away from navigation, navigation -> overview transitions. The workaround should be future proof as the mapbox/mapbox-gl-native#15574 exists only with MGLMapView.contentInset. Additionally, it simplifies the code as it doesn't need to ensure that safeArea and contentInset are kept in sync: according to work in #2211, contentInset is to be used to position puck and this causes map position sudden change when panning. Fixes: #2190

1ec5

Thanks again for contributing these workarounds! As noted above, the changes do address the map SDK regressions. #2246 and #2247 track regressions caused by this PR that, compared to #2145 and #2190, are relatively minor. #2248 tracks the removal of the workarounds added in this PR.

astojilj requested review from frederoni, 1ec5 and fabian-guerra August 12, 2019 15:26

astojilj self-assigned this Aug 12, 2019

astojilj mentioned this pull request Aug 12, 2019

Edge padding should not be persisted as content insets mapbox/mapbox-gl-native#15233

Open

1ec5 reviewed Aug 14, 2019

View reviewed changes

astojilj mentioned this pull request Aug 14, 2019

Remove workarounds adding content inset when setting map camera #2145

Closed

1ec5 added the topic: camera label Aug 14, 2019

astojilj force-pushed the astojilj-15233 branch from 34822f9 to 98551dd Compare September 9, 2019 07:26

astojilj requested a review from 1ec5 September 9, 2019 07:42

astojilj mentioned this pull request Sep 18, 2019

CarPlay route preview does not fit to the route #2190

Closed

noway mentioned this pull request Sep 26, 2019

Navigation user puck arrow is incorrect when we zoom in to map and resume #2242

Closed

astojilj added 3 commits October 2, 2019 08:22

Don't move mapView content when panned away from tracking user

c4ac726

1ec5 force-pushed the astojilj-15233 branch from 98551dd to 2e540c0 Compare October 2, 2019 15:22

Cleaned up semicolons

e137bda

This was referenced Oct 2, 2019

User puck detaches from route line when panning in CarPlay #2246

Closed

Route overview excludes destination pin #2247

Open

Updated changelog

eecd726

1ec5 mentioned this pull request Oct 2, 2019

Remove workarounds unsetting map content inset when setting map camera #2248

Closed

1ec5 approved these changes Oct 2, 2019

View reviewed changes

1ec5 merged commit 7ff86d9 into master Oct 2, 2019

1ec5 deleted the astojilj-15233 branch October 2, 2019 16:26

1ec5 added this to the v0.38.0 milestone Oct 2, 2019

astojilj mentioned this pull request Oct 6, 2019

[ios] Fixes an issue that caused the ornaments ignore contentInset property. mapbox/mapbox-gl-native#15584

Merged

2 tasks

1ec5 mentioned this pull request Feb 4, 2020

UserCourseView puck is off #2287

Open

1ec5 mentioned this pull request Jun 5, 2020

Puck Location fix #2387

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify anchor point calculation and puck centering #2211

Simplify anchor point calculation and puck centering #2211

astojilj commented Aug 12, 2019 •

edited

Loading

1ec5 Aug 14, 2019

astojilj Sep 9, 2019

1ec5 Oct 2, 2019

1ec5 Aug 14, 2019

astojilj Aug 14, 2019

1ec5 commented Aug 14, 2019

noway commented Sep 26, 2019 •

edited

Loading

astojilj commented Sep 26, 2019 •

edited

Loading

noway commented Sep 26, 2019 •

edited

Loading

1ec5 commented Sep 27, 2019

1ec5 left a comment

Simplify anchor point calculation and puck centering #2211

Simplify anchor point calculation and puck centering #2211

Conversation

astojilj commented Aug 12, 2019 • edited Loading

1ec5 Aug 14, 2019

Choose a reason for hiding this comment

astojilj Sep 9, 2019

Choose a reason for hiding this comment

1ec5 Oct 2, 2019

Choose a reason for hiding this comment

1ec5 Aug 14, 2019

Choose a reason for hiding this comment

astojilj Aug 14, 2019

Choose a reason for hiding this comment

1ec5 commented Aug 14, 2019

noway commented Sep 26, 2019 • edited Loading

astojilj commented Sep 26, 2019 • edited Loading

noway commented Sep 26, 2019 • edited Loading

1ec5 commented Sep 27, 2019

1ec5 left a comment

Choose a reason for hiding this comment

astojilj commented Aug 12, 2019 •

edited

Loading

noway commented Sep 26, 2019 •

edited

Loading

astojilj commented Sep 26, 2019 •

edited

Loading

noway commented Sep 26, 2019 •

edited

Loading