Skip to content
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

Check if faster route is new route #3262

Closed
wants to merge 2 commits into from

Conversation

kmadsen
Copy link
Contributor

@kmadsen kmadsen commented Jun 30, 2020

Description

Resolves #3144
Resolves #3116

Looked at pulling over code from Legacy #808, but it won't work. The current status is, as you traverse a route, the route needs to be clipped. Legacy does not do this, and this current solution does not do this. Wonder how we should do this 🤔

Curious if showing alternatives will help us debug reroutes: #3184.

  • 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 or Gifs

output

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

@kmadsen kmadsen force-pushed the km-show-alternatives-in-history-activity branch 2 times, most recently from 6b41ed9 to 07bc00c Compare June 30, 2020 23:28
@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2020

Codecov Report

Merging #3262 into master will decrease coverage by 0.00%.
The diff coverage is 81.81%.

@@             Coverage Diff              @@
##             master    #3262      +/-   ##
============================================
- Coverage     38.65%   38.65%   -0.01%     
- Complexity     2261     2268       +7     
============================================
  Files           542      543       +1     
  Lines         19842    19849       +7     
  Branches       1886     1888       +2     
============================================
+ Hits           7670     7672       +2     
- Misses        11273    11275       +2     
- Partials        899      902       +3     

@kmadsen kmadsen changed the title Show alternatives in history replay Check if faster route is new route Jul 1, 2020
@kmadsen kmadsen force-pushed the km-show-alternatives-in-history-activity branch from 6af704e to b30188c Compare July 1, 2020 04:28
@@ -171,6 +172,13 @@ class ReplayHistoryActivity : AppCompatActivity() {
}
})

mapboxNavigation.attachFasterRouteObserver(object : FasterRouteObserver {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Showing all the routes helps debug faster route.

Also found that, if you reverse the route list - you'll get a faster route just about every time 🪂

val reversed = alternatives.reversed()
navigationContext?.navigationMapboxMap?.drawRoutes(reversed)
navigationContext?.mapboxNavigation?.setRoutes(reversed)

@kmadsen kmadsen force-pushed the km-show-alternatives-in-history-activity branch 5 times, most recently from 4384ff3 to 0026fe5 Compare July 7, 2020 00:04
@kmadsen kmadsen force-pushed the km-show-alternatives-in-history-activity branch from 0026fe5 to 060dd38 Compare July 7, 2020 00:46
Copy link
Contributor

@Guardiola31337 Guardiola31337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolves #3116

Looked at pulling over code from Legacy #808.
I haven't been able to find a test case where we need to use the O(n*m) Damerau Levenshtein Algorithm. This pull request sets up tests, and if we can find a case where we need a more sophisticated algorithm - we have one available.

All we're doing here is comparing the geometry strings, which is faster: O(max(n,m)) aka O(m+n).

Not so sure about this. As pointed out below geometry is quite sensitive and it's going to be different most of the times even if there's a small deviation. I think that was the main reason to rely on algorithms like Damerau Levenshtein. 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.

val weightedDuration = routeProgress.durationRemaining * PERCENTAGE_THRESHOLD
return newRouteDuration < weightedDuration
val isNewRouteFaster = alternativeDuration < weightedDuration
return isNewRouteFaster && routeComparator.isNewRoute(routeProgress, alternativeRoute)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe routeComparator.isNewRoute(routeProgress, alternativeRoute) is going to be true most of the times as alternativeRoute is going to have a different geometry most likely and we're always passing the alternativeRoute 👀 FasterRouteController

private val fasterRouteRequestCallback = object : RoutesRequestCallback {
    override fun onRoutesReady(routes: List<DirectionsRoute>) {
        val currentRoute = directionsSession.routes.firstOrNull()
            ?: return
        tripSession.getRouteProgress()?.let { progress ->
            val isAlternativeFaster = fasterRouteDetector.isRouteFaster(routes[0], progress)
            fasterRouteObserver?.onFasterRoute(currentRoute, routes, isAlternativeFaster)
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you're totally right. Pushed the breaking test case

Also turns out the legacy approach has the same issue. It can only compare routes with the same origin and destination. After completing many steps, the the summary of the route changes.

Here is legacy code that identifies a route

private String obtainRouteLegDescriptionFrom(DirectionsRoute route) {
List<RouteLeg> routeLegs = route.legs();
StringBuilder routeLegDescription = new StringBuilder();
for (RouteLeg leg : routeLegs) {
routeLegDescription.append(leg.summary());
}
return routeLegDescription.toString();
}

When testing a route, all of these are on the same route:
Route start: "Monterey Boulevard, Taraval Street"
Mid route: "Taraval Street, 28th Avenue"
End of route: "28th Avenue"

If faster route happens at any of these stages, it would think they are different routes

@kmadsen kmadsen mentioned this pull request Jul 8, 2020
11 tasks
@kmadsen
Copy link
Contributor Author

kmadsen commented Jul 8, 2020

Closing this as documentation. Comparing routes midway through a route is an open problem.

Another approach being considered here #3301

@kmadsen kmadsen closed this Jul 8, 2020
@kmadsen kmadsen deleted the km-show-alternatives-in-history-activity branch August 21, 2020 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants