-
Notifications
You must be signed in to change notification settings - Fork 658
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
Removed voice & banner instructions from last step and added ssmlAnnouncements #4644
Removed voice & banner instructions from last step and added ssmlAnnouncements #4644
Conversation
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 was not aware that the MapBox navigation SDK requires a ssmlAnnouncement
but thank you for adding that.
I'll have to check what happens in the MapLibre navigation SDK when the last bannerInstructions
and voiceInstructions
are empty, I assume that it still works.
Might also depend on the SDK version. In my case the app just crashed. I would guess mapbox navigation SDK set this as requiered at some point.
Would be really cool if you can do this. At least directions API V5 of mapbox always returns empty lists in the last step. So I guess it shouldn't be a probelm for Maplibre. In case of Mapbox Navigation SDK it just leads to the strange side effects mentioned in the Issue. |
Just wanted to give a small update on this: I've it running now with an Maplibre app. Works fine on my side |
Yeah, I can confirm that some versions of the Mapbox-derived SDKs will crash without this. In our API middle layer for navigation at Stadia Maps, we added SSML
This should work in every SDK I'm aware of (incl. newer ones like Ferrostar). Anyone able to fix up the conflicts + get this merged? Seems pretty straightforward and low-risk to me :D |
Thanks for updating the branch! Should be ready for merging now. |
I tried the directions from this branch and they work just as well. Thanks! I would love to see some further improvements in the voice directions. @Trietes you mentioned that you made them less annoying on short steps. Is that something we could incorporate into the main repository? |
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 have tried this and it works fine. Adding the ssmlAnnouncement
is nice improvement.
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.
thanks all!
@eikes I've opened a draft merge request here: #4696. It already got tested by @ianthetechie and by myself. I'm basically waiting for a maintainer to say he would be willing to merge it and then I would start cleaning it up and adding some tests to get it ready to merge. I would also appreciate it a lot if you would give it a test. Maybe you've also some suggestions how to improve it even more. Or we can add more improvements later on in more MR's |
@Trietes I will definitively give it a try! |
Issue
This PR is targetting to solve two of the issues mentioned in #4640. I've added simple voice ssmlAnnouncements to make the routing answers compatible with the navigation SDKs. I've also removed the voice and banner instructions from the last step since they are not required by the SDKs and just lead to confusing instructions on the screen.
Tasklist