-
Notifications
You must be signed in to change notification settings - Fork 318
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
Route alternatives (aka FasterRoute v2) #4375
Conversation
0289296
to
d158609
Compare
22502bf
to
7702f7f
Compare
What's in review
Should we add it now? - explicit call to request alternativesRight now there is a set interval to request alternatives periodically. But we could expose a function to force it to trigger "out of interval". The function is already available in the Out of scope - road aware alternative requestsSimilar to explicitly requesting explicitly. Considering how we could use the road information to determine when to request route alternatives (approaching intersections, enter highway, exit highway, ...). This sounds good but considering it out of scope because I'd like to scope it with nav native. Also we want to consider that, once the road tiles have been downloaded for things like EHorizon, the directions api request is not even needed. The onboard router is able to fulfill the request for alternatives - meaning a simple fast interval is also worth considering under these conditions. Out of scope - alternative route progressSimilar to how the current route has progress. A driver will be making progress on an alternative route. This will impact how a alternative route lines are shown on the map while driving. This is considered out of scope of the current change, because it will not impact the current interface being designed. Continuously showing alternative routes will be addressed after integrating this interface. |
2faa0ad
to
59f7477
Compare
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.
Looking good so far!
Should we add it now? - explicit call to request alternatives
I'm on the fence. Sounds like a great tool to give developers more flexibility and control when the request is triggered to build a UX around it. If we do that, we should add an option to disable the automatic alternative routes requests (RouteAlternativesOptions#enabled
) so that developers can fully own this. However, if we do that, we still need to have some kind of a throttling mechanism so that we do not allow for flooding our APIs too easily. cc @jyrigo @AhmerKhan1 for visibility.
Expose a isDifferentRoute function that allows you to compare cached routes or create "rejected lists"
Could you give an example of how to build that rejected list? What would be the criteria? There is one feature that I think we might be missing for this to be possible, like Navigator#isDifferentRoute(route1, route2)
.
...re/src/main/java/com/mapbox/navigation/core/routealternatives/RouteAlternativesController.kt
Outdated
Show resolved
Hide resolved
...re/src/main/java/com/mapbox/navigation/core/routealternatives/RouteAlternativesController.kt
Outdated
Show resolved
Hide resolved
...re/src/main/java/com/mapbox/navigation/core/routealternatives/RouteAlternativesController.kt
Outdated
Show resolved
Hide resolved
...re/src/main/java/com/mapbox/navigation/core/routealternatives/RouteAlternativesController.kt
Outdated
Show resolved
Hide resolved
...re/src/main/java/com/mapbox/navigation/core/routealternatives/RouteAlternativesController.kt
Outdated
Show resolved
Hide resolved
...c/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteAlternativesTest.kt
Show resolved
Hide resolved
...c/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteAlternativesTest.kt
Show resolved
Hide resolved
} | ||
|
||
private companion object { | ||
private val MINIMUM_REFRESH_INTERVAL = TimeUnit.SECONDS.toMillis(10) |
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.
Isn't that too often? I'm worried about the overhead that developers could even unintentionally cause.
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 agree. What about battery power consumption?
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.
Our default value is 5 minutes. The minimum allowed is 10 seconds. Customer feedback is typically, asking for lower lower thresholds.
For what it's worth, Lyft requested directions from Google once every 5 seconds in their driver app. Battery was not a concern because drivers are always plugged in.
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'm thinking more in terms of not flooding Directions API.
@kshehadeh @danpat we're reworking here a feature that periodically requests alternative routes (by making a new route request). What would be your recommendation for a minimal interval between those requests?
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.
@LukasPaczos Rather than hard-coding a refresh interval, I would do the following:
- Examine the
Cache-Control
header and look at themax-age
value. Set the refresh timer to trigger whenmax-age
runs out. - If
max-age
is not set, look for anExpires
header - if set, set the fresh timer to trigger whenExpires
occurs. - If neither header is set, use 60 seconds.
We do set Cache-Control
on all API responses currently, and intend to do so indefinitely. Using these headers will allow us to optimize the experience for all customers without triggering excessive refresh requests at a resolution greater than our traffic/incident data is updating.
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.
@danpat just to clarify, because it's not about a route expiring. It's considering active guidance. This is different from a refresh interval. This is, examining the alternatives while in-route. So the origin location is changing.
For example, I just used Google's version of this. I was driving and there was a truck blocking the street. I looked at my phone and saw I had an alternative with a similar ETA by turning right a few meters ahead. Keeping track of these alternatives, is not about the age of the route - they expire when your location changes.
I think we should cut a ticket for this feature so that we don't forget. cc @kmadsen |
Looking at the code in a first glance, there are 2 questions that come to my mind:
I guess my point is that we need to choose the nomenclature carefully as I think this is gonna lead to confusion and misinterpretation not only between customers but internally b/w support teams and TAM's as well. |
Responding to: #4375 (comment) cc: @abhishek1508
Good question and not exactly. For example,
Hmm good thought here. If you call
|
59f7477
to
82f7c90
Compare
Responding to #4375 (review) cc: @LukasPaczos
Good comments and questions. My opinion is that it is not needed for this change. Adding new options and functions will not be a breaking change - so we can continue to discuss and possibly add it in the future. Even as part of a minor version.
Thanks for the callout here. isDifferentRoute will not help identify previously rejected routes. The code we have to support "rejected lists" could be achieved by revising the RouteComparator.. but here we are deleting it. Worth requesting this from native cc: @SiarheiFedartsou |
@abhishek1508 https://github.com/mapbox/navigation-sdks/issues/985 |
82f7c90
to
61c8e59
Compare
@LukasPaczos @abhishek1508 An update after the feedback - so this is ready for another round.
The changes requested from the native team have a few different approaches. I think this change is still in the direction we want to go in, and we'll be able to follow up with improvements without breaking apis. What do you think? |
I'm thinking that we shouldn't. We advertise the When they want to dismiss, they just do nothing. When they want to display the alternatives, they call |
7e02a0d
to
3bfcd32
Compare
4c6a874
to
df2fb24
Compare
.../java/com/mapbox/navigation/instrumentation_tests/utils/idling/RouteRequestIdlingResource.kt
Outdated
Show resolved
Hide resolved
...re/src/main/java/com/mapbox/navigation/core/routealternatives/RouteAlternativesController.kt
Outdated
Show resolved
Hide resolved
...re/src/main/java/com/mapbox/navigation/core/routealternatives/RouteAlternativesController.kt
Outdated
Show resolved
Hide resolved
...core/src/main/java/com/mapbox/navigation/core/routealternatives/RouteAlternativesObserver.kt
Outdated
Show resolved
Hide resolved
3a697ff
to
1e487e5
Compare
...re/src/main/java/com/mapbox/navigation/core/routealternatives/RouteAlternativesController.kt
Show resolved
Hide resolved
5caa544
to
6c13e15
Compare
[Stephen Mcclure - Chat @ Spike](https://spikenow.com/r/a/?ref=spike-organic-signature&_ts=11ar0z) [11ar0z]
On May 18, 2021 at 17:22 GMT, Kyle Madsen ***@***.***> wrote:
@kmadsen commented on this pull request.
---------------------------------------------------------------
In [libnavigation-core/src/main/java/com/mapbox/navigation/core/routealternatives/RouteAlternativesController.kt](#4375 (comment)):
+ logger.e( + msg = Message("Route alternatives options are not available"), + tr = routeOptionsResult.error + ) + } + } + } + + private val routesRequestCallback = object : RoutesRequestCallback { + override fun onRoutesReady(routes: List<DirectionsRoute>) { + val routeProgress = tripSession.getRouteProgress() + ?: return + jobControl.scope.launch { + if (tripSession.getState() == TripSessionState.STARTED) { + val alternatives = routes.filter { navigator.isDifferentRoute(it) } + observers.forEach { it.onRouteAlternatives(routeProgress, alternatives) }
As for this fix. https://github.com/mapbox/mapbox-navigation-android/pull/4375/files#diff-9d5de4542e0fc70f55cc5dc3007a2636f073fb9a262f37b906bfcae09b72140eR142
When the routes are set it restarts the interval. Thinking about times when a new route is set and the interval happens to be close to requesting, would cause an inconsistent callback.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, [view it on GitHub](#4375 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AR65J23MGGRXWDT2PCYXBBDTOKO33ANCNFSM44RXRWIQ).
|
dfc3c6a
to
c98ab03
Compare
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.
8b52a6f
to
00f3156
Compare
00f3156
to
a14f188
Compare
Our usecase required it specifically to filter current route/routeProgress from the overall list of routes. With new implementation isDifferentRoute seems not necessary with filtered alternatives returned. |
Description
Addressing feedback on Faster Route and making a more flexible interface and feature called Route Alternatives. Resolves #4233.
Surfaces the ability to continuously monitor route alternatives, migrates the options interface to be consistent with other features, and showcases an integration test. More details in this comment
Changelog
Screenshots or Gifs
Pictures of the routes on the instrumentation test. The test is initialized with the slower route, the faster alternative will periodically refresh.
rendering-alternatives.mp4