How to handle backward incompatible change to voice hint indexing?

[rkflx]

c9ae7c8681 changed indexing of voice hints, e.g. it now has TRU = 12; // Right U-turn which was OFFR = 12; // Off route before, breaking everyone depending on those ids (see also nrenner/brouter-web#751).

As far as I can see those changes have not been approved in a review, neither is there any discussion in the commit message on why those changes are needed, nor any mention in the release notes or even tests to catch such regressions. This way it is hard to know what the original motivation for that re-indexing was.

Since the breakage already shipped in 1.7.0, there are now two options:

1. Revert the re-indexing and append new ids only at the end.

• This will restore backward compatibility somewhat, but there would still be a regression (i.e. an unknown hint) in old clients for TLU, which probably was emitted as TU or something else before.

• It will also require clients which already incorporated the changes to yet again adapt their code, possibly having to support pre-1.7.0, 1.7.0 and post-1.7.0.

2. Keep the changes as-is.

• This will require clients which did not yet adapt to be changed, which might be an issue for unmaintained projects.

• It might require clients to support two different indexing schemes in parallel, i.e. pre-1.7.0 and 1.7.0.

Do we know which projects are already aware of this or still need to adapt? There is BRouter-Web, fit-route, OsmAnd, Locus, QMapShack and probably many more...

Which option should we implement?

[afischerdev]

changes have not been approved in a review, neither is there any discussion

Yes, this was part of a larger PR #441, we splitted into smaller peaces. Please see for discussion:

Changes on turn instruction are mainly discussed in https://github.com/abrensch/brouter/issues/375. The main discussion for the lib 1.6.4 is found in https://github.com/abrensch/brouter/pull/441.

Locus, Cuiser was part of the discussion

Output PR is found here #510

OsmAnd was present there. The output of the lib/apk is still conform to older versions like OsmAnd, Orux, Gpsies.
Only json or comment-style export using clients need an update.

[rkflx]

changes have not been approved in a review, neither is there any discussion

Yes, this was part of a larger PR #441, we splitted into smaller peaces. Please see for discussion:

If you could provide a link to the specific comment where re-indexing was discussed, I would appreciate it.

(Of course I read through the whole discussion before posting this issue initially, but I might have missed something due to the length of it. That's why I am asking for clarification.)

[devemux86]

Cruiser with BRouter version:

• before 1.7 used the timode 3 (osmand-style) that has turn strings (not turn ids).
• after 1.7 uses the timode 2 (new locus-style) with the offline gpx navigation track also having turn strings.

[afischerdev]

If you could provide a link to the specific comment where re-indexing was discussed, I would appreciate it.

No, I couldn't. May be there was none. Anyway append new ids only at the end will not do the job in this case. There are different arrays with ids (or shortcuts) and when the first new id comes out the routing it will break.

Means for me you should change to the new ones or do a wrapper around.

[rkflx]

The output of the lib/apk is still conform to older versions like OsmAnd, Orux, Gpsies. Only json or comment-style export using clients need an update.

AFAIU only formatAsGeoJson() has a dependency on the actual ids (via getCommand()). It is also called from routingapp/BRouterWorker.java (see OUTPUT_FORMAT_JSON), so I am not sure what is meant by "still conform to older versions" when the changed ids end up there too. On the other hand, comment-style employs strings and should not be affected, so its users should not need indexing updates (apart from supporting newly added commands like BL and TLU).

IOW voice hints in clients using &format=geojson via BRouter's HTTP API or trackFormat=json via the BRouterService Java API got broken, unless the indexing change gets reverted. I have not yet checked whether OsmAnd or other apps are using that request format internally, but at least BRouter-Web is.

Anyway append new ids only at the end will not do the job in this case. There are different arrays with ids (or shortcuts) and when the first new id comes out the routing it will break.

Sorry to bother again, but I am not sure I understand: Why would routing break when the mapping of voice hint commands to voice hint ids changes? BL was appended, why was it necessary to squeeze in TLU instead of appending it too? Is there any external dependency which dictates the ids to use in class VoiceHint? If so, could it be documented in the code?

do a wrapper around

Do you mean we should implement the wrapper before emitting the voicehints GeoJSON array, or do you intend each and every single client depending on voice hint ids (some possibly unmaintained) to add a wrapper?

[afischerdev]

I am not sure what is meant by "still conform to older versions"

The output of BRouter for e.g. OsmAnd is still the same as before. It doesn't know a 180 degree turn, so this is mapped back to known TurnTypes. Please see OsmAnd git

I have not yet checked whether OsmAnd or other apps are using that request format

As far as I know, only the web client and AFTrack are using the json export.

Why would routing break when the mapping of voice hint commands to voice hint ids changes?

Not the routing will break, the calling client and its logic. The routing will bring out the new values.

new list	old list
x1	x1
x2	x2
x3	x3
x4
x5

When routing presents a value x4 this is not in the old list. You have to fill the gap.

Do you mean we should implement the wrapper

No, I don't mean that, it is not recommended in my eyes. I mean if you are not lucky with the new values, ...

[rkflx]

I'm sorry, but I still don't get why changing the ids of existing voice hints was necessary. I appreciate the answers you tried to give, I just don't feel we are talking about the same thing.

I'll keep this issue open for a day or two if anyone else has something to add, otherwise I'll close it and go ahead with option 2 as outlined in my first comment (breaking bikerouter.de in the process).

[afischerdev]

What I see is the function in Voicehint.js I don't know javascript that much, but I think you have to add two lines to become conform with VoiceHint.java, or something similar

            10: new Command('TLU', 13, 1003, 'TU', 'u-turn', 'u-turn'), // Left U-turn
            11: new Command('TU', 13, 1003, 'TU', 'u_turn', 'u-turn'), // 180 degree U-turn
            12: new Command('TRU', 14, 1003, 'TU', 'u_turn', 'u-turn'), // Right U-turn
            13: new Command('OFFR', undefined, undefined, undefined, 'danger', undefined), // Off route
            14: new Command('RNDB', 26, 1008, 'RNDB', 'generic', 'Take exit '), // Roundabout
            15: new Command('RNLB', 26, 1008, 'RNLB', 'generic', 'Take exit '), // Roundabout left
            16: new Command('BL', undefined, undefined, undefined, 'beeline', undefined),  // beeline

Anyway when you dealing with own gpx export you also have add a new Locus export (voicehints on trkpt)

[rkflx]

you have to add two lines

That's not what your example is showing, as it changes the ids of existing lines besides adding more lines. That's exactly what this issue is about: Not breaking compatibility with older BRouter releases and also not breaking existing code.

I'm running BRouter-Web with a similar patch since months. I'll probably have to push it upstream, but ideally I'd prefer not to, since the actual issue is in BRouter. If you don't want to change it (or accept a PR to that effect), could you at least detail why swapping command ids in the GeoJSON API was needed?

10: new Command('TLU'

I'm afraid that's not quite correct, it should read TU to be compliant with BRouter's output.

when you dealing with own gpx export you also have add a new Locus export

I am aware of that, but thanks for the reminder. However, "adding" in the sense of backward compatibility is not the right wording here. The turnInstructionMode of Locus was changed from 2 to 7 (backward incompatible change), and the output for 2 was changed to something else (new-style Locus), breaking compatibility another time. Anyway, that's off-topic here.

[afischerdev]

Ok, lets go it your way, step by step:

The task is add a 180 degree u-turn

We have this ids. If you don't mind I ignore beeline for a moment, it is not relevant here. What we had in the past:

 10: TU
 11: TRU
 12: OFFR
 13: RNDB
 14: RNLB

This brings as output of your little sample the voicehints:

forward 10 - TU
reverse 11 - TRU

Now you want to add a new line in the back - I hope I understand it the right way:

 10: TU
 11: TRU
 12: OFFR
 13: RNDB
 14: RNLB
 15: TLU

But, ups, the challenge was to add a 180degree U-turn and not a Left U-turn. And our output now looks like:

forward 15 - TLU
reverse 11 - TRU

Shouldn't it be 10 - TLU? To get that we need a this list

 10: TLU
 11: TRU
 12: OFFR
 13: RNDB
 14: RNLB
 15: TU

Now we have a perfect output and added a 180degree u-turn at the end. But we got changes in the old running team.

Every path ends in a mess. The problem is: in the past the number 10: TU wasn't a 180degree u-turn it always was a left u-turn

That's why I still think it is the better way to make a break and keep the u-turns together. It beautifies your code and makes it more readable / understandable for other people.

And I understand you problem. You are using hard coded ids - e.g for RNDB, RNLB. I don't know if this is the way Javascript needs. In Java I can use constants which makes changes easier.

[quaelnix]

Could someone please explain me why we need three different types of u turns?

[afischerdev]

@quaelnix Please have a look here #436.

[quaelnix]

Ok thanks, here is the answer to my question: https://github.com/abrensch/brouter/issues/436#issuecomment-1159795615

[rkflx]

Shouldn't it be 10 - TLU?

While I find it questionable that you rename TU to TLU, this won't affect the GeoJSON API, since both before and after the change 10 is emitted, both times meaning u-turn-left. That's okay, but not the topic of this issue.

However, as mentioned in my very first comment, TRU is where it breaks, where off-route would be shown by clients. That's not okay, but surprisingly your example makes it look as if it was unaffected.

I don't even want to think about what happens when the "should be changed to u-turn-left when osmand uses new voice hint constants"-comment in BRouter's code comes into fruition. It will be another mess for sure, when multiple old and new versions of BRouter, BRouter-Web and OsmAnd are interacting with each other.

You are using hard coded ids - e.g for RNDB, RNLB. I don't know if this is the way Javascript needs.

You must be joking, right? Of course clients could simply use strings via JavaScript. It is BRouter's GeoJSON API which is forcing ids upon them.

---

To me it looks like you want to:

1. Introduce the new concept of u-turn-180, with u-turn-left and u-turn-right already existing. – Not sure whether that's really needed (marking errors in the route this way is a bit of a strange workaround), but whatever.
2. You have to find a string for that new turn. Instead of inventing something new like T180, you think TU looks nice and proceed to steal it from u-turn-left. – Now clients got a problem, because the meaning of TU has silently changed.
3. Next, u-turn-left is without string. You think it might be nice for it to look similar to TRU, so TLU it is. – A new string which clients know nothing about, while they could show u-turn-left just fine before.
4. You believe TLU, TU and TRU should be ordered numerically one after the other for beauty, so you assign 10, 11 and 12. – This completely ignores that those numbers are used in the API to represent meaning, e.g. 12 now means u-turn-right, while before it meant off-road.

This is what you achieved:

• Clients using strings will be confused by the changed meaning of TU.
• Clients using ids will be confused by the changed meaning of everything after 9.
• The code looks nicely ordered.

What you should have done (assuming u-turn-180 is really needed, which I am not convinced about):

1. Introduce the concept of u-turn-180.
2. Assign a string (e.g. T180) and an id (e.g. 15).
3. Don't change anything else.

This would have achieved:

• Clients have a new concept at their disposal they might want to implement, but don't have to in case they handle unknown hints reasonably well or are unmaintained.
• The code still looks fine, and everbody will perfectly understand TU is called like that for historical reasons and should not be changed to keep compatibility. After all, TU and TRU were co-existing even before T180 came along, so nothing new really.
• Everbody is happy.

---

FINAL QUESTIONS:

Would you accept a PR where I completely eliminate the concept of numerical ids for voice hints in the codebase?

Obviously that would needs clients using the GeoJSON/JSON APIs to adapt, but since they would need to be changed anyway due to the re-indexing, it would not be much more work for them (I can do it for BRouter-Web), and they could then also benefit from "constants" like you are describing as being superior in Java (rightfully so!). In addition, you could order the constant in the code any way you'd like (randomly, alphabetically, by type, by length etc.), and it won't affect the changed meaning of TU.

Patches to achieve that are already done for BRouter as well as for BRouter-Web.

Alternatively, would you accept a PR where I revert the re-indexing of ids in the API? (This also won't affect the changed meaning of TU.)

[afischerdev]

Alright, I give up. I jumped too short and in the wrong direction. But I would have liked a correction a little earlier.

I would prefer option one. A little overhead, but smart.

If no one else has another vote, you could start a PR.

[quaelnix]

Is it really too late to go back and do it as described in the "What you should have done ..." section?

[rkflx]

Thank you for your patience.

I'll prepare a PR in the next days, so we will be able to make an informed decision on how to go forward with this.

[afischerdev]

No, I think it is no to late. As this issue shows no other has a problem with the new version. Or they are all sleeping. The rewind could go this way:

 10: TU
 11: TRU
 12: OFFR
 13: RNDB
 14: RNLB
 15: T180

[devemux86]

15: T180

This will break compatibility in apps using BRouter 1.7.X with the already published TLU tag.

At least we should keep the current strings.

[afischerdev]

@devemux86 This are the input values and should not affect the current output. Only json export will get other output values and comment-style may be other labels.

[devemux86]

@afischerdev The turn strings like the "TLU" are used in the timode=3 answer, so it affects the output.

Cruiser already uses the new timode=2, so it is not affected.

But maybe other apps using timode=3 will have a problem?

[afischerdev]

@devemux86 I'm not sure how OsmAnd ir running. The issue is still open and the voicehint turntype are the old one. I think I already have mapped it to get the old export, and only the 'getCruiserCommandString' function has the new output.

[quaelnix]

This will break compatibility in apps using BRouter 1.7.X with the already published TLU tag.

Yes, but these are the unproblematic applications that are well maintained. If we announce the rollback properly, it should go pretty smoothly. And the many not well maintained apps (which did not add support for the new logic yet) would no longer need to be changed at all. But I also agree with all the solutions proposed by @rkflx. I'm just thinking out loud.

[afischerdev]

@rkflx

I'll prepare a PR in the next days, so we will be able to make an informed decision on how to go forward with this.

Maybe wait a little longer, perhaps there will be more opinions on these changes

I see three options:

• stay with id export on json, re-index and offer ids as above
• switch to 'named' export on json, do not re-index
• switch to 'named' export on json and use the new Command class offered here BRouter

[afischerdev]

@quaelnix

And the many not well maintained apps (which did not add support for the new logic yet) would no longer need to be changed at all.

What are the not well maintained apps that are using json export? And I'm not sure about no longer need to be changed at all. When new re-indexed BRouter deliver T180 or BL they are unprepared and in the best way they return nothing.

All options above should only affect the json export, all other candidates should remain as before.

[quaelnix]

What are the not well maintained apps that are using json export?

I don't know, but I guess the "don't know" part is the whole reason why full backwards compatibility is desirable?

[rkflx]

@afischerdev

As this issue shows no other has a problem with the new version.

What are the not well maintained apps that are using json export?

As reiterated by @quaelnix, the point of an API is that we do not know and do not have to know. The API is a promise, and BRouter should stick to it. In addition, as I mentioned before, such API breakage would make things difficult for services like bikerouter.de or basically anyone choosing to mix-and-match between different version of BRouter and web as well as app clients.

I would have liked a correction a little earlier.

Or they are all sleeping.

Please keep in mind people have work, family and other commitments and might not be able communicate as timely or in depth as you or even they themselves might wish to.

they are unprepared and in the best way they return nothing

Keeping backward compatibility does not mean not adding new stuff. It means not breaking existing functionality. Obviously old clients will not support newly added commands (besides a generic hint, perhaps), but at least they won't break.

I'll prepare a PR in the next days, so we will be able to make an informed decision on how to go forward with this.

Maybe wait a little longer, perhaps there will be more opinions on these changes

What I meant was to continue discussion based on an actual PR vs. in easily misunderstood comments like here. This is not to exclude anyone from chiming in.

switch to 'named' export on json, do not re-index

If we emitted strings in the GeoJSON/JSON API (the only part of the codebase currently exposing ids externally), any internal indexing would become pointless anyway.

@quaelnix

I'm just thinking out loud.

Your contributions here and elsewhere to prevent mistakes from slipping in are very much appreciated, thank you.

---

See #591 for PR reverting the reindexing, with optional cleanup.

[afischerdev]

See #591 for PR reverting the reindexing, with optional cleanup.

I've seen, please let me do some tests. As additional test I use this.