Continuing the discussion from here related to the priority differences for the bike routing network.
Did you already fix this? Or will this be included in another priority related change?
No. We need to discuss first why there is a change for the routing networks necessary between the bike types because at the moment I would just remove the difference
I could somewhat understand why we would prefer bike networks for bike compared to racingbike but why are we ignoring e.g. local route networks for racingbike and why should mtb be different from bike?
Given the priority inversion you detected for mountainbike as identified and commented I agree with your proposed change to remove this difference.
For race bike the most important thing is that network routes with bad surface do not get boosted in priority. I verified that following additional testcase in RacingBikeTagParserTest::testHandleWayTagsInfluencedByRelation :
@karussell It looks as this discussion has suddenly stalled for no reason. I expected it to become part of release 11.0, but it didn’t. What are your plans in terms of merging the speed code simplification and this priority improvements for the bicycle profiles?
I’d like to finish the average speed parser branch first. Will prepare a new branch for this here afterwards. But probably we should fix the mtb priority inconsistency already, will create a PR for this soonish.
Now that the speed problems&inconsistency between the different bike types are mostly fixed and simplified I would like to continue the work for the priority.
reduce code in master for BikecommonPriorityParser
Although this way is part of an official bike route it is not used (OSRM uses it). But likely it is because of pebblesone or segregated=no or just because highway=path?
what about scenic=yes and speed hack? I would prefer to completely remove it. Shouldn’t we handle scenic like the best possible option? Like a cycleway or bicycle=designated?
Will start with some simplification and let you know
And maybe we should only “avoid” ways instead of “avoid & prefer” them? This would also make reasoning simpler and e.g. “an official bike route with a good surface” is priority==1?
Maybe the best way to start the discussion is to think about what we should do with the PriorityCode enum.
One requirement for such a big re-structuring is that we should be able to utilize as many values as possible. Such that we actually utilize the available 16 values.
I must confess that I do not yet understand work items 2., 3., 4, and 7. Can you please provide more details here?
Regarding 2. (could be the same issue as 3.). the problem is that we overwrite a potential better PriorityCode with the PriorityCode from class:bicycle leading to strange routing in the example shown (it should prefer the official bike route instead a bit eastwards). Will create a PR shortly.
Furthermore we do not use the correct equivalent PriorityCode for a certain value of class:bicycle I would suggest something like:
private PriorityCode convertClassValueToPriority(String tagvalue) {
try {
return switch (Integer.parseInt(tagvalue)) {
case 3 -> BEST;
case 2 -> VERY_NICE;
case 1 -> PREFER;
case -1 -> AVOID;
case -2 -> BAD;
case -3 -> REACH_DESTINATION;
default -> UNCHANGED;
};
} catch (NumberFormatException e) {
return UNCHANGED;
}
}
The one example is invalid (or more a data or real world issue) as e.g. brouter shows the same result. But for the other example route we avoid the official bike route:
Unfortunately even my bug fix still leads to the same route. Likely because 12km/h is too slow compared to the 18km/h on the current route and the priority cannot correct it.
Boosting the priority with bike_network in the custom model (instead of in the parser) works and would be a solution for this problem - we already do the same for hike.json. See this branch.
But all these bug fixes are IMO insufficient to simplify the priority parser. Will think about the necessary steps.
Based on my first cleanup branch for the priority parser I have now created a second one that tries simplify the priority parser in the following steps - with separate commits (with green tests) for every step:
move ferry avoidance into custom model
replaces PriorityCode with just a double (allows us to use all the 16 values and IMO also makes reading easier when details are required, because it is very likely that you need to look up the exact value)
removes the weightToPrioMap (this required some logic changes but I was able to keep most tests identical)
the following 3 cases should give prio=1.3: highway=*&cycleway=track AND highway=cycleway AND highway=path&surface=asphalt&bicycle=designated
further simplifies the code to get a single if-else-if block for a simpler reasoning
minor alignment of racingbike code required to get same test response
Do you have comments on the changes?
Do you have suggestions on the structure of the if-else-if block - maybe you see some further simplification?
(Probably I should move the commit where I moved the bike_network out of the priority parser into the simplify_priority branch as it is not just “cleaning up“)
