Don't continual reroute user if standing still, allow user time to get to route start #84

bsudekum · 2017-03-22T23:14:08Z

This addresses two issues:

A user is standing still far from the route. Consider if someone starts a route in a large parking lot. Yes, they are from the route, but we don't need to reroute them.
The user is moving to/from the start. Again consider a large parking lot. If the user is traveling towards the beginning of the route, we do not need to reroute them even if they are greater than the max reroute distance.

…t to route start

1ec5

This is a solid start. I think we’re going to uncover a lot of hidden cases as we iterate on this design. It may help to focus the conversation on some common scenarios / lot layouts. I’ve included one below, but feel free to draw out some other scenarios you consider important.

1ec5 · 2017-03-22T23:21:16Z

MapboxNavigation/RouteController.swift

@@ -9,6 +9,10 @@ import MapboxDirections
 @objc(MBRouteController)
 open class RouteController: NSObject {

+    var lastUserDistanceToStartOfRoute = Double.infinity
+
+    var userIsMovingAwayFromStartCounter: Int = 0


Instead of counting the number of location updates, let’s consider the amount of time the user spends going away from the route.

1ec5 · 2017-03-22T23:21:56Z

MapboxNavigation/RouteController.swift

        monitorStepProgress(location)
    }

+    func resetStartCounter() {
+        userIsMovingAwayFromStartCounter = 0;


Nit: stray semicolon.

1ec5 · 2017-03-22T23:39:33Z

MapboxNavigation/RouteController.swift

@@ -110,9 +107,46 @@ extension RouteController: CLLocationManagerDelegate {
            }
        }

+        if routeProgress.currentLegProgress.stepIndex == 0 {
+            let step = routeProgress.currentLegProgress.currentStepProgress.step
+            let stepLocation = CLLocation(latitude: step.coordinates!.first!.latitude, longitude: step.coordinates!.first!.longitude)


This is equivalent to step.maneuverLocation.

Oh yeah, I guess the first steps maneuver location is the beginning of the route

1ec5 · 2017-03-22T23:48:04Z

MapboxNavigation/RouteController.swift

+            let step = routeProgress.currentLegProgress.currentStepProgress.step
+            let stepLocation = CLLocation(latitude: step.coordinates!.first!.latitude, longitude: step.coordinates!.first!.longitude)
+
+            var currentUserToStartOfRoute = location.distance(from: stepLocation)


I think we’re more interested in the distance from the user to the nearest point on the route, not necessarily the start of the route. Consider the possibility that the user heads toward the maneuver at the end of the first step:

Here, the Directions API has snapped the user’s location to the nearest point on the road network (disregarding parking aisles), even though the user will first touch the road network further to the west. In this scenario, the user moves away from the start of the route and never returns to it.

To address this issue, get the distance between the actual user location and the snapped user location.

1ec5 · 2017-03-22T23:49:43Z

MapboxNavigation/RouteController.swift

@@ -110,9 +107,46 @@ extension RouteController: CLLocationManagerDelegate {
            }
        }

