Reroute when the user is moving away from maneuver#646
Conversation
|
|
||
| var hasFoundOneQualifiedLocation = false | ||
|
|
||
| var previousDistancesFromManeuver: [CLLocationDistance] = [] |
There was a problem hiding this comment.
recentDistancesFromManeuver makes it more clear that this isn’t a full history of distances.
| } | ||
|
|
||
| func resetPreviousDistanceArray() { | ||
| previousDistancesFromManeuver.removeAll() |
There was a problem hiding this comment.
Is a separate method really needed for this statement? It looks like we’re calling it every time we touch routeProgress.currentLegProgress.stepIndex – maybe what this class needs is a property that mirrors routeProgress.currentLegProgress.stepIndex:
var currentStepIndex: Int {
get {
return routeProgress.currentLegProgress.stepIndex
}
set {
routeProgress.currentLegProgress.stepIndex = newValue
recentDistancesFromManeuver.removeAll()
}
}There was a problem hiding this comment.
As much as I like this style, I don't like hiding routeProgress.currentLegProgress.stepIndex behind another var. I like explicitly changing values on routeProgress.
There was a problem hiding this comment.
Two alternatives:
- Use KVO to observe for changes to the key path
routeProgress.currentLegProgressand reset the array. - Listen for alert level change notifications and reset the array.
There was a problem hiding this comment.
| let userDistanceToManeuver = distance(along: coordinates, from: location.coordinate) | ||
|
|
||
| guard previousDistancesFromManeuver.count <= 3 else { | ||
| resetPreviousDistanceArray() |
There was a problem hiding this comment.
We should make it so that rerouting automatically calls this method, making this line redundant.
|
|
||
| if previousDistancesFromManeuver.isEmpty { | ||
| previousDistancesFromManeuver.append(userDistanceToManeuver) | ||
| } else if let lastSpeed = previousDistancesFromManeuver.last, userDistanceToManeuver > lastSpeed { |
There was a problem hiding this comment.
Are we comparing distances or speeds?
| if previousDistancesFromManeuver.isEmpty { | ||
| previousDistancesFromManeuver.append(userDistanceToManeuver) | ||
| } else if let lastSpeed = previousDistancesFromManeuver.last, userDistanceToManeuver > lastSpeed { | ||
| previousDistancesFromManeuver.append(userDistanceToManeuver) |
There was a problem hiding this comment.
RouteStepProgress.userDistanceToManeuverLocation already tracks the most recent distance, so all this class needs is to keep a counter of increasing distances: countOfRecentBackwardsUpdates or something along those lines.
|
@1ec5 updated. |
|
Note, this does not solve the case when the user is at the beginning of the route and moving away since distance along has a max value (length of the step). However, if the user is moving away from the departure maneuver, they should be rerouted quickly. |
|
|
||
| guard recentDistancesFromManeuver.count <= 3 else { | ||
| resetPreviousDistanceArray() | ||
| return false |
|
|
||
| if recentDistancesFromManeuver.isEmpty { | ||
| recentDistancesFromManeuver.append(userDistanceToManeuver) | ||
| } else if let lastSpeed = recentDistancesFromManeuver.last, userDistanceToManeuver > lastSpeed { |
| } | ||
|
|
||
| func resetPreviousDistanceArray() { | ||
| recentDistancesFromManeuver.removeAll() |
|
@1ec5 👍 fixed |
| if let lastReroute = outstandingFeedbackEvents.map({$0 as? RerouteEvent }).last { | ||
| lastReroute?.update(newRoute: routeProgress.route) | ||
| } | ||
| recentDistancesFromManeuver.removeAll() |
There was a problem hiding this comment.
Does it make a difference if this is in willReroute(notification:) or didReroute(notification:)? Wondering if there’s a possibility of a race condition if a location even more distant from the maneuver comes in while waiting for a response from the API.
2 participants
If the user is moving away from the maneuver (whips a uturn on a country road half way down a step), we should reroute them once we know they are really moving away.
The gist:
/cc @1ec5 @frederoni @ericrwolfe