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

Confusing onFasterRoute API #4233

Closed
yunikkk opened this issue Apr 6, 2021 · 3 comments · Fixed by #4375
Closed

Confusing onFasterRoute API #4233

yunikkk opened this issue Apr 6, 2021 · 3 comments · Fixed by #4375
Assignees

Comments

@yunikkk
Copy link

yunikkk commented Apr 6, 2021

Currently faster route logic will fetch faster route every 5 minutes (customizable, but not less then 2 minutes), and determine that route is faster, if it's at least 10% (not customizable) faster than the current one.
Then, after 5 minutes when faster route is requested, there are two possible situations:

Couple of questions / notes regarding this:

  1. If user would like to still make use of it, even if we don't say new route is faster, they'll need to filter current route in some way (also, maybe compare it with the current RouteProgress), facing with the same complexities that we've dealt in Compare routes using step names #3301.
    What I would expect is that alternatives list should not contain the current route at all. Suppose we could filter it out from the alternatives.

  2. By the way why the minimum 2 minutes limit was introduced? It's a bit weird that setting this value less that 2 minutes will just crash the application in runtime without any prior warnings.

  3. Could we customize 10% limit when calculating logic of faster route? We've ended up implementing similar logic on our side since had a requirement to show any faster route, even 1 second faster than current.

cc @mapbox/navigation-android; @kmadsen since you've implemented #3301

@kmadsen
Copy link
Contributor

kmadsen commented Apr 10, 2021

Hey thanks for the feedback! Some of the confusions are due to; porting an interface from v1 navigation, adding ideas, learning about the issues you listed. After we realized this is mostly about alternatives, there was some discussion about repurposing it as such (check linked issues)

IMO :)
Screen Shot 2021-04-09 at 9 00 15 PM

That's the background. Below will answer the numbered questions

  1. If user would like to still make use of it, even if we don't say new route is faster, they'll need to filter current route in some way (also, maybe compare it with the current RouteProgress), facing with the same complexities that we've dealt in Compare routes using step names #3301.
    What I would expect is that alternatives list should not contain the current route at all. Suppose we could filter it out from the alternatives.

Altering the RouteProgress into an object of relevant info is a good idea too. I just tried that for Arrival, but ended up using RouteProgress. Happy to chat more about this

  1. By the way why the minimum 2 minutes limit was introduced? It's a bit weird that setting this value less that 2 minutes will just crash the application in runtime without any prior warnings.

This was a product ask, see "Feature Change: Faster Route API" linked issue.

  1. Could we customize 10% limit when calculating logic of faster route? We've ended up implementing similar logic on our side since had a requirement to show any faster route, even 1 second faster than current.

Also love this idea. The 10% limit came from legacy. Would like to loop in iOS to make something consistent.

@Guardiola31337 Guardiola31337 added the needs discussion Can't be started in current state, needs clarification. label Apr 20, 2021
@kmadsen
Copy link
Contributor

kmadsen commented Apr 20, 2021

We spoke about estimating the work in this ticket, and agreed it was still too ambiguous to start work. I've translated it into a actionable proposal here https://github.com/mapbox/navigation-sdks/issues/956

The goal of the proposal is to address and close this ticket

@kmadsen kmadsen removed the needs discussion Can't be started in current state, needs clarification. label Apr 20, 2021
@kmadsen
Copy link
Contributor

kmadsen commented May 12, 2021

@yunikkk Please take a look #4375

That new interface is trying to close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants