Faster route: part 1 #6434
Faster route: part 1 #6434
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6434 +/- ##
==========================================
Coverage 69.30% 69.30%
- Complexity 4718 4752 +34
==========================================
Files 702 710 +8
Lines 27704 27966 +262
Branches 3269 3295 +26
==========================================
+ Hits 19199 19381 +182
- Misses 7216 7290 +74
- Partials 1289 1295 +6
|
| // this one feels like similar to initial alternative because they both go through A9 | ||
| "UOIW_1UUIDFfyICssWNnKB2o4cANnHc5pQ4WjsBOKW694GD7ZFwG5Q==#1", |
There was a problem hiding this comment.
I'm thinking about comparing routes not only by geometry, but also using summary see calculateDescriptionSimilarity. That would help with the cases when routes have not super similar geometry, but follow the same highway for example. The main thing that stop me is the fact that summary filed from route leg doesn't have stable format.
There was a problem hiding this comment.
@VysotskiVadim have you considered using LegStep#ref or LegStep#name? This paired with the distance annotation (or any other mean to create a ratio of distance traveled per ref/name) should give us an understanding of the road network that is used for a specific route.
qa-test-app/src/main/java/com/mapbox/navigation/qa_test_app/view/AlternativeRouteActivity.kt
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private val fasterRouteObserver = NewFasterRouteObserver { newFasterRoute: NewFasterRoute -> |
There was a problem hiding this comment.
@korshaknn , @Zayankovsky , your feedback about faster routes API will be super valuable as you guys will be the first users :)
VysotskiVadim
force-pushed
the
NAVAND-611-compare-similar-routes
branch
from
4f8993f to
7a51baa
Compare
| fun `faster route is available driving in Munich after picking the slowest road`() = | ||
| runBlocking<Unit> { | ||
| val fasterRoutesTracker = createFasterRoutesTracker() | ||
| val routeUpdates = readRouteObserverResults(FASTER_ROUTE_IN_MUNICH) | ||
| val fasterRoutesIds = mutableListOf<String>() | ||
| for (recordedUpdate in routeUpdates) { | ||
| val result = fasterRoutesTracker.routesUpdated( | ||
| recordedUpdate.update, | ||
| recordedUpdate.alternativeMetadata.values.toList() | ||
| ) | ||
| if (result is FasterRouteResult.NewFasterRoadFound) { | ||
| fasterRoutesIds.add(result.route.id) | ||
| } | ||
| } | ||
| assertEquals( | ||
| listOf( | ||
| "JJXKpAxo3Yhh3rbJJmToNPPkzjh-hJHZV8u2Uksl1gwue_3sdZZ0ig==#1", | ||
| "b_dZe9Trx9MkOIphyCILePn4cBI6taAQmSctw1k5jaNzWz8vL-10-w==#1", | ||
| "b_dZe9Trx9MkOIphyCILePn4cBI6taAQmSctw1k5jaNzWz8vL-10-w==#1" | ||
| ), | ||
| fasterRoutesIds | ||
| ) | ||
| } |
There was a problem hiding this comment.
Now I realise that it's hard to read those tests because routes ids don't say much about scenario. I recorded seeing the map and routes, but without it reader has to manually check geometries of the routes to say if test has a valid scenario. Maybe if I put links to geometries decoder it helps 🤔
|
I think it can be tested in instrumentation tests.
|
|
|
||
| @ExperimentalMapboxNavigationAPI | ||
| class FasterRouteOptions internal constructor( | ||
| val maxSimilarityToExistingRoute: Double |
There was a problem hiding this comment.
Can you add docs to explain how the similarity is measured?
libnavigation-core/src/main/java/com/mapbox/navigation/core/fasterroute/FasterRouteOptions.kt
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/fasterroute/FasterRouteOptions.kt
Outdated
Show resolved
Hide resolved
libnavigation-core/src/main/java/com/mapbox/navigation/core/fasterroute/FasterRouteTracker.kt
Outdated
Show resolved
Hide resolved
| object NoFasterRoad : FasterRouteResult() | ||
| data class NewFasterRoadFound( | ||
| val route: NavigationRoute, | ||
| val fasterThanPrimary: Double, |
There was a problem hiding this comment.
It seems like boolean, can it be sth like primaryRouteDiff? It should be clear that it's faster because of the class name.
| private fun toJson(metadata: AlternativeRouteMetadata): String { | ||
| val gson = GsonBuilder() | ||
| .registerTypeAdapter( | ||
| NavigationRoute::class.java, |
There was a problem hiding this comment.
Is this class correct? I mean you pass AlternativeRouteMetadata here, not NavigationRoute.
There was a problem hiding this comment.
yep. gson can serialise AlternativeRouteMetadata on its own using reflections. But I don't want it to serialize whole navigation route + I doubt that it can this specific field. So I customise only navigation route serialisation
| if (!folder.exists()) { | ||
| folder.mkdir() | ||
| } else { | ||
| folder.deleteRecursively() |
There was a problem hiding this comment.
Do you really have to delete it? 1 observer = 1 folder? What if some SDK creates an instance of this observer and an app creates a second one? There can only be 1 folder and some of the already recorded data will be erased.
It should be controlled somehow (e. g. forbid creating several instances of this observer) or multiple observers should be supported.
| "record-routes-observer" | ||
| ) | ||
| if (!folder.exists()) { | ||
| folder.mkdir() |
There was a problem hiding this comment.
It can return false. Don't you want to handle this situation?
| scope.launch { | ||
| val folder = preparation.await() | ||
| val file = File(folder, fileName) | ||
| file.writeText(content) |
There was a problem hiding this comment.
It can throw an exception, we should catch that.
.../src/main/java/com/mapbox/navigation/core/internal/fasterroute/RecordRouteObserverResults.kt
Outdated
Show resolved
Hide resolved
This is one of the goals: handle the case when users pick slower route on purpose. The SDK shouldn't bother users with the faster route if it has been already rejected by user. |
| public final class SimilarRoutesKt { | ||
| } |
There was a problem hiding this comment.
who knows how to get rid of this?
There was a problem hiding this comment.
If I'm not mistaken it's a metalava issue and we leave it as is.
But you only add rejected routes for reasons "new" and "reroute". In the case I provided it's "alternative". |
| * New instance is created for the first call or after calling [FasterRoutesTracker.destroy] on existing | ||
| */ | ||
| @ExperimentalPreviewMapboxNavigationAPI | ||
| @UiThread |
There was a problem hiding this comment.
nit: This is redundant, the whole class is tagged as @UiThread.
| ) | ||
|
|
||
| @ExperimentalPreviewMapboxNavigationAPI | ||
| private var fasterRoutesInstance: FasterRoutesTracker? = null |
There was a problem hiding this comment.
This should be destroyed in MapboxNavigation#onDestroy since it's tightly coupled with the MapboxNavigation instance (including the coroutine scope).
There was a problem hiding this comment.
FasterRoutesTracker takes coroutine scope as a parameter and stops working as soon as it cancels. FasterRoutesTracker was designed just to destroy FasterTracker earlier than MapboxNavigation. I'm already thinking of removing it to avoid confusions and simplify things.
| import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI | ||
|
|
||
| @ExperimentalPreviewMapboxNavigationAPI | ||
| data class FasterRouteOptions internal constructor( |
There was a problem hiding this comment.
| data class FasterRouteOptions internal constructor( | |
| class FasterRouteOptions internal constructor( |
There was a problem hiding this comment.
I think it's okay to use data classes while feature is in experimental stage. I don't want to rewrite equals, toString,
getHashCode every time I change set of fields. I plan to stop using data classes when feature goes stable and then implement all 3 methods. What do you think about this approach?
| ) { | ||
| class Builder { | ||
|
|
||
| private var maxGeometrySimilarityToRejectedAlternatives = 0.5 |
There was a problem hiding this comment.
This ratio might work for intercity routes (or longer) but might make us unable to suggest faster alternatives in city centers that often rely on taking a turn earlier to later rejoin the original route and avoid congested intersection. These types of routes could have both a significant overlap while still having a significant time delta. I'm just speculating but just wanted to mention this as a discussion point, is there a more complex metric that we could use to account for these situations? Maybe road classes could be taken into account.
There was a problem hiding this comment.
make us unable to suggest faster alternatives in city centers that often rely on taking a turn earlier to later rejoin the original route and avoid congested intersection
We do not compare alternative with the primary route. But if there is a rejected alternative which is very similar to the primary route, it could be a problem.
What if we consider initial alternatives as rejected only if user picks slower route? It will decrease chance of this happening.
There was a problem hiding this comment.
is there a more complex metric that we could use to account for these situations?
I will try to add comparison of steps names
There was a problem hiding this comment.
As an experiment, I switched from comparing geometries to calculateStreetsSimilarity which calculates how much distance of the streets with the same name routes shares.
I see following corner cases with this approach:
- It may happen that routes which are going through the different parts of the same streets may be considered similar (I don't think it's going to happen in case of alternative routes)
- streets could have the same names in different cities
| * Stops observation and processing routes from [MapboxNavigation] | ||
| */ | ||
| @UiThread | ||
| fun destroy() { |
There was a problem hiding this comment.
I think we're expecting too much from the user of this API by exposing the whole object, and also making the user maintain the lifecycle. Why don't we always collect the routes in the FasterRouteTrackerCore and process them only if at least one observer is registered, and at the same time hide the lifecycle under the MapboxNavigation hood?
There was a problem hiding this comment.
Why don't we always collect the routes in the FasterRouteTrackerCore and process them only if at least one observer is registered, and at the same time hide the lifecycle under the MapboxNavigation hood?
I wanted to do the same at the beginning, but then I thought "why do I need add such a complex initialisation if I'm not even sure that the feature makes sense?". So I decided to keep things simpler while feature is experimental.
I think we're expecting too much from the user of this API by exposing the whole object, and also making the user maintain the lifecycle.
After seeing the comments I realised that the lifecycle might be confusing. It's an experimental feature and I can simplify lifecycle by removing destroy. Without calling destroy the feature lives as long as MapboxNavigaiton. I think it's okay for an experimental feature. If we decide that the feature makes sense, I will implement sophisticated system with initialisation and destructions of faster routes. What do you think about it?
| * Sets faster route as primary to [MapboxNavigation] | ||
| */ | ||
| @UiThread | ||
| fun acceptFasterRoute(newFasterRoute: NewFasterRoute) { |
There was a problem hiding this comment.
| fun acceptFasterRoute(newFasterRoute: NewFasterRoute) { | |
| fun acceptFasterRoute(newFasterRoute: NewFasterRoute, callback: RoutesSetCallback? = null) { |
| currentRoutes.filterNot { it == newFasterRoute.fasterRoute } | ||
| ) | ||
| } else { | ||
| logE("Ignoring accepted faster route as it's not present in current routes") |
There was a problem hiding this comment.
| logE("Ignoring accepted faster route as it's not present in current routes") | |
| callback?.onRoutesSet(ExpectedFactory.createError(RoutesSetError("Ignoring accepted faster route as it's not present in current routes"))) |
| val points = a.directionsRoute.completeGeometryToPoints() | ||
| val segments = mutableSetOf<Segment>() | ||
| var previousPoint: Point? = null | ||
| for (point in points.drop(1).dropLast(1)) { |
There was a problem hiding this comment.
@VysotskiVadim why would destination point not match?
| } | ||
|
|
||
| private data class Segment(val from: Point, val to: Point) { | ||
| val length get() = TurfMeasurement.distance(from, to) |
There was a problem hiding this comment.
This is expensive. Not only the calculation itself but because we re-calculate it on each call instead of caching the result.
val length = TurfMeasurement.distance(from, to)
Have you considered summing it LegAnnotation#distance when available?
| val diff = a.toMutableSet().apply { | ||
| removeAll(b) | ||
| } | ||
| return (1.0 - (aggregator(diff) / aggregator(a))) |
There was a problem hiding this comment.
All of this looks rather expensive (a lot of Point#equals checks paired with recalculation of distance between points). On top of that, we will be rerunning the calculations for the same pairs of routes every time (unless rejected). Have you tested the performance for long routes?
RingerJK
left a comment
RingerJK
left a comment
There was a problem hiding this comment.
I focused on the architecture aspect and ignored the technical side. Overall it looks good, a few notes:
- looking for a faster route should also work for
refresh routereason; - why does by default rejected routes that are set with reason
ROUTES_UPDATE_REASON_NEW?; - it may make sense that a faster route works in the same scope as routes in MapboxNavigation (then it will be declined immediately when routes are re-set or clean-up).
|
Late suggestion here but I think you should consider building these experimental features outside of MapboxNavigation. Faster route is now an experience that does not need to be built into MapboxNavigation. For example, you will not need to add the If you were to define a class FasterRouteProvider(options: FasterRouteOptions) : MapboxNavigationObserver {
private val currentLetIndex: Int -1
private val previousRouteLeg: RouteLeg
// anything you find necessary or want to experiment with
..
companion object {
private const val MIN_THRESHOLD_METERS = 120L
..
}
}
MapboxNavigationApp.registerObserver(fasterRouteProvider)
fasterRouteProvider.state.collect { fasterRoute ->
...
}It can also be disabled with clear expectations. |
5 participants
Description
This PR introduces experimental "faster route" feature. For the first step I handled a case when the SDK receives an alternative which is faster than primary route, the SDK suggests the alternative as a faster route. But the SDK shouldn't suggest faster alternative if it's very similar to the alternative that was already rejected.
Test case
I used route from Munich to Nuremberg for testing. You will find recorded routes update with this route in unit tests.
You have two options: via Ingolstadt (faster) and Regensburg (slower).
If user picks the slower route via Regensburg and passes the fork point, the Navigation SDK generates a new alternative: turn around and go to the Nuremberg via Ingolstard, which is faster than primary route and has new alternative id.
The new alternative has a very similar geometry, so
FasterRouteTrackerrejects it as very similar to existing alternative.Next steps
I won't handle all the cases in this PR because I prefer moving with small steps. I'm going to handle following things in the upcoming PRs:
Routs similarity
I considered a few ways to calculate how similar routes are.
I tested 3 of them.
I didn't like calculating Levinstein distance between leg's summaries like iOS does. For example if one route has "Maddison Avenue" the other has "Maddison Garden" their leveinstein distance is 6, what makes them pretty similar despite the fact that they're not.
I liked the result of comparing parsed summary + looking for how many roads name they have in common. See
calculateDescriptionSimilarity. The problem is that Directions API doesn't guarantee stability of summary, it's design to be read by humans.So I ended up comparing geometries.
Note for reviewers
The faster route feature isn't as clean as the rest of the SDK. But TBH I don't see much value in keeping experimental feature super clean as I don't know yet if we need it in the future. We will try to use it and then decide if it makes sense to have it at all. But if you see something very dirty that you won't accept even in experimental features, please comment and I will fix it.