Support for visual sub-maneuvers #303

ahmedre · 2024-10-21T17:34:21Z

Within BannerInstructions, the MapBox API allows for a sub_content for describing sub-maneuvers. This data is used either for showing the step after the current one if it is to happen really quickly after the current step (ex turn left then turn right), or to specify lane information. This PR implements this within Ferrostar.

ahmedre · 2024-10-21T17:35:48Z

relevant urls:

I set this as a draft because I am not sure if this approach is correct, especially considering Valhalla responses (which I didn't think about too much), and about different types of subs, and about the overall model structuring. I figured it was easier to discuss over a PR than just text, so happy to make changes accordingly.

Also, I guess I am breaking backward compatibility, but am not sure about what the recommended way to fix this is.

common/ferrostar/src/routing_adapters/osrm/models.rs

ahmedre · 2024-10-21T17:42:15Z

common/ferrostar/src/routing_adapters/osrm/models.rs

+pub struct SubManeuverBannerContentComponent {
+    #[serde(rename = "type")]
+    pub component_type: Option<String>,
+    pub directions: Option<Vec<String>>,


i only focused on directions here, but didn't think too much about other subs, like just lists of type + text, or type + image_base_url, etc, as per the sample json in this file. should we handle these also?

We're generally fine merging partial implementations if it's well-documented what's missing in an appropriate place. In this case, I think just a quick comment noting that we parse these but don't use them yet is enough (this is just the low-level OSRM model anyways, and we usually try to make these extremely complete, so that data is there if we need it).

Re: the sample JSON in this file, here's a gist with a sample route that we can add to the project. This one comes from Valhalla via our API (so, FWIW, Valhalla also does lane guidance :D). https://gist.github.com/ianthetechie/fc3c983b3242a84987ec7ddf468971ac

I added a new test for this, and updated the old test to also do some checks here.

👍

You know what's funny? I just realized we already have another example from Mapbox's public docs which I had forgotten about (see the deserialize_banner_instruction test) 😂 Let's keep both; good to have diversity of API tests in here. We could probably turn one or both of those into snapshot tests now that I think of it.

ianthetechie

Wow, thanks for the PR! I think this is pretty much the right direction; just a few comments on models.

I've just reviewed the Rust side of things for now; once we're aligned on data modeling, we can move up to the UI layer (wasn't expecting that in the PR too; thanks!) :D

Also, I guess I am breaking backward compatibility, but am not sure about what the recommended way to fix this is.

You're talking about semver compatibility, right? Or was there something I missed / didn't review?

Generally speaking, this project is pre-1.0, and a significant number of the organizations actively using it are also engaged in development, so don't worry too much about breaking changes as long as they are announced.

EDIT: I see you might have just been asking what do to in that case ;) Just run the update release version shell script and commit the files (careful with Package.swift; if you are using the local development flag, you'll need to unstage that change :D)

common/ferrostar/src/routing_adapters/osrm/models.rs

ianthetechie · 2024-10-22T02:41:16Z

common/ferrostar/src/routing_adapters/osrm/models.rs

+pub struct SubManeuverBannerContentComponent {
+    #[serde(rename = "type")]
+    pub component_type: Option<String>,
+    pub directions: Option<Vec<String>>,


We're generally fine merging partial implementations if it's well-documented what's missing in an appropriate place. In this case, I think just a quick comment noting that we parse these but don't use them yet is enough (this is just the low-level OSRM model anyways, and we usually try to make these extremely complete, so that data is there if we need it).

Re: the sample JSON in this file, here's a gist with a sample route that we can add to the project. This one comes from Valhalla via our API (so, FWIW, Valhalla also does lane guidance :D). https://gist.github.com/ianthetechie/fc3c983b3242a84987ec7ddf468971ac

common/ferrostar/src/models.rs

common/ferrostar/src/routing_adapters/osrm/models.rs

ahmedre · 2024-10-22T12:52:34Z

common/ferrostar/src/routing_adapters/osrm/models.rs

+pub struct SubManeuverBannerContentComponent {
+    #[serde(rename = "type")]
+    pub component_type: Option<String>,
+    pub directions: Option<Vec<String>>,


I added a new test for this, and updated the old test to also do some checks here.

ahmedre · 2024-10-22T12:56:39Z

common/ferrostar/src/routing_adapters/osrm/models.rs

+    pub image_base_url: Option<String>,
+    pub abbr: Option<String>,
+    pub abbr_priority: Option<u8>,
+    pub active_direction: Option<String>,


btw, we have Lane already for intersections (though it's not exposed right now) - is that okay or do you think this would cause any confusion in the future?

Correct; intersections also have detailed lane info, which is what I originally thought this PR was about 😂 My current understanding (not having designed either API) is that these are complementary, but that the Mapbox usage is the more common one across navigation SDKs. The detailed intersection info may be useful in specific contexts though, so we modeled it just in case.

ianthetechie

Fantastic; thanks!

I also had a quick look over the UI stuff and that all looked fine.

Is this PR still considered a draft? Were you also planning to push SwiftUI+Jetpack Compose views for the sub-instruction layout? (No worries if not; I know you guys also do a lot of custom UI; that can come in a later PR if it's not immediately available. This looks solid to merge as-is.)

ianthetechie · 2024-10-22T13:19:54Z

common/ferrostar/src/routing_adapters/osrm/models.rs

+pub struct SubManeuverBannerContentComponent {
+    #[serde(rename = "type")]
+    pub component_type: Option<String>,
+    pub directions: Option<Vec<String>>,


👍

You know what's funny? I just realized we already have another example from Mapbox's public docs which I had forgotten about (see the deserialize_banner_instruction test) 😂 Let's keep both; good to have diversity of API tests in here. We could probably turn one or both of those into snapshot tests now that I think of it.

ianthetechie · 2024-10-22T13:26:23Z

common/ferrostar/src/routing_adapters/osrm/models.rs

+    pub image_base_url: Option<String>,
+    pub abbr: Option<String>,
+    pub abbr_priority: Option<u8>,
+    pub active_direction: Option<String>,


Correct; intersections also have detailed lane info, which is what I originally thought this PR was about 😂 My current understanding (not having designed either API) is that these are complementary, but that the Mapbox usage is the more common one across navigation SDKs. The detailed intersection info may be useful in specific contexts though, so we modeled it just in case.

_typos.toml

common/ferrostar/src/routing_adapters/osrm/models.rs

ianthetechie · 2024-10-22T13:39:51Z

Looks like a few failures CI caught which are easy to fix (missing params etc.)

Co-authored-by: Ian Wagner <ianwagner+github@switzerlandmail.ch>

* Update ferrostar version * Add prettier * Run Prettier * Update guide * Rename format to format:fix, add format:check script * Add format:check to GitHub workflow

* Support arbitrary Valhalla options (formerly we only merged costing options) * Fix argument * Improve docs of some convenience initializers * Align Android with iOS costing options * swiftformat * Don't forget the web ;) * Add Android-specific README * Set up Android to use API keys properly via local.properties * Fix CI * Fix subtle bug where Android location providers never updated lastLocation * Rename U-Turn images to match how Kotlin stringifies the maneuvers * Allow the fused location provider to report an initial fix faster * First pass at an actually usable demo app * Minor build script improvement * Checkpoint commit phase 1 * Checkpoint commit phase 2 highlighting some bad stuff to clean up * Add docs on the StaticLocationEngine * Fix formatting + doc test * Fix typo in Android readme Co-authored-by: Jacob Fielding <jf0517@gmail.com> * Kotlin let closure idiom * ktfmtFormat + auto-commit * Remove extraneous env? * Try switching to a mix of Apple Silicon Ubuntu runners for Android * Fix runner permissions (I think) * Apply automatic changes * Address open thread re: iOS uturn image name * Add TODO docs * Fix the navigation camera (re-)application logic * Cleanup * Remove old comment * Apply automatic changes --------- Co-authored-by: Jacob Fielding <jf0517@gmail.com> Co-authored-by: ianthetechie <ianthetechie@users.noreply.github.com>

ahmedre · 2024-10-22T13:51:00Z

had to merge with main to do the fixes but should hopefully be good now

ahmedre · 2024-10-22T13:53:19Z

for the UI, happy to try this in another PR - we're still wrapping a ManeuverView from maplibre-navigation so would be a good opportunity for us to not depend on it anymore 👍

ianthetechie

Sounds good on the UI in a separate PR. Thanks! Can merge this as soon as CI passes.

ahmedre force-pushed the support_sub_maneuvers branch from 5cd1245 to 0e20949 Compare October 21, 2024 17:38

ahmedre commented Oct 21, 2024

View reviewed changes

ahmedre force-pushed the support_sub_maneuvers branch 4 times, most recently from 4913e37 to e3699cc Compare October 21, 2024 19:22

Support for visual sub-maneuvers

5aadeac

ahmedre force-pushed the support_sub_maneuvers branch from e3699cc to 5aadeac Compare October 21, 2024 20:32

ianthetechie reviewed Oct 22, 2024

View reviewed changes

ahmedre added 3 commits October 22, 2024 15:15

Address PR comments

41e8e86

Added and updated tests

ac7a8d3

Update release version

4016dba

ahmedre commented Oct 22, 2024

View reviewed changes

ahmedre added 2 commits October 22, 2024 17:02

Fix a typo

6f245d6

Update _typos.toml to ignore extended response

15ecad7

ianthetechie reviewed Oct 22, 2024

View reviewed changes

ahmedre and others added 4 commits October 22, 2024 17:47

Update common/ferrostar/src/routing_adapters/osrm/models.rs

5ec611c

Co-authored-by: Ian Wagner <ianwagner+github@switzerlandmail.ch>

Add Prettier (#304)

d12757c

* Update ferrostar version * Add prettier * Run Prettier * Update guide * Rename format to format:fix, add format:check script * Add format:check to GitHub workflow

Fix ManeuverImage

fa2bbf8

ahmedre marked this pull request as ready for review October 22, 2024 13:51

ahmedre added 3 commits October 22, 2024 18:28

Merge branch 'main' into support_sub_maneuvers

107c799

Fix ManeuverImage again

f4a5ac7

Fix newline ending

ae580b2

ianthetechie approved these changes Oct 22, 2024

View reviewed changes

ianthetechie merged commit 359ec90 into stadiamaps:main Oct 22, 2024
14 checks passed

ahmedre deleted the support_sub_maneuvers branch October 22, 2024 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for visual sub-maneuvers #303

Support for visual sub-maneuvers #303

ahmedre commented Oct 21, 2024

ahmedre commented Oct 21, 2024 •

edited

Loading

ahmedre Oct 21, 2024

ianthetechie Oct 22, 2024

ahmedre Oct 22, 2024

ianthetechie Oct 22, 2024

ianthetechie left a comment •

edited

Loading

ianthetechie Oct 22, 2024

ahmedre Oct 22, 2024

ahmedre Oct 22, 2024

ianthetechie Oct 22, 2024

ianthetechie left a comment

ianthetechie Oct 22, 2024

ianthetechie Oct 22, 2024

ianthetechie commented Oct 22, 2024

ahmedre commented Oct 22, 2024

ahmedre commented Oct 22, 2024

ianthetechie left a comment

Support for visual sub-maneuvers #303

Support for visual sub-maneuvers #303

Conversation

ahmedre commented Oct 21, 2024

ahmedre commented Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianthetechie left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianthetechie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianthetechie commented Oct 22, 2024

ahmedre commented Oct 22, 2024

ahmedre commented Oct 22, 2024

ianthetechie left a comment

Choose a reason for hiding this comment

ahmedre commented Oct 21, 2024 •

edited

Loading

ianthetechie left a comment •

edited

Loading