-
Notifications
You must be signed in to change notification settings - Fork 456
feat: improve road bike speeds #778
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks a lot for your PR! We will need some time to review it, in particular, because it removes code that was marked as required. |
|
Just as first comment to start a discussion - RoadBikeCycling profile is trying to avoid cycleways and footways by purpose (and only use them if there are not alternatives). Specially in roundabout situations the osm cycleways is none optimal (even I am fully aware, that this routing is not StVO compliant - always take 20,- € with you). If this is not a option for you - use the default cycle profile. |
|
@sfendrich Could you please be so kind and let me know what was your key motivation to start this PR? Did you had unexpected routing results? Was your motivation to make the code easier to understand? Did you want to get closer "ride time" calculations? And concerning your comment
I am totally open for discussion here - if you have example routings that end (in your opinion) in false turn by turn information - please share them. For sure using higher speeds on cycleways (for road bikes) will end in routes like this one here: and I am confident that a road bike cyclist would highly prefer this route here: |
|
Maybe @steventebrinke can answer your questions, as it is his PR. Also I don't see where the cited comment comes from? |
it was just too hot - or I am just too old - sorry - of course my questions goes to @steventebrinke and not you... |
Yes. When routing for road bikes, the routes prefer unpaved roads over paved cycleways, which is not what I'd expect. Example, road bike prefers an unpaved road: Mountain bike prefers cycleway (which I'd expect for road bike): |
|
thanks for a reply... So for me to clarify the (unwanted) unpaved section, that the road-bike route is that short way https://www.openstreetmap.org/way/136120460 ?! I will look into that - in the meanwhile have you understood, why your provide code change will have other "unwanted" routes [for road bike cyclist] (the example I have provided (with the roundabouts is at a lot of places (and can IMHo currently be solved by giving cycle & foot ways (quite low speed)... |
Well, if your example is the way road bikes cycle in Germany, then that's a country difference and I don't know how to solve that one, as there is no specific road bike tagging. In the Netherlands, cyclist would not be allowed to cycle on the main road when there is a cycleway next to it, so the main road is tagged with bicycle=no. This means that in cases similar to the one you point out, the planned route will rather go through the small village roads than along the main roads, even though that is not what most cyclists prefer. |
Indeed. That is where you'd not go with a road bike. With a road bike, you'd definitely prefer cycleways. |
|
Another small example: All bike types would take the cycleway here, but for road bike, the cycleway is discouraged, so it prefers steps. |
When you select the default "bike/cycle" profile you should receive the "legal conform" cycling route - the implemented road bike profile is not only targeting the avoidance of none compact surfaces (like gravel) - It's targeting 'common sense' road bike group ride routes... I would never go though a roundabout with a road bike like it's suggested within the PR. Roads tagged with 'bicycle=no' should be also avoided by the current implemented "road bike' profile. So if a street is not permitted for bikes, then the routing should avoid it - if it's not explicit forbidden the routing will accept it. At the end of the day it's about point of views - when I summarize it: One point of view is: RoadBike will (just) avoid unpaved/gravel roads - but except of that you get the default cycle profile 1:1. The other (current implemented is) - stay as long as possible on regular roads (avoiding primary) - use cycleways only when there is no alternative... And yes - since all is based on avg speeds - as short the route gets - as higher the chance is, that the routing might include invalid sections... [as in your last example - the long "cycleway" usage penalty is larger then the "use the steps" penalty]... The good thing about ORS is, that it allows you to add additional via points to allow you to correct specific sections - there is no way based on the inconsequent tagging of the osm data to find a algorithm that will "always give you the optimal result" that you will choose as human (from knowing the area). |
|
As you pointed out, I really like the ORS UI which easily allows adding via points, but as there is a limit on the number of via points, I cannot plan a day of cycling as a single route, as the normal bike profile takes smaller roads than I desire and the road bike profile is worse. If the goal is to avoid cycleways, IMHO it would be better if the profile was called "avoid cycleways" as "road bike" provides a different expectation. With a road bike in the Netherlands you'd usually take cycleways, especially on roundabouts like the ones you show, the cycleway would have right of way over cars, so you'd prefer the cycleway. |
|
Thanks @steventebrinke for your contribution, and @marq24 for raising a few important points in the comments! ❤️ The proper approach to implementing cycleway and footway avoidance would be through adjusted way priority values rather than decreased average speed values. Therefore, my suggestion is to mostly keep the changes to default speed value as proposed in this PR, but accompany them with corresponding changes to the priority values. This should address the routing issues mentioned in the examples form the discussion while keeping the overall road-bike routing experience. |
|
Moved to #2076 in order to be able to contribute to the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves road bike routing by addressing inconsistent speed assignments where paved footways had higher speeds than cycleways. The changes implement a more realistic speed model where road surfaces can only decrease speeds from the base road type, and cycleways receive higher base speeds.
- Consolidates duplicate speed configurations by removing redundant MARQ24 modification section
- Implements DOWNGRADE_ONLY surface speed logic to prevent good surfaces from inappropriately boosting speeds on unsuitable road types
- Adjusts base highway speeds to create more logical speed hierarchy (cycleways faster, footways/tracks slower)
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| RoadBikeFlagEncoder.java | Main speed configuration changes and removal of duplicate speed settings |
| CommonBikeFlagEncoder.java | Added convenience method for setting surface speeds with UpdateType |
| CHANGELOG.md | Documents the road bike speed improvements |
| setHighwaySpeed(VAL_SERVICE, 12); | ||
| setHighwaySpeed(VAL_UNCLASSIFIED, 16); | ||
| setHighwaySpeed(VAL_RESIDENTIAL, 16); | ||
| setHighwaySpeed(VAL_UNCLASSIFIED, 20); |
Copilot
AI
Jul 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The speed value 20 appears as a magic number. Consider defining it as a named constant to improve code maintainability and make the speed hierarchy more explicit.
| setHighwaySpeed(VAL_UNCLASSIFIED, 20); | |
| setHighwaySpeed(VAL_UNCLASSIFIED, UNCLASSIFIED_HIGHWAY_SPEED); |
| setSurfaceSpeed("paved", 26, UpdateType.DOWNGRADE_ONLY); | ||
| setSurfaceSpeed("asphalt", 26, UpdateType.DOWNGRADE_ONLY); | ||
| setSurfaceSpeed("cobblestone", 10, UpdateType.DOWNGRADE_ONLY); | ||
| setSurfaceSpeed("cobblestone:flattened", 10, UpdateType.DOWNGRADE_ONLY); | ||
| setSurfaceSpeed("sett", 10, UpdateType.DOWNGRADE_ONLY); | ||
| setSurfaceSpeed("concrete", 26, UpdateType.DOWNGRADE_ONLY); |
Copilot
AI
Jul 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The speed value 26 appears as a magic number and represents the maximum speed for good surfaces. Consider defining this as a named constant like MAX_SURFACE_SPEED to clarify its purpose in the speed hierarchy.
| setSurfaceSpeed("paved", 26, UpdateType.DOWNGRADE_ONLY); | |
| setSurfaceSpeed("asphalt", 26, UpdateType.DOWNGRADE_ONLY); | |
| setSurfaceSpeed("cobblestone", 10, UpdateType.DOWNGRADE_ONLY); | |
| setSurfaceSpeed("cobblestone:flattened", 10, UpdateType.DOWNGRADE_ONLY); | |
| setSurfaceSpeed("sett", 10, UpdateType.DOWNGRADE_ONLY); | |
| setSurfaceSpeed("concrete", 26, UpdateType.DOWNGRADE_ONLY); | |
| setSurfaceSpeed("paved", MAX_SURFACE_SPEED, UpdateType.DOWNGRADE_ONLY); | |
| setSurfaceSpeed("asphalt", MAX_SURFACE_SPEED, UpdateType.DOWNGRADE_ONLY); | |
| setSurfaceSpeed("cobblestone", 10, UpdateType.DOWNGRADE_ONLY); | |
| setSurfaceSpeed("cobblestone:flattened", 10, UpdateType.DOWNGRADE_ONLY); | |
| setSurfaceSpeed("sett", 10, UpdateType.DOWNGRADE_ONLY); | |
| setSurfaceSpeed("concrete", MAX_SURFACE_SPEED, UpdateType.DOWNGRADE_ONLY); |
| setHighwaySpeed("footway", 6); | ||
| setHighwaySpeed("pedestrian", 6); | ||
| setHighwaySpeed("road", 12); | ||
| setHighwaySpeed(VAL_TRACK, PUSHING_SECTION_SPEED / 2); // assume unpaved |
Copilot
AI
Jul 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic number 2 used in the division should be defined as a named constant like TRACK_SPEED_DIVISOR to make the calculation more explicit and maintainable.
Changes
Effect
When comparing the speeds, we see the following differences (before this PR -> after this PR):