Port to new string-based BRouter voice hint GeoJSON API

[rkflx]

This is just a draft to show how little changes a string-based GeoJSON API will actually require. It will make the code more readable an protect against any future changes to voice hint ids / indexing.

(Only the latest commit is relevant, since this is based on #753. I'll rebase this once the other PR got merged. Sadly stacked PRs in GitHub don't really work for external contributors.)

The corresponding change to BRouter is available here, with extensive reasoning in the commit message (probably futile in the end anyway, but at least I tried). I'd recommend to look at the individual commits, most of them are just optional cleanup on top, which became possible by using an enum.

Of course this is a breaking API change, but compared to the previous sneaky re-indexing (see abrensch/brouter#584) it would be explicit at least.

Another possibility would be to add this as an additional format=geojsonWithVoiceHints, which would use strings rather than ids and include voice hints by default:

• We'd not break other BRouter GeoJSON/JSON API users while keeping the advantages of strings over ids.
• We'd be protected from future changes to how timode=2 affects GeoJSON voice hints. Currently this is a rather crude way of triggering them, see also abrensch/brouter#531.
• The decision which format to export to (by selecting the respective radio button in the Web UI) and whether to include voice hints or not (BRouter-Web should add a checkbox for this eventually to improve UX, instead of hiding it in the profile via turnInstructionMode where only experts will find it) is only taken after BRouter-Web already requested all data from BRouter. Having to recalculate the whole route (as currently triggered by editing the profile) should be avoided.
• Therefore BRouter-Web should IMO unconditionally request voice hints at all times (even if they might get dropped later in some cases), which is also far cheaper than recalculation. In terms of bandwith, they are small compared to the geometry and message arrays.
• Implementation note 1: Extra data for comment-style hints would then need to be enabled by default.
• Implementation note 2: timode=2 seems to be overridden if another turnInstructionMode is set in the profile (need to check). Not sure yet how to trigger voice hints instead, the early return for detourMap == null in processVoiceHints might be a hint.

Any thoughts appreciated on how to best go forward with it, or whether to just drop the idea entirely.

[nrenner]

This indeed would be an improvement.

Another possibility would be to add this as an additional format=geojsonWithVoiceHints

I don't mind a breaking change and would prefer not to have an additional format.

We'd be protected from future changes to how timode=2 affects GeoJSON voice hints. Currently this is a rather crude way of triggering them

The timode parameter is the intended way for callers to set their default mode, so it should be rather stable. And while a specific flag or generic mode would be nicer, I don't think this is too crude, even if we don't have a specific format and any timode > 0 will do.

Therefore BRouter-Web should IMO unconditionally request voice hints at all times (even if they might get dropped later in some cases), which is also far cheaper than recalculation

That's the plan before releasing the FIT format and I would like to get that finally done before adding too much other stuff. I also expect it to be much cheaper, but that also depends on the number of route edits done, and I wanted to do at least a simple performance comparison, but haven't taken the time yet.

Implementation note 1: Extra data for comment-style hints would then need to be enabled by default.

I don't know what the comment-style is for and if anybody uses that, but I would simply exclude it, it could still be activated in the profile

Implementation note 2: timode=2 seems to be overridden if another turnInstructionMode is set in the profile (need to check).

Yes timode is just the default and overridden by any turnInstructionMode in the profile other than 1. But I don't see an issue with that, in the end it's the same and voice hints are set either way?

[rkflx]

Thanks for the comments, they are very helpful.

In general I prefer non-disruptive options, but value robustness and code quality too. As what might be accepted in BRouter still seems in flux and opinions in the discussion are changing daily, the scope and direction of my PRs is currently not as firm as I'd like, too. Sorry for that.

Let's see what we will end up with.

Another possibility would be to add this as an additional format=geojsonWithVoiceHints

I don't mind a breaking change and would prefer not to have an additional format.

I could have worded this less ambiguously: My idea was to add this to BRouter as an option (allowing for flexibility in clients regarding if and when they migrate). The plan for BRouter-Web would be to make a clean switch, i.e. not having both formats in parallel.

Would that work for you, or was your comment about only ever supporting a single format in BRouter too?

timode is just the default and overridden by any turnInstructionMode in the profile other than 1. But I don't see an issue with that, in the end it's the same and voice hints are set either way?

That's true for turnInstructionMode >= 1, but not for == 0, at least in my testing. Even if we chose to hide that combobox in the profile tab, users might just paste random profiles in there which could break our shiny future voice hints UI checkbox (and possibly a combobox for the voice hint style) in the export UI. Not a blocker, but something to keep in mind (might probably be fixable in BRouter).

[rkflx]

Rebased on master.

[rkflx]

Closing due to how refactoring patches in BRouter are handled. It's not worth the trouble.