mapbox / mapbox-navigation-ios

Turn-by-turn navigation logic and UI in Swift on iOS
https://docs.mapbox.com/ios/navigation/
Other
860 stars 310 forks source link

Invalid course used to orient camera and user puck #2046

Open 1ec5 opened 5 years ago

1ec5 commented 5 years ago

We aren’t checking for CLLocation.course being −1 in several places:

https://github.com/mapbox/mapbox-navigation-ios/blob/abde5f63e58b686c3849e4cddc0d06dee385592b/MapboxNavigation/CarPlayNavigationViewController.swift#L252 https://github.com/mapbox/mapbox-navigation-ios/blob/abde5f63e58b686c3849e4cddc0d06dee385592b/MapboxNavigation/NavigationMapView.swift#L336 https://github.com/mapbox/mapbox-navigation-ios/blob/abde5f63e58b686c3849e4cddc0d06dee385592b/MapboxNavigation/UserCourseView.swift#L26 https://github.com/mapbox/mapbox-navigation-ios/blob/abde5f63e58b686c3849e4cddc0d06dee385592b/MapboxNavigation/UserCourseView.swift#L49

MGLMapView treats a rotation of −1° as meaning “keep the current rotation”, which may not always be accurate. More worryingly, NavigationMapView.updateCourseTracking(location:camera:animated:) updates not only the camera but also the user puck’s transformation. A direction of −1° would actually rotate the puck close to due north, possibly resulting in some spinning when the speed is too low for Core Location to report a course.

We can use CLLocationDirection.isQualified to easily perform this check. In the event of a negative value, we could calculate the direction to the next coordinate along the route line. Or better yet, we could keep a running comparison between CLHeading.trueHeading and CLLocation.course so that we can fall back on the heading (with an offset) when the course becomes less reliable than the heading.

/cc @mapbox/navigation-ios @d-prukop @kevinkreiser

d-prukop commented 5 years ago

@kevinkreiser I keep thinking that it makes sense for MapboxNavigationNative to handle course updates. Right now there is a way to configure the minimum speed that location projection starts working. Could we do the same thing for course using a separate configurable value? And could course below this speed threshold just return the value of the last course above the threshold?

kevinkreiser commented 5 years ago

@d-prukop you mean in the event that you arent following a route? Yes we can do exactly that. Essentially we'll keep the last known valid (as in not nil) course that was provided and that will be echoed back out in the NavigationStatus unless you are tracking along the route in which case we will be returning the course at the snapped location of the route. Does that sound ok?

d-prukop commented 5 years ago

@kevinkreiser That sounds great to me. Thank you

1ec5 commented 5 years ago

And could course below this speed threshold just return the value of the last course above the threshold?

As far as the camera is concerned, that would have no effect on current behavior, because MGLMapView considers an MGLMapCamera.heading of −1 to mean “keep the current heading”. However, it could address any issue (which I haven’t observed) where the puck spins around needlessly.

Merely persisting the previous correct course could still result in a perceived lag around curves and turns. The original post suggests two possible solutions:

kevinkreiser commented 5 years ago

Merely persisting the previous correct course could still result in a perceived lag around curves and turns.

Please note that if you are tracking along a route, we set the course to that of the route's geometry. The only time we dont do that is if you are in situation where you are in a parkinglot that has no geometry and are making your way to the route or if you go offroute.

1ec5 commented 5 years ago

MapboxNavigationNative v6.0.0 will eliminate most cases where MBFixLocation.bearing is nil, so the bug described here will become mostly latent except for the situation where a navigation session has just begun at rest and the map view is being reused (such as in CarPlay). In that case, the puck might point towards due north(ish) while the camera remains pointed in the direction the phone is facing. We can mitigate that issue by checking for −1 and falling back on CLHeading.trueHeading.

akitchen commented 5 years ago

The goal here is for us to find the remaining places in the code where we are incorrectly orienting the camera to near north - we need to figure out what to fill in for the -1's

1ec5 commented 1 year ago

Some of the code in the original post has been rewritten or replaced, but the issue remains in the replacements:

https://github.com/mapbox/mapbox-navigation-ios/blob/0a93bb726f9e4bf15b71a5f81b8229ae77ae1bc2/Sources/MapboxNavigation/NavigationViewportDataSource.swift#L310 https://github.com/mapbox/mapbox-navigation-ios/blob/0a93bb726f9e4bf15b71a5f81b8229ae77ae1bc2/Sources/MapboxNavigation/UserCourseView.swift#L21 https://github.com/mapbox/mapbox-navigation-ios/blob/0a93bb726f9e4bf15b71a5f81b8229ae77ae1bc2/Sources/MapboxNavigation/UserCourseView.swift#L117

As a result, the camera and puck can both swivel back and forth when Core Location momentarily skips a CLLocation.course reading as part of a location update, reporting −1 instead.