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

Add Nav Native E-Horizon Support #2717

Closed
wants to merge 13 commits into from
Closed

Add Nav Native E-Horizon Support #2717

wants to merge 13 commits into from

Conversation

avi-c
Copy link
Contributor

@avi-c avi-c commented Oct 30, 2020

Draft PR for reviewing work on adding support for E-Horizon information flowing from Nav Native.

@avi-c avi-c marked this pull request as draft October 30, 2020 18:28
@avi-c avi-c added the ⚠️ DO NOT MERGE PR should not be merged! label Oct 30, 2020
@avi-c avi-c self-assigned this Oct 30, 2020
@avi-c
Copy link
Contributor Author

avi-c commented Nov 17, 2020

Dropping some notes here on concerns I have:

@1ễc5 is correct that it isn't clear to me how the EH will deliver the kinds of information that is needed for the user facing features.
An example is having the information needed to label the road names of upcoming intersections along your active route, or along the MPP in free nav.
Acting as a customer and trying to incorporate this "upcoming intersections" feature into the Nav SDK sample app, I don't know how I am supposed to accomplish this. Am I supposed to write code in the client to parse and walk the Edge tree to find the road names? If so, what logic am I intended to use to know how to look through the graph tree of Edge nodes? That seems like too difficult a problem to expect a the customer to solve. An API that provided the semantically appropriate information along the MPP would be idea.
It is possible that I am unaware of some work or API that exists. It's hard to keep track of all the Nav Native improvements since some of the work gets tracked in the customer repo, some gets tracked in the Android Nav SDK and some gets tracked in the Nav Native repo itself. If there are resources for this then please provide me some links.

I would strongly suggest that we pick a couple of key features, like upcoming road names, upcoming road classes, speed limits, etc. and construct a high-level API exposed through the Nav SDKs to deliver that information to clients apps. Then we can incorporate those features into the Nav SDK sample apps and let customers drive other uses cases, like BMWs request for exposing curvature of the upcoming roads.

If for some reason the product roadmap doesn't include putting the features into a Nav SDK for a while then I'd recommend adding the features into Apex, which I am willing to do.

@avi-c
Copy link
Contributor Author

avi-c commented Nov 17, 2020

After speaking to Pablo it sounds like the details of what's exposed by Nav Native for EH are still being worked on. The main customer driver for the feature is not being worked on until 2021.

For now the main concern of this ticket is to reach parity with Android which I think this PR does. I will probably move the visualization of MPP data to a separate debugging app though, depending on how much visualization code we want in the SDK itself.

@avi-c
Copy link
Contributor Author

avi-c commented Nov 19, 2020

Removed debug visualizer and added stubs for eHorizon callback functions. We can implement the debug visualizer in a client app rather than the SDK or sample app.

@avi-c avi-c marked this pull request as ready for review November 19, 2020 21:16
@avi-c avi-c requested review from 1ec5 and MaximAlien and removed request for 1ec5 and MaximAlien November 19, 2020 21:16
@avi-c
Copy link
Contributor Author

avi-c commented Nov 20, 2020

@1ec5 @MaximAlien - Some important points to consider:

  1. Adding E-Horizon support requires us to bump NN to at least v26.1.0. We may want to go further than this depending on what NN recommends but it is a big jump from what we have chosen for v1.1 (NN v22.0.5)

  2. Bumping NN required me to make some fixes here that are unrelated to EH and we might want to inherit from a different PR (Offline tiles, etc.)

  3. EH doesn't actually enable much now from a client app perspective. We do want to add the debug visualization of the EH to 1Tap so it can be reviewed by test drivers and to have parity with what is in 1Tap Android and this requires us to support it in the SDK, but it doesn't add much utility to anyone else just yet. Does that change anything about when we merge support or on which branch?

@avi-c
Copy link
Contributor Author

avi-c commented Nov 23, 2020

@zugaldia @Guardiola31337 - Tagging you here to see if you have suggestions as to which parts of the implementation go where (Nav SDK vs. Client app)

return Navigator(profile: settingsProfile, config: NavigatorConfig(), customConfig: "", tilesConfig: TilesConfig())
}()

let electronicHorizonOptions = ElectronicHorizonOptions(length: 2000, expansion: 1, branchLength: 50, includeGeometries: true)
Copy link
Contributor Author

@avi-c avi-c Nov 23, 2020

Choose a reason for hiding this comment

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

These length:expansion:branchLength: values come from what the Android implementation is doing now. We may want to change them later based on feedback from Nav Native.

Choose a reason for hiding this comment

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

@Guardiola31337
Copy link

@avi-c

Tagging you here to see if you have suggestions as to which parts of the implementation go where (Nav SDK vs. Client app)

What do you want to know exactly? What were you thinking on exposing from the SDK? On the Android side we only provide the core / data side of things from the SDK, there's nothing UI related exposed and that's developed in the app side. Does that help / answer your question?

@avi-c
Copy link
Contributor Author

avi-c commented Nov 24, 2020

It does help. So the Android side just added the couple of new callbacks for EH (EH updates and position updates) with the NN data structures and no extra processing or semantics added on top? This is pretty much where this PR is now.

@zugaldia
Copy link
Member

This is pretty much where this PR is now.

:shipit:

@Guardiola31337
Copy link

@avi-c

So the Android side just added the couple of new callbacks for EH (EH updates and position updates) with the NN data structures and no extra processing or semantics added on top?

Plus some utility functions from EHorizon (current and mpp) 👀

https://github.com/mapbox/mapbox-navigation-android/blob/c8ae2296fe577d51531e27b89b2091f2f4d77761/libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/model/eh/EHorizon.kt#L33-L69

@avi-c
Copy link
Contributor Author

avi-c commented Jan 11, 2021

I'll bring this up to date with my work on e-horizon in a test app. If possible it would be nice to merge the base support in soon to reach parity with Android.

@1ec5 1ec5 added this to the e (android-v1.5.0 / ios-v1.3.0) milestone Jan 14, 2021
@1ec5
Copy link
Contributor

1ec5 commented Feb 25, 2021

MapboxNavigationNative has revamped its e-horizon functionality and API. #2834 will implement the new version for navigation SDK v2.0.0.

@1ec5 1ec5 closed this Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ DO NOT MERGE PR should not be merged!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants