brouter: How to handle backward incompatible change to voice hint indexing?

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:

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.

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?

About this issue

Original URL
State: closed
Created a year ago
Comments: 28 (3 by maintainers)

Most upvoted comments

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:

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.
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.
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.
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):

Introduce the concept of u-turn-180.
Assign a string (e.g. T180) and an id (e.g. 15).
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.)

rkflx on Jul 12, 2023

@quaelnix Please have a look here #436.

afischerdev on Jul 12, 2023