-
Notifications
You must be signed in to change notification settings - Fork 326
Multi-leg nearby coordinate lookups. #1883
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
Conversation
…/behind for first-step/last-step edge-cases on multi-leg routes.
updating with PR link
| } | ||
|
|
||
| let nearByCoordinates = routeProgress.currentLegProgress.nearbyCoordinates | ||
| let nearByCoordinates = routeProgress.nearbyCoordinates |
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.
Does it make sense to keep the currentLegProgress.nearbyCoordinates in addition to the new routeProgress.nearbyCoordinates? should we be deprecating the older one?
also, the B in nearby should be a small b, but that's ultra-minor
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've no real feeling on the subject either way, but @1ec5 did argue for keeping it the other day, and his rationale (which I cannot remember OOTOMH) made sense to me.
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 had assumed that nearbyCoordinates would become yet another aggregate property along the lines of remainingDistance. But nearbyCoordinates can’t be aggregated; it’s actually more of a view into the overall route geometry, along the lines of Route.polylineAroundManeuver(legIndex:stepIndex:distance:).
Put another way, when we want the route’s remainingDistance, we can add in the remaining legs wholesale; we don’t need step-level granularity for other legs. But with nearbyCoordinates, we’re always working at a step level, but the method now goes on RouteProgress because we’re trying to erase the boundaries between legs.
The current implementation of this PR still relies on RouteLegProgress.nearbyCoordinates, but my suggestion in #1883 (review) shows that we can achieve a more elegant implementation without it. Therefore, I’m 👍 on deprecating RouteLegProgress.nearbyCoordinates.
AKA: Renaming `upComingStep` to `upcomingStep`.
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.
The progress classes were designed from the beginning around a divide-and-conquer strategy, with each computed property functioning as an aggregator. But increasingly we’re asking RouteStepProgress for information that erases the boundaries between legs, which calls into question this strategy, given that we already retain the entire route geometry in memory. A different approach would’ve been to map each of the steps to its starting coordinate index when getting a new route. Then this sort of “nearby” query would be just a single index into the master route geometry and working forwards and backwards from there.
All that can wait. What we have here is functional, though I’ve left comments for making it more self-documenting and easier to piggyback on for additional features.
CHANGELOG.md
Outdated
|
|
||
| ### Other changes | ||
|
|
||
| * This release includes a possible fix for spurious rerouting when passing an intermediate waypoint. ([#1869](https://github.com/mapbox/mapbox-navigation-ios/pull/1869)) |
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 still fixed, but via this PR rather than #1869.
Fixes #1879. Allows for look-behind when you're on the first step of a leg, and look-ahead when you're on the last step.
/cc @mapbox/navigation-ios