-
Notifications
You must be signed in to change notification settings - Fork 314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't continual reroute user if standing still, allow user time to get to route start #84
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -9,6 +9,10 @@ import MapboxDirections | |||
@objc(MBRouteController) | |||
open class RouteController: NSObject { | |||
|
|||
var lastUserDistanceToStartOfRoute = Double.infinity | |||
|
|||
var userIsMovingAwayFromStartCounter: Int = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of counting the number of location updates, let’s consider the amount of time the user spends going away from the route.
monitorStepProgress(location) | ||
} | ||
|
||
func resetStartCounter() { | ||
userIsMovingAwayFromStartCounter = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: stray semicolon.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is equivalent to step.maneuverLocation
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, I guess the first steps maneuver location is the beginning of the route
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -110,9 +107,46 @@ extension RouteController: CLLocationManagerDelegate { | |||
} | |||
} | |||
|
|||
if routeProgress.currentLegProgress.stepIndex == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is equivalent to step.maneuverType == .depart
.
let stepLocation = CLLocation(latitude: step.coordinates!.first!.latitude, longitude: step.coordinates!.first!.longitude) | ||
|
||
var currentUserToStartOfRoute = location.distance(from: stepLocation) | ||
if currentUserToStartOfRoute > lastUserDistanceToStartOfRoute { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:)
).
@@ -110,9 +107,48 @@ extension RouteController: CLLocationManagerDelegate { | |||
} | |||
} | |||
|
|||
if routeProgress.currentLegProgress.currentStepProgress.step.maneuverType == .depart && !userIsOnRoute(location) { | |||
let step = routeProgress.currentLegProgress.currentStepProgress.step |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this line up so you don't have to repeat yourself.
@1ec5 I think this is a good start. We should look at using course in a separate PR. |
MapboxNavigation/Constants.swift
Outdated
@@ -50,6 +50,8 @@ public var RouteControllerHighAlertInterval: TimeInterval = 15 | |||
*/ | |||
public var RouteControllerManeuverZoneRadius: CLLocationDistance = 40 | |||
|
|||
public var MaxSecondsSpentTravelingAwayFromStartOfRoute: TimeInterval = 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This public global should have documentation.
|
||
lastUserDistanceToStartOfRoute = userSnappedDistanceToClosestCoordinate | ||
|
||
if userSnappedDistanceToClosestCoordinate > lastUserDistanceToStartOfRoute { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
userSnappedDistanceToClosestCoordinate
is never greater than lastUserDistanceToStartOfRoute
, because the line right above sets lastUserDistanceToStartOfRoute
to be equal to userSnappedDistanceToClosestCoordinate
.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don’t consider the start of the route at all anymore, do we? #84 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. Maximum time spent moving away from first step
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? |
This addresses two issues:
/cc @frederoni @1ec5