Adapt voice hints and exporting to changes in BRouter 1.7.X #753
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abrensch/brouter@c9ae7c8681 added support for two new voice hints: A hint for beelines (`BL`), and a hint for 180 degree u-turns (`TU`). By adding support for both, we now know about all types of voice hints as defined in BRouter again. What makes things confusing is that the `TU` name for the respective `static final int` constant in BRouter's `VoiceHint.java` was repurposed for 180 degree u-turns, with left u-turns now being mapped to the new `TLU` constant name. Also note that originally the indexing of voice hints as used in BRouter's GeoJSON API has been changed as well due to inserting new commands in the middle of the numbering scheme instead of at the end. This API break has been fixed only in abrensch/brouter@82fecf9. Here we will rely on the fixed indexing, BRouter versions 1.7.0 and 1.7.1 without the re-indexing revert will not be supported. In addition, the voice hint mapping table has been checked to be identical to BRouter (this led to adding a missing `OFFR` symbol), and clarifying comments for planned future changes (e.g. changing the `TU` output token to `TLU` for OsmAnd) have been added. Note that beelines and 180 degree u-turns are only added to the mapping table for completeness. As BRouter-Web is handling straight lines on the client-side exclusively (which makes sense performance-wise when loading a route from a pasted URL with lots of them tracing an unmapped path), they are not expected to be in any GeoJSON response from BRouter, at least as of now. The same is true for 180 degree u-turn voice hints at cul-de-sac-style vias. If and when to emit voice hints for both cases in BRouter-Web itself is a different question, though it could likely also use the table for lookup. Test Plan: - `yarn test` - Confirm voice hints for routes with roundabouts and u-turns are unchanged.
abrensch/brouter@c9ae7c8681 changed indexing of voice hint ids, because some new hints were inserted in the middle instead of strictly at the end, changing the numbering of existing ids. For example, now id `12` was sent to indicate a right u-turn, while we still assume the old meaning of `12`, i.e. "off route". This clearly was an API break. This leads us to abort exporting with the Gpsies turn instructions style, since `OFFR` has an `undefined` symbol assigned, as well as emitting wrong voice hints for ids after `9`. Another unwelcome side effect is showing negative exit numbers for roundabouts. This breakage in the GeoJSON HTTP API has been shipping in BRouter 1.7.0 and 1.7.1 and finally got fixed with abrensch/brouter@82fecf9 contained in BRouter 1.7.2 or later. Earlier releases like 1.6.3 are also unaffected. To avoid emitting incorrect voice hints in BRouter-Web, running with broken versions of BRouter should be avoided. By checking the "Creator" field after receiving the first response from BRouter, we can now emit a warning if the version of BRouter used is unsupported. The warning mostly targets administrators and power users, i.e. those responsible for choosing the software versions used, and it is also only shown once per session. Note that the version check is compatible with the common "SemVer" scheme, so the check should continue working and even support more complex version compatibility scenarios as long as BRouter stays SemVer-compliant. Ref #751 Test Plan: - Run with BRouter 1.6.3 and 1.7.2, no warnings shown. - Run with BRouter 1.7.0 and 1.7.1, warnings shown only for the first segment.
BRouter 1.7.0 implemented support for three new export formats: "Cruiser", "BRouter internal" and "Locus(-new)". "Cruiser" (`turnInstructionMode=8`) and "BRouter internal" (`turnInstructionMode=9`) are not yet exposed in BRouter-Web's UI through profiles, so we do not need to implement them at the moment. Here we only document them by making them explicit unimplemented `cases` in the code. In addition, BRouter changed "locus-style" with `turnInstructionMode=2` to emit a different format for newer releases of Locus, while the old format is now referred to as "locus-old-style" from profiles with `turnInstructionMode=7`. Since BRouter-Web does not know yet about the the new id, exports will fail with "unhandled turnInstructionMode" errors. To fix the latter issue, we now map `turnInstructionMode=7` to the newly renamed `LocusOldVoiceHints()`. Note that `turnInstructionMode=2` is also currently using `LocusOldVoiceHints()`, i.e. the new format still needs an implementation. Test Plan: - `yarn test` - Check choosing "locus-old-style" now exports without an error.
While abrensch/brouter@82fecf9 fixed the export already by avoiding the problematic `OFFR` voice hint, we would still throw in case we encountered an `undefined` symbol entry: > Uncaught TypeError: Cannot read properties of undefined (reading > 'toLowerCase') This can be prevented by not calling `toLowerCase()` on `undefined` objects. Fixes #751 Test Plan: - Change `VoiceHints.commands` to contain `undefined` entries for a particular voice hint. - Create a route with that voice hint. - Test that exporting for each `turnInstructionMode` does not throw.
This was referenced Jul 12, 2023
|
Thanks for the PR! I'm fine with merging that as is.
Fine by me.
Not at all, other than providing a working standalone bundle with each release. On my to-do list is to include brouter as a dev or peer dependency, which initially wasn't possible because of the GPL license. |
|
I'll come back to this once the dust in BRouter has settled. We can probably salvage most of it.
That's a nice idea to make it easier to run for new contributors or on a server for sure. It sounds complicated to get working due to the Java-based nature of BRouter though, but admittedly I have not tried yet. Hopefully enough flexibility will be kept to be able to override this with custom BRouter versions (a necessity exemplified by the current situation). |
rkflx
changed the title
|
The reindexing revert in BRouter got in, phew. Since brouter.de/brouter-web seems to be running with or very close to BRouter master, it will be fixed there without needing a new BRouter-Web release. Hence I reworked this patch series:
|
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Patches contained in this series (see commit messages for details), all relating to adapting to changes in BRouter 1.7.0:
Further changes would be needed (e.g. new Locus format, straight line voice hints, 180° u-turn voice hints at cul-de-sac-style vias, height calculation buffers etc.), but that's all there is for now.
Regarding the new console warning if BRouter is too old, I'm open to better suggestions, of course. I haven't added this new requirement to the CHANGELOG yet, but it should probably be mentioned there and in the release notes once a new release is tagged. Not sure how increasing the required BRouter version was handled in the past.