Add passive smart rerouting by bsudekum · Pull Request #346 · mapbox/mapbox-navigation-ios

bsudekum · 2017-07-07T19:22:19Z

This PR adds the ability for the route controller to check for routes with better ETAs while the user is navigating. There are some checks in place to make sure we do this only when necessary:

There much be an upcoming step.
There must be at least 70 seconds left on the current step to allow ample maneuvering for new route.
The current route must have at least 10 minutes left on it.
The new route must have an ETA that is at least 10% faster

If the new route has the same upcoming maneuver as the current route, the step is not announced since it already has been announced.

There currently is no UI around this. This is okay since the heuristics are extremely stiff; triggering a new route in it's current implementation will be extremely rare IMO.

/cc @willwhite @ericrwolfe @1ec5 @frederoni

1ec5 · 2017-07-09T02:40:15Z

    var lastReRouteLocation: CLLocation?

    var routeTask: URLSessionDataTask?
+    var lastLocationTimestamp: Date?


If it’s a Date, let’s call it a date instead of a timestamp.

1ec5 · 2017-07-09T02:45:37Z


+    func checkForFasterRoute(from location: CLLocation) {
+
+        // If the user is on a short route, don't check for faster alternatives


This comment is a bit misleading: we’re actually bailing if the user is less than 10 minutes from their destination, regardless of the length of the overall trip. Is the rationale that it’s less likely for there to be any viable alternatives towards the end of the trip? In that case, perhaps this heuristic should be based on distance rather than time.

I'm not certain I see why distance remaining would be more useful here. If there is not much distance remaining on a route, but the duration remaining is high, we should still be looking for a new route.

1ec5 · 2017-07-09T02:52:03Z

+        guard routeProgress.durationRemaining > 600 else { return }
+
+        // If the user is approaching a maneuver, don't check for a faster alternatives
+        guard routeProgress.currentLegProgress.currentStepProgress.durationRemaining > 70 else { return }


Since we’re rerouting automatically instead of prompting the user, we could wind up in the following scenario:

As soon as the user turns right, we say, “Continue on Main Street for 1 mile” (alertLevel = .low).

Nary a second later, we abruptly say, “Turn left onto Commercial Street” (alertLevel = .high), surprising the user.

This makes me wonder whether we should be rerouting based on the upcoming maneuver point, to avoid this scenario, until we’re able to give the user a choice. That would be subtle enough that we can remove the 70-second heuristic, as well as the complicated logic below that suppresses redundant announcements.

@1ec5 how could be force this? Make the first waypoint the upcoming maneuver?

Yes, exactly.

1ec5 · 2017-07-09T02:57:18Z

+        guard let currentUpcomingManeuver = routeProgress.currentLegProgress.upComingStep else { return }
+
+        guard let lastLocationTimestamp = lastLocationTimestamp else {
+            self.lastLocationTimestamp = location.timestamp


Nit: remove self., since the local variable is only available outside this guard statement.

1ec5 · 2017-07-09T02:57:56Z

+        options.waypoints = [Waypoint(coordinate: location.coordinate)] + routeProgress.remainingWaypoints
+        if let firstWaypoint = options.waypoints.first, location.course >= 0 {
+            firstWaypoint.heading = location.course
+            firstWaypoint.headingAccuracy = 90


There’s quite a bit of duplicated code between this method and reroute(from:). Let’s extract that shared code into a separate method instead of copy-pasting.

1ec5 · 2017-07-09T03:11:52Z

+
+            strongSelf.lastLocationTimestamp = nil
+
+            if let route = routes?.first {


A guard statement would simplify this closure a bit.

1ec5 · 2017-07-09T03:16:31Z

+            strongSelf.lastLocationTimestamp = nil
+
+            if let route = routes?.first {
+                let percentDifference = ((durationRemaining - route.expectedTravelTime) / route.expectedTravelTime) * 100


If the user is currently 10 minutes from their destination and the new route is 9 minutes long, that’s a 10% improvement, but percentDifference is 11.1 because it puts the new route’s time in the denominator instead of the old route’s time.

1ec5 · 2017-07-09T03:17:44Z

+                let percentDifference = ((durationRemaining - route.expectedTravelTime) / route.expectedTravelTime) * 100
+
+                // Only use new route if it's at least 10% faster
+                if percentDifference > 10 {


The comment says 10% faster is adequate, but this if statement disagrees.

1ec5 · 2017-07-09T03:19:18Z

+        }
+
+        // Only check ever 2 minutes for faster route
+        guard location.timestamp.timeIntervalSince1970 - lastLocationTimestamp.timeIntervalSince1970 > 120 else { return }


Use Date.timeIntervalSince(_:).

1ec5 · 2017-07-09T03:42:20Z

+
+        let options = routeProgress.route.routeOptions
+        options.waypoints = [Waypoint(coordinate: location.coordinate)] + routeProgress.remainingWaypoints
+        if let firstWaypoint = options.waypoints.first, location.course >= 0 {


firstWaypoint is guaranteed to be the one we created above using Waypoint(coordinate:). Pull it out into a local variable and reuse it below.

1ec5

Interested to see what the refactored reroute(from:) ends up looking like.

1ec5 · 2017-07-11T01:52:26Z

+
+        guard let currentUpcomingManeuver = routeProgress.currentLegProgress.upComingStep else { return }
+
+        guard let lastLocationTimestamp = lastLocationDate else {


Rename the local variable to lastLocationDate for consistency.

1ec5 · 2017-07-11T01:53:39Z

+        }
+
+        // Only check ever 2 minutes for faster route
+        guard location.timestamp.timeIntervalSince(lastLocationTimestamp) > 120 else { return }


The comment implies that 120 seconds is enough for a new route, but this line disagrees.

1ec5 · 2017-07-11T01:57:13Z

+            let percentDifference = ((durationRemaining - route.expectedTravelTime) / durationRemaining) * 100
+
+            // Only use new route if it's at least 10% faster
+            if percentDifference >= 10 {


This can be simplified algebraically:

if route.expectedTravelTime <= 0.9 * durationRemaining

This is also safer, on the off-chance that we get into this code when the user is already at the destination. With the current code, there would be a division by zero.

bsudekum · 2017-07-11T18:27:18Z

@1ec5 updated

1ec5 · 2017-07-11T20:14:07Z

-        guard let lastLocationTimestamp = lastLocationDate else {
-            lastLocationDate = location.timestamp
+        guard let lastLocationDate = lastLocationDate else {
+            self.lastLocationDate = location.timestamp


Huh, I wouldn’t’ve expected that self. would be required here. 🤷‍♂️

1ec5 · 2017-07-11T20:15:13Z

+        let durationRemaining = routeProgress.durationRemaining
+        let currentAlertLevel = routeProgress.currentLegProgress.alertUserLevel
+
+        getDirections(from: location) { [weak self] (route, error) in


We should reroute from the upcoming maneuver’s location, not from the user’s current location: #346 (comment).

1ec5 · 2017-07-12T03:50:48Z

+
+        // Check for faster route given users current location
+        //
+        // If the user does not have much time left on the route, don't check for faster alternatives


This double-negative combined with the guard statement below make the logic a bit confusing. Here’s a less confusing way to describe what’s going on:

Only check for faster alternatives if the user has plenty of time left on the route.

1ec5 · 2017-07-12T03:56:14Z

+            guard let route = route else { return }
+
+            if route.legs[0].steps[1].expectedTravelTime <= 70 {
+                let maneuverLocation = CLLocation(coordinate: currentUpcomingManeuver.maneuverLocation, altitude: 0, horizontalAccuracy: 1, verticalAccuracy: 1, course: currentUpcomingManeuver.finalHeading ?? currentUpcomingManeuver.initialHeading ?? location.course, speed: 10, timestamp: Date())


This is an extremely long line. The course argument should be broken out into a local variable.

1ec5 · 2017-07-12T04:04:08Z

+                    // Only use new route if it's at least 10% faster
+                    if route.expectedTravelTime <= 0.9 * durationRemaining {
+                        // If the upcoming maneuver in the new route is the same as the current upcoming maneuver, don't announce it
+                        strongSelf.routeProgress = RouteProgress(route: route, legIndex: 0, alertLevel: currentUpcomingManeuver.description == route.legs[0].steps[1].description ? currentAlertLevel : .none)


RouteStep.description is really just an alias for RouteStep.instructions, which isn’t guaranteed to be unique among RouteSteps. For example, what if currentUpcomingManeuver is a ramp step and the new route starts with a different ramp step? In both cases, instruction might be:

Take the ramp on the right

but the new step could be much closer. (instruction doesn’t include the “In distance” prefix.)

For the purposes of this feature, two steps are equivalent if they’re located in the same place and have the same maneuver type and direction. mapbox/mapbox-directions-swift#151 would implement a custom == operator for RouteStep, but there’s nothing stopping us from implementing the same logic here for now.

1ec5 · 2017-07-12T04:05:54Z

+            if route.legs[0].steps[1].expectedTravelTime <= 70 {
+                let maneuverLocation = CLLocation(coordinate: currentUpcomingManeuver.maneuverLocation, altitude: 0, horizontalAccuracy: 1, verticalAccuracy: 1, course: currentUpcomingManeuver.finalHeading ?? currentUpcomingManeuver.initialHeading ?? location.course, speed: 10, timestamp: Date())
+
+                strongSelf.getDirections(from: maneuverLocation, completion: { (route, error) in


Nit: use trailing closure syntax.

1ec5 · 2017-07-12T04:08:25Z

+                })
+            } else {
+                // Only use new route if it's at least 10% faster
+                if route.expectedTravelTime <= 0.9 * durationRemaining {


There’s a fair amount of duplication within this method. This if statement represents the case where the new route leaves enough time to maneuver. Since it appears twice in the same method, let’s factor it out into a local closure, then call it in both places.

ericrwolfe · 2017-07-12T16:55:37Z

+        }
+
+        // Only check ever 2 minutes for faster route
+        guard location.timestamp.timeIntervalSince(lastLocationDate) >= 120 else { return }


Let's change this to once every 5 minutes. Eventually, we could adapt this for longer segments (e.g. >50 mile highway driving) and space this out to once every 10-15 minutes.

bsudekum · 2017-07-13T18:53:53Z

Ready for another round of reviews since tests are passing now.

1ec5 · 2017-07-13T21:12:05Z

@@ -351,10 +351,19 @@ extension RouteController: CLLocationManagerDelegate {
        }



newRoute is just an unwrapped version of route, so it’s fine to shadow the original variable and call the new local variable route as well.

1ec5 · 2017-07-13T21:14:48Z

A “compare” function returns whether something is greater, equal, or less than another thing, whereas this method potentially reroutes the user. A more accurate name would be rerouteIfFaster(onto:).

1ec5 · 2017-07-13T21:17:31Z

We need to check whether the steps are equivalent, not just whether they occur at the same location. What if the current step turns left at this location, whereas the new route’s first step turns right at the same location? See mapbox/mapbox-directions-swift#151 for a rough sketch of what RouteStep equality would look like.

ericrwolfe · 2017-07-14T18:44:09Z

+        // Only check for faster alternatives if the user has plenty of time left on the route.
+        guard routeProgress.durationRemaining > 600 else { return }
+        // If the user is approaching a maneuver, don't check for a faster alternatives
+        guard routeProgress.currentLegProgress.currentStepProgress.durationRemaining > 70 else { return }


Can we use the existing low alert level constant here?

ericrwolfe · 2017-07-14T18:44:14Z

+            guard let firstLeg = route.legs.first, firstLeg.steps.count > 2 else { return }
+            strongSelf.lastLocationDate = nil
+
+            if firstLeg.steps[1].expectedTravelTime <= 70 {


Can we use the existing low alert level constant here?

ericrwolfe · 2017-07-14T18:46:59Z

+                let course = currentUpcomingManeuver.finalHeading ?? currentUpcomingManeuver.initialHeading ?? location.course
+                let maneuverLocation = CLLocation(coordinate: currentUpcomingManeuver.maneuverLocation, altitude: 1, horizontalAccuracy: 1, verticalAccuracy: 1, course: course, speed: 1, timestamp: Date())
+
+                strongSelf.getDirections(from: maneuverLocation) { (route, error) in


If you're not yet at the maneuver location, wouldn't this trigger another reroute?

That’s entirely up to the logic that follows for determining whether the new route is distinct from the existing route. Currently it doesn’t even check whether the two routes are distinct; it just goes with the new one as long as it’s faster. Conceptually, we should only use the new route if doing so would keep the contents of the turn banner the same for now.

Moreover, in terms of the alert level, it compares the currentUpcomingManeuver’s coordinates with those of firstLeg.steps[1], which I think is incorrect. Consider the following scenario:

currentManeuver has a few miles left.

currentUpcomingManeuver is a right turn at Albuquerque.

firstLeg.steps[1] is a departure at the user’s current location.

firstLeg.steps[2] is a right turn at Albuquerque.

As I pointed out in #346 (comment), we should be comparing the maneuver type, direction, and coordinates of currentUpcomingManeuver and firstLeg.steps[2]. Otherwise, we are indeed guaranteed a reroute.

willwhite · 2017-07-25T01:12:44Z

The behavior here could be a little funny/unexpected unless we also update the current route - for example using map matching. On longer trips, overall traffic conditions across all possible routes can significantly improve across the board and you'd be doing your 10% comparison against a stale version of your current route, putting it at a big disadvantage even thought it might still be a reasonable way to go.

The opposite can also be true - traffic conditions for all possible routes can get significantly worse causing us to never pass the 10% threshold. In this situation we'd never update the ETA or congestion information on the current route.

Updating the ETA (and congestion information) on the existing route as you drive feels like the higher priority.

ericrwolfe · 2017-07-25T20:09:50Z

Updating the ETA (and congestion information) on the existing route as you drive feels like the higher priority.

It high priority, but also more complicated to build right now given we'll need to build out support for the map-matching API in MapboxDirections.swift.

On longer trips, overall traffic conditions across all possible routes can significantly improve across the board and you'd be doing your 10% comparison against a stale version of your current route, putting it at a big disadvantage even thought it might still be a reasonable way to go.

In this case, the new route will exceed the 10% threshold and the user will continue on the fastest route, now with an accurate ETA.

traffic conditions for all possible routes can get significantly worse causing us to never pass the 10% threshold. In this situation we'd never update the ETA or congestion information on the current route.

In this case, nothing changes from what we have now.

We should ship this before ETAs since it's a quick win that will help overall, and in the 2 cases @willwhite provided we're no worse off than where we're at now.

1ec5 · 2017-07-25T22:29:19Z

more complicated to build right now given we'll need to build out support for the map-matching API in MapboxDirections.swift

After sketching out a design mapbox/mapbox-directions-swift#158, this task doesn’t seem all that daunting. But it would be higher-hanging fruit than what we’ve got here.

Bobby Sudekum added 4 commits July 7, 2017 11:35

Add smart reroute

0ac33f6

add keys

1a8db02

pass through alert level

872a9c9

remove

9078ad4

bsudekum requested a review from 1ec5 July 7, 2017 19:31

1ec5 reviewed Jul 9, 2017

View reviewed changes

update

488c2d4

1ec5 reviewed Jul 11, 2017

View reviewed changes

Bobby Sudekum added 2 commits July 11, 2017 11:20

factor out getdirections

f5e8c69

revert

9f21cc5

1ec5 approved these changes Jul 11, 2017

View reviewed changes

1ec5 suggested changes Jul 11, 2017

View reviewed changes

Dont use short first step route

cfb10ac

1ec5 reviewed Jul 12, 2017

View reviewed changes

ericrwolfe reviewed Jul 12, 2017

View reviewed changes

Bobby Sudekum added 2 commits July 12, 2017 13:31

Merge branch 'master' into smartreroute

39d8b35

refactor

e7a5ef4

1ec5 mentioned this pull request Jul 13, 2017

Prevent abrupt re-routing / immediate u-turns #221

Closed

Bobby Sudekum added 4 commits July 13, 2017 10:28

remove dupe

be69700

add back

2ab912e

fix

f6a4a7e

add back

8de2d9e

ericrwolfe mentioned this pull request Jul 13, 2017

Background rerouting based on current traffic mapbox/mapbox-navigation-android#129

Closed

1ec5 reviewed Jul 13, 2017

View reviewed changes

Bobby Sudekum added 2 commits July 13, 2017 15:23

Updates

f8f5c2a

Add doc

43403ae

ericrwolfe suggested changes Jul 14, 2017

View reviewed changes

Merge branch 'master' into smartreroute

5eb82ef

Bobby Sudekum added 4 commits July 17, 2017 11:23

const

1368834

remove request from maneuver

8310bbf

Merge branch 'master' into smartreroute

619578c

rm token

3059069

ericrwolfe added this to the v0.6.0-1 milestone Jul 19, 2017

ericrwolfe added feature New feature request. topic: traffic labels Jul 19, 2017

ericrwolfe modified the milestones: v0.6.0-2, v0.6.0-1 Jul 21, 2017

Bobby Sudekum added 2 commits July 24, 2017 16:42

Merge branch 'master' into smartreroute

4b21788

add to extension folder

f44d222

ericrwolfe added the ✓ ready for review label Jul 25, 2017

Add option to toggle background fetching on and off

e627191

ericrwolfe approved these changes Jul 26, 2017

View reviewed changes

bsudekum merged commit 2e2ef48 into master Jul 26, 2017

bsudekum deleted the smartreroute branch July 26, 2017 01:08

ericrwolfe mentioned this pull request Aug 23, 2017

Indicate when background rerouting occurs #530

Closed

1ec5 mentioned this pull request Sep 9, 2021

Fix inconsistent route progress and index after rerouting #3345

Merged


		func checkForFasterRoute(from location: CLLocation) {

		// If the user is on a short route, don't check for faster alternatives


		strongSelf.lastLocationTimestamp = nil

		if let route = routes?.first {


		guard let currentUpcomingManeuver = routeProgress.currentLegProgress.upComingStep else { return }

		guard let lastLocationTimestamp = lastLocationDate else {

		@@ -351,10 +351,19 @@ extension RouteController: CLLocationManagerDelegate {
		}

Conversation

bsudekum commented Jul 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsudekum commented Jul 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsudekum commented Jul 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willwhite commented Jul 25, 2017

Uh oh!

ericrwolfe commented Jul 25, 2017

Uh oh!

1ec5 Jul 11, 2017 •

edited

Loading

1ec5 Jul 12, 2017 •

edited

Loading