mapbox / mapbox-navigation-ios

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

Avoid inconsistencies when a route doubles back on itself #1733

Open 1ec5 opened 6 years ago

1ec5 commented 6 years ago

When a route doubles back on itself and the user visits the same coordinate a second time, the SDK gets confused, momentarily thinking the user is visiting that coordinate for the first time. There are several ways to reproduce this issue, for example:

Symptoms of this issue are:

arrow

The problem is that we’re passing a coordinate into Polyline.trimmed(from:distance:), which assumes the coordinate can only appear once in the polyline:

https://github.com/mapbox/mapbox-navigation-ios/blob/f7ba632b731e7d8f8ccbad77df3ea07510bab395/MapboxNavigation/NavigationMapView.swift#L592-L593 https://github.com/mapbox/mapbox-navigation-ios/blob/f7ba632b731e7d8f8ccbad77df3ea07510bab395/MapboxCoreNavigation/SimulatedLocationManager.swift#L157

The most robust fix is to rely on indices into the coordinate array rather than the coordinates themselves. This approach will likely be necessary as part of the navigation-native refactoring. But that could make it much more difficult to memoize the remaining portion of the route, as in #1728.

An alternative approach, which accommodates #1728, is to form polylines from the two nearest steps’ coordinates properties rather than Route.coordinates. In the event that there are more than two maneuvers in close proximity, we’ll have to expand outward from those two steps. It’ll look pretty similar to RouteLegProgress.nearbyCoordinates, but that computed property is too aggressive because it unconditionally adds both the previous and next steps’ coordinates to the result.

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

1ec5 commented 6 years ago

1735 fixes the maneuver arrow, but not the similar simulated location issue pointed out above. SimulatedLocationManager makes assumptions about the uniqueness of coordinates in multiple places, and untangling those assumptions will be less straightforward. mapbox/turf-swift#67 adds a method that may help with that.

1ec5 commented 5 years ago

1735 fixed enough of this issue that the rest can land in a future release as we find reproducible cases for the remaining theoretical concerns. Removing the milestone.