+        if routeProgress.currentLegProgress.stepIndex == 0 {


This is equivalent to step.maneuverType == .depart.

1ec5 · 2017-03-23T00:11:55Z

MapboxNavigation/RouteController.swift

+            let stepLocation = CLLocation(latitude: step.coordinates!.first!.latitude, longitude: step.coordinates!.first!.longitude)
+
+            var currentUserToStartOfRoute = location.distance(from: stepLocation)
+            if currentUserToStartOfRoute > lastUserDistanceToStartOfRoute {


This seems to calculate the user’s course by storing and comparing the last n distances. Could we achieve the same effect or better by looking at whether location.course points toward the route? You could do that by comparing location.course to the direction of the closest path to the route line (using CLLocationCoordinate2D.direction(to:)).

1ec5 · 2017-03-23T16:40:43Z

MapboxNavigation/RouteController.swift

@@ -110,9 +107,48 @@ extension RouteController: CLLocationManagerDelegate {
            }
        }

+        if routeProgress.currentLegProgress.currentStepProgress.step.maneuverType == .depart && !userIsOnRoute(location) {
+            let step = routeProgress.currentLegProgress.currentStepProgress.step


Move this line up so you don't have to repeat yourself.

bsudekum · 2017-03-23T21:00:04Z

@1ec5 I think this is a good start. We should look at using course in a separate PR.

1ec5 · 2017-03-23T21:44:04Z

MapboxNavigation/Constants.swift

@@ -50,6 +50,8 @@ public var RouteControllerHighAlertInterval: TimeInterval = 15
 */
 public var RouteControllerManeuverZoneRadius: CLLocationDistance = 40

+public var MaxSecondsSpentTravelingAwayFromStartOfRoute: TimeInterval = 3


This public global should have documentation.

1ec5 · 2017-03-23T21:45:58Z

MapboxNavigation/RouteController.swift

+
+            lastUserDistanceToStartOfRoute = userSnappedDistanceToClosestCoordinate
+
+            if userSnappedDistanceToClosestCoordinate > lastUserDistanceToStartOfRoute {


userSnappedDistanceToClosestCoordinate is never greater than lastUserDistanceToStartOfRoute, because the line right above sets lastUserDistanceToStartOfRoute to be equal to userSnappedDistanceToClosestCoordinate.

1ec5 · 2017-03-23T22:26:54Z

MapboxNavigation/Constants.swift

@@ -52,6 +52,12 @@ public var RouteControllerManeuverZoneRadius: CLLocationDistance = 40


 /*
+ Maximum number of seconds the user can travel away from the start of the route before rerouting occurs
+*/
+public var MaxSecondsSpentTravelingAwayFromStartOfRoute: TimeInterval = 3


We don’t consider the start of the route at all anymore, do we? #84 (comment)

True. Maximum time spent moving away from first step

frederoni · 2017-03-27T13:25:21Z

This issue was introduced by snap to route in #57 but also fixed in the same PR https://github.com/mapbox/MapboxNavigation.swift/pull/57/files#diff-36ca7eeae4537f27ed429d4f64eaa209R157. Does this leave the previous logic obsolete or will it work well together?

Don't continual reroute user if standing still, allow user time to ge…

a256664

…t to route start

1ec5 reviewed Mar 23, 2017

View reviewed changes

bsudekum added 2 commits March 22, 2017 18:02

Use snapped distance

1ff4c6d

fix up tests

fba6dd1

1ec5 reviewed Mar 23, 2017

View reviewed changes

bsudekum added 4 commits March 23, 2017 10:22

Use timer

716126e

fix test

8228957

simple

1e43ceb

move

7f0e438

1ec5 reviewed Mar 23, 2017

View reviewed changes

Fix

98685f4

bsudekum merged commit c1371f0 into master Mar 23, 2017

bsudekum deleted the start branch March 23, 2017 22:15

1ec5 reviewed Mar 23, 2017

View reviewed changes

bsudekum mentioned this pull request Oct 12, 2017

remove departure timer #703

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't continual reroute user if standing still, allow user time to get to route start #84

Don't continual reroute user if standing still, allow user time to get to route start #84

bsudekum commented Mar 22, 2017

1ec5 left a comment

1ec5 Mar 22, 2017

1ec5 Mar 22, 2017

1ec5 Mar 22, 2017

bsudekum Mar 23, 2017

1ec5 Mar 22, 2017

1ec5 Mar 22, 2017

1ec5 Mar 23, 2017

1ec5 Mar 23, 2017

bsudekum commented Mar 23, 2017 •

edited

Loading

1ec5 Mar 23, 2017

1ec5 Mar 23, 2017

1ec5 Mar 23, 2017

bsudekum Mar 23, 2017

frederoni commented Mar 27, 2017


		lastUserDistanceToStartOfRoute = userSnappedDistanceToClosestCoordinate

		if userSnappedDistanceToClosestCoordinate > lastUserDistanceToStartOfRoute {

Don't continual reroute user if standing still, allow user time to get to route start #84

Don't continual reroute user if standing still, allow user time to get to route start #84

Conversation

bsudekum commented Mar 22, 2017

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsudekum commented Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederoni commented Mar 27, 2017

bsudekum commented Mar 23, 2017 •

edited

Loading