Compare routes using step names

[kmadsen]

Description

Resolves #3144

Midway through a route we request a new route, and we're looking to see if it's faster. This is working as design. The problem is the route we're checking, can be the current route.

This is the 3rd approach considered for this.

• compare geometry strings Check if faster route is new route #3262 ❌
• compare leg summaries ❌
• This change is to compare remaining LegStep.names 🥗

Going to test this in a few regions

• I have added any issue links
• I have added all related labels (bug, feature, new API(s), SEMVER, etc.)
• I have added the appropriate milestone and project boards

Screenshots

Testing

Please describe the manual tests that you ran to verify your changes

• I have tested locally (including SNAPSHOT upstream dependencies if needed) through testapp/demo app and run all activities to avoid regressions
• I have tested via a test drive, or a simulation/mock location app
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

Checklist

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have updated the CHANGELOG including this PR
• We might need to update / push api/current.txt files after running $> make 1.0-core-update-api (Core) / $> make 1.0-ui-update-api (UI) if there are changes / errors we're 🆗 with (e.g. AddedMethod changes are marked as errors but don't break SemVer) 🚀 If there are SemVer breaking changes add the SEMVER label. See Metalava docs

[Guardiola31337]

How is @mapbox/navigation-ios approach to tackle this issue? Do you know @1ec5?

[kmadsen]

These test values are covered in FasterRouteDetectorTest

[codecov-commenter]

Codecov Report

Merging #3301 into master will increase coverage by 0.00%.
The diff coverage is 39.28%.

@@            Coverage Diff            @@
##             master    #3301   +/-   ##
=========================================
  Coverage     39.30%   39.31%           
- Complexity     2312     2314    +2     
=========================================
  Files           547      548    +1     
  Lines         20019    20043   +24     
  Branches       1901     1913   +12     
=========================================
+ Hits           7869     7880   +11     
- Misses        11228    11229    +1     
- Partials        922      934   +12     

[kmadsen]

Follow up issue from this, there is a race condition.

In the event of a faster route, the current step index can change while requesting. This will cause the requested route to include an extra step.

For example,
t0 = current route includes steps "A", "B", "C", "D"
t1 = request faster route
t2 = current route becomes "B", "C", "D"
t3 = faster route response is "A", "B", "C", "D"

The route will be considered different

[Guardiola31337]

@kmadsen

Resolves #3116

As mentioned in #3262 (review)

Which BTW was used for the Off-route scenario as re-routes are handled by us by default i.e. we try to respect the original route if possible so we might need it either way.

we used Damerau Levenshtein algorithm in the legacy for off-routes and this PR is addressing Faster route only. Should we add same Compare routes using step names logic suggested for Off-route to try to stick to the original route when getting re-routed? Maybe we can follow up in a separate PR, but was wondering if we've considered it.

[1ec5]

This approach seems OK to me: #3116 (comment).

How is @mapbox/navigation-ios approach to tackle this issue?
Should we add same Compare routes using step names logic suggested for Off-route to try to stick to the original route when getting re-routed? Maybe we can follow up in a separate PR, but was wondering if we've considered it.

The iOS navigation SDK ranks the routes by edit distance over the summary string, the same approach @kmadsen investigated in #3116. The comparison happens in a bottlenecked part of the code that handles rerouting due to both going off-route and finding a faster route.

I think either approach is sufficient for this use case, because a sufficiently different route will rank lower by either measure (leg summary or step names). It’s possible that the step name comparison will be slightly more reliable in cases where the alternative routes differ in a way that isn’t significant enough to result in a difference to the summary, but I’d assume that getting such cases right would be a marginal improvement. (The summary consists of the two most common street names by length along the leg.) If you have a concrete example of where it yields a practical improvement, I’d love to consider it.

Note that the iOS navigation SDK only uses the edit distance algorithm to choose from among multiple alternative routes. It doesn’t use the algorithm to decide whether to replace the preexisting route. That determination is based on a much simpler heuristic: whether the new route is at least 10% faster and the user isn’t near the destination.

[kmadsen]

That determination is based on a much simpler heuristic: whether the new route is at least 10% faster and the user isn’t near the destination.

@1ec5 this is what is being addressed in this pull request. it turns out, the "new route that is 10% faster" is not new. It's more often than not, the same route

Note that I agree with everything else though, maybe comparing steps is better than route summaries when the origin and destination are the same. The issue with faster route, is that it has been partially completed and the route summary is no longer a working description for faster route. Also agreeing with performance considerations, there may 10-1000 route steps to compare. This is worth testing

[kmadsen]

In the question for performance. Tested creating a route from SF to NYC and this was the result:

faster_route_debug time to compare 46 steps 0.053s 

considering this operation is run once every 5 minutes, there's little concern here. but it should be executed on a background thread.

[kmadsen]

The answer is in, runBlocking is better than runBlockingTest

Kotlin/kotlinx.coroutines#1222 (comment)

[Guardiola31337]

A couple of minor comments.

[Guardiola31337]

Thanks for addressing the feedback @kmadsen 🚢