mapbox-navigation-ios icon indicating copy to clipboard operation
mapbox-navigation-ios copied to clipboard

Invalid course used to orient camera and user puck

Open 1ec5 opened this issue 5 years ago • 8 comments

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

1ec5 avatar Mar 20 '19 20:03 1ec5

@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?

d-prukop avatar Mar 20 '19 20:03 d-prukop

@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?

kevinkreiser avatar Mar 20 '19 23:03 kevinkreiser

@kevinkreiser That sounds great to me. Thank you

d-prukop avatar Mar 20 '19 23:03 d-prukop

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:

  • Calculate the direction to the next coordinate along the route line (which we could easily do in the navigation SDK using CLLocationCoordinate2D.direction(to:)).
  • Keep track of the delta between heading and course, then fall back on an offset heading.

1ec5 avatar Mar 21 '19 06:03 1ec5

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.

kevinkreiser avatar Mar 21 '19 12:03 kevinkreiser

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.

1ec5 avatar Mar 22 '19 17:03 1ec5

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

akitchen avatar Mar 29 '19 16:03 akitchen

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.

1ec5 avatar Nov 14 '22 19:11 1ec5