for bike? Shouldn’t we always use a faster speed for racing bike? Same for path (10 vs 8), service (14 vs. 12) and track (12 vs. 2). And probably some more I overlooked.
Also path is added to the pushing_section list for racingbike but has not the pushing section speed and uses 8km/h instead.
It seems we mix the priority into the speed. I would prefer a separation (especially when we have more options with custom routing and for priority with issue 2333). Do you see a possibility to make this more consistent?
As far as I remember the lower speed limit for residential and service are attempts to reduce the number of artefacts where the route leaves highway=primary in favour of a highway=residential or highway=service for just a short segment because of higher priority.
On highway=path one needs to be extremely cautious with a racing bike, especially as I’m assuming that you do not have any bell. So I believe it is not unrealistic to estimate a slower speed compared to an ordinary bike, also because usually a path without more information probably goes along with a bad surface for a race bike. By the way, a simple highway=path results in PUSHING_SECTION_SPEED in the test for both bike and raceingbike, despite the speed set with setHighwaySpeed.
For the highway=track I intentionally used the lowest possible speed, as it is usually tagged together with a tracktype and if not we assume the worst condition for a race bike you can imagine. It is simply no fun riding there if you don’t have any information about the surface/tracktype. But for tracktype=grade1 we do use 20 km/h.
Summing up from my point of view up the inconsistency is only an issue for highway=residential or highway=service case. Maybe this can be solved when having issue 2333 by preferring highway=residential one step less as compared to what we currently do and by reducing the speed for highway=service to 16 for the bike profile.
Thanks for the explanation - this makes sense. Then I’ll create an issue to increase the speed for racingbike for residential roads and reducing the priority one step (plus reducing speed for service roads for normal bike and removing the explicit speed change in racing bike) (?)
It is simply no fun riding there if you don’t have any information about the surface/tracktype
I would argue that “fun” should go into priority … so even if a road is very dangerous and/or unpleasant we should try to keep the speed assumption separate from this. But I understand that you probably (have to) ride more cautious/slower with a racing bike if the surface is ugly.
I found that several cases regarding highway=path are inconsistent (leading to strange differences for e.g. bike and mtb) and will submit a PR shortly to propose a solution.
For the highway=track problem I can imagine that we do not need to add code that makes it more complex and instead we should streamline the current code.
But before I can do this I have some questions as I didn’t find a solution yet (for these problems in my dev branch):
We have two similar code sections where we handle tracktype and surface and both use different definitions of “is designated for bike” (see priority parser and speed parser). Do you know if there was a reason for this difference?
Furthermore the handling of tracktype and surface for track and bridleway is different to that of the “pushing sections” (a term which I’m going to change to slowByDefaultHighways). Why don’t we increase the speed of the “pushing sections” for good surface tags too? Could we add track and bridleway to the “pushing sections“ and have one place for all of them that also handles the surface and not only the tracktype? Or is there a reason we don’t do this?
Yes, we should have better covered the priority/fun aspect you pointed out. But I think that small improvements such as this will always be possible!
I intentionally wanted to minimize the effects of this change localized to the bike profile only as I think that we already covered it for mountainbike and race bike sufficiently.
I guess that nobody checked for consistency of this yet. When you change it we’ll see if and what tests break and need to decide if the changes make sense.
I have now code that is simpler but causes a few changes that needs to be reviewed. For now this just touches the speed; other inconsistencies will be fixed later&separately.
Note in certain separate commits I moved a few tests around. Walking through these commits separately makes it probably more obvious what changed. Or probably just this comparison, which excludes the first commit.
Can you have a look at it? Or maybe you can come up with even less code?
Also from the perspective of a “conceptual simple” approach to avoid having too many differences and that we basically have only two things that influence speed: 1. bike allowed/designated and 2. surface/tracktype/smoothness and the `highway` value basically says which of the default should be used. (elevation is another big factor, but that influence comes outside of the parser)
I think in master we do not properly attribute this. E.g. “path” is added to pushing section although the slow speed (by default) is more because the surface is unpaved.
Also note that the speed should not be mixed with the priority. E.g. for mtb many bad surfaces have an unrealistic high speed. This does not make much sense and is probably just because we did this in the past to influence priority but this is no longer necessary.
Also you can see there is one big TODO NOW because not only bicycle=designated should boost the speed for e.g. track but also a bike route relation should do the same.
update, some examples:
highway=path & surface=paved is now 12 and was 4km/h reasoning: we assume highway=path allows bicycle usage by default
highway=track & bicycle=designated & segregated=no is now 18 and was 12km/h reasoning: same behaviour like highway=path or do we have to assume a bad surface even for bicycle=designated?
highway=track & tracktype=grade1 is now only 12 and was 18km/h reasoning: track has no explicit tag for bicycle usage
default for mtb with highway=track is now also 12 reasoning: mtb shouldn’t have much different speeds for bad tracks
TODO: for racingbike highway=track & bicycle=designated speed of this branch is too high as no surface is specified (18)
For sake of easier commenting I reverted the unit tests, made changes to get them pass based on the new code and commented on all the changed results. See here.
I only had a short glimpse at the code changes so far. While I like the introduction of the case statement I am skeptical on the overall result. I gave the comments based on my experience on the various bicycle types.
Here are comments on the update
highway=path & surface=paved is now 12 and was 4km/h reasoning: we assume highway=path allows bicycle usage by default
We need to consider that ways tagged as such are ways in the mountain/forest. I remember inappropriate usage of such sections through the mountains from the early days. Therefore I consider 12 as much too high, maybe we might increase to 6.
highway=track & bicycle=designated & segregated=no is now 18 and was 12km/h reasoning: same behaviour like highway=path or do we have to assume a bad surface even for bicycle=designated?
I think that we need to assume bad surface there even if bicycle=designated
highway=track & tracktype=grade1 is now only 12 and was 18km/h reasoning: track has no explicit tag for bicycle usage
If a bicycle is allowed there we should assume the same speed compared to a residential as the surface is appropriate.
default for mtb with highway=track is now also 12 reasoning: mtb shouldn’t have much different speeds for bad tracks
The former 18 was probably a bit too high, but with 12 I’m missing the advantage of wide tires.
TODO: for racingbike highway=track & bicycle=designated speed of this branch is too high as no surface is specified (18)
I do not understand the argument. If the path is steep we consider the speed reduction already elsewhere. If the surface is paved why should we assume the speed is only walking speed? We should have less classification possibilities and maybe also less speed values like 6,12 and 18 km/h to simplify the reasoning. Please note again that the priority should not be considered here.
I think that we need to assume bad surface there even if bicycle=designated
Assuming this for racingbike seems to be reasonable but otherwise this does not make sense. Why should someone tag it with designated and leave the surface tag empty if the surface is bad?
If a bicycle is allowed there we should assume the same speed compared to a residential as the surface is appropriate.
the bicycle tag was unknown in this example. The question is: do we assume bicycle=yes or designated by default for highway=track? I think just bicycle=yes is more appropriated and then 12km/h makes more sense.
The former 18 was probably a bit too high, but with 12 I’m missing the advantage of wide tires.
I think it is unrealistic that the same person with different tires can have a dramatic different speed (18 instead of 12). I would doubt that it will be more than 1km/h difference. Furthermore a small change like 12→14 is not necessary (as it increases complexity for no real benefit).
I would really prefer to make the speed estimate for bike much simpler and just use a few differentiators like bike=no/yes/designated and surface=paved/unpaved (+ a few speeds for the different surfaces but not more). This way it is very easy to judge about a situation - at least regarding the speed. Let’s not forget that the main reason for different speed is the stamina and power of the driver and the elevation.
And what do you think about handling the route relations in the speed parser too? If there is e.g. a track with a route relation shouldn’t this at least qualify for a bicycle=yes (if no other access tags)?
and it returned like 50 places in Germany and I looked closer at 5 of them and all of them are endstanding roads leading to nowhere. IMO not worth the discussion if 12 or 18km/h is the right value. (And some of them had motor_vehicle = private which means the surface might not be that bad.)
Also this cleanup helped uncover some more inconsistencies. E.g. cycleway is 18 in master for racingbike and not 20 like the other bike-acessible and paved roads.
We need to consider that ways tagged as such are ways in the mountain/forest. I remember inappropriate usage of such sections through the mountains from the early days. Therefore I consider 12 as much too high, maybe we might increase to 6.
and look into several examples and if the surface is good we should not keep the speed at 6. See e.g. this way and this bike route where we avoid an IMO perfectly fine path because of that or this way and this bike route.
I agree that highway=path is an undefined ugly mess but if the surface tag is present we should not put such a low speed on it and make it more consistent with highway=track. Also have a look into the following test which shows some more speed inconsistencies (currently green in master):
Your overpass query convinced me here. I additionally made some checks in Africa, there it is even more rare.
Also this cleanup helped uncover some more inconsistencies. E.g. cycleway is 18 in master for racingbike and not 20 like the other bike-acessible and paved roads.
The is not an inconsistency, it was intended. Reasoning is imagine a residential and parallel a cycleway in a city. On the cycleway you need to be very careful with a race bike, there are crossings to be expected and slow bicycle traffic on the cycleway. Given the todo comment in the code I therefore suggest to keep the 18 km/h here.
Regarding the paved path, it is the most controversial highway type we have and there are multiple conflicting interpretations. 12 km/h is really high for it, but maybe we might consider the maxwidth encoded value at some point in the future if we get complaints.
I again checked all the results of the changed tests and only detected two further issues I want to address here:
This MTB test was 16 km/h before. Slowdown to 8 is not OK for MTB, it is too much. MTB is made for bad surface and you will definitely be faster compared to a trackingbike, where the same test results in 8 km/h.
resulted in 18 km/h before and now result in 12 km/h. Why did you change this? I would revert this change. I don’t see any reason why those shouldn’t give the same result as compared to a residential, they should be wide enough.
Hm, but this is a small differences and depends on the cycleway as well as the residential. Is there a real world case where this really matters?
At the end a cycleway should be assumed to be equally well and fast as any other highway. And again such a difference should not influence speed. Maybe priority (?) but I am still convinced that we need to increase granularity - at least for the speed - in order to get more consistent and reasonable routes, also between the bike types.
So the question is: do we really want that racingbike leaves the cycleway even if the residential is ~10% longer and even if it is just a few meters? We should accept such detours only if they have a good reason and IMO only surface/highway=* & bicycle=* make these differences easy to understand.
Slowdown to 8 is not OK for MTB
The problem with “ground” is (according to the wiki) that it is a very broad category. And furthermore path by default assumes bicycle=no. If we follow the logic “+6 for good surface” and “+6 for bicycle access”, then 8 makes sense as ground is not such a good surface.
But I can put “ground” on the same level as “grass”, then this is a bit faster (10) - see the changed code.
resulted in 18 km/h before and now result in 12 km/h.
Indeed, a very good surface and implied bicycle=yes should be faster than 12. Will have a look.
btw thanks for taking the time to discuss this with me !
Hm, for track I assumed bicycle=no in this branch by default (if no other tag is given) and so it will be surfaceSpeed * 0.7 = 18 * 0.7 ~= 12.
It is fixed if we assume bicycle=yes when the bicycle tag is missing. So this is basically a 3rd category “bicycle=yes & surface=unpaved by default”.
Which is different to path&bridleway (I assume bicycle=no & surface=unpaved by default) and also different to footway etc (I assume bicycle=no & surface=paved).
Regarding the discussion of how to handle track & bicycle=designated but without surfacetag: we can safely ignore this as it is rare, but the case path & bicycle=designated (without surface tag) seems to be very common and IMO here it is clear in most cases that a good surface or “inner city“ is assumed (at least in Germany). You can try this overpass query:
I’m sorry, I really don’t want to offend you: But did you ever use a race bike in any city area? To me it looks not. Believe me, on cycleways there you are objectively slower compared to staying on the road! You might be safer depending on the amount of traffic on the road, but you will be slower. Just ask a friend who has experience on a race bike. But maybe it might be that your concern comes because of the special German situation, where there is this general “Radwegbenützungpflicht”, which you don’t have anywhere else as far as I know.
So the question is: do we really want that racingbike leaves the cycleway even if the residential is ~10% longer and even if it is just a few meters? We should accept such detours only if they have a good reason and IMO only surface/highway=* & bicycle=* make these differences easy to understand.
I do not understand this concern because if it was an issue, then it would have been already reported. We always had this weighting for race bikes if I remember it correctly.
btw thanks for taking the time to discuss this with me !
Thank you for considering my pedantic concerns. I like your two commits from above .
But did you ever use a race bike in any city area? To me it looks not.
Yes, right, but I have similar ebike speed experience
I don’t think it makes sense for outer city, like for this case where racingbike is currently 30s faster on the secondary road. This is unrealistic as the cycleway is wide enough and even if it would be tight it does not matter as traffic is not that frequent so that even racing bikes prefer this over the car traffic.
So my argument is not that this could be partially wrong, but more against small differences that make no big difference anyway (and could introduce small detours). Everyone has many little requirements and our first aim should be to grasp the main differences.
So for racingbike I would describe it as “faster and without unpaved roads and less safety concerns“. So instead of +2km/h for cycleways we should discuss why racingbike is not +25km/h for the best case instead of only 20km/h
The following problem for mtb is one of the examples leading my need to simplify code and it was also not reported yet
The linked route shows how subtle differences can create unwanted little detours. In this case debugging was a bigger challenge. (At the end I found that we use an IMO strange and different default for mtb regarding route networks.) I’m not arguing against this specific case (at the moment ), but I’m arguing here that we need to simplify and focus on important differences.
And even once we are finished with simplifying speed and priority code I think we should not add complex code as there are many more important issues for bike routing to fix: better elevation, “prefer parks/greenery”, less turns, better turn instructions, avoid wrong side of road, more bike related encoded values, …
I fully agree here. I think that this low speed comes from the original implementation with the flag encoder method where it was not possible to increase the 4 bit spent for the storage of speed into 5 bit easily. And I wanted to have some have some room for higher speed values because of descent sections. Correct me if I’m wrong, but I think this limitation is not there any longer.
The linked route shows how subtle differences can create unwanted little detours. In this case debugging was a bigger challenge. (At the end I found that we use an IMO strange and different default for mtb regarding route networks.)
The priority issue looks strange indeed. It is caused by this inverse priority logic specific for MTB:
I must confess that I forgot the reason why we changed that. It was during the review of #2904 in this commit.
→ Did you already fix this? Or will this be included in another priority related change?
Unfortunately there is still a limitation as we use an encoded value (and parser) for the entire speed (which is not necessary but for historical reasons we do so for all vehicles). Still for bike we use 4 bits and a factor of 2 and so up to 32km/h should be possible by default.
Did you already fix this? Or will this be included in another priority related change?
Can you again review the branch? I just pushed some minor changes.
Let me know where you disagree
Ignoring the first two commits of the branch makes it easier to compare due to the test changes:
Is it possible to change that into usage of 5 bits with a factor of 2? If it is, should we change this for all vehicles or just bicycle?
For now I would keep it at 4 bits. The better approach for the parsers would be to move them into a custom model to avoid this limitation at all but let them us simplify first
Also should we increase from 20 to 24km/h? If yes, let’s do this in a separate PR afterwards.