Skip to content

Slightly increase bicycle priority classification on "good" highway=track#3022

Merged
karussell merged 2 commits intographhopper:masterfrom
ratrun:parallel_cycle_track_priority_improvement
Jun 21, 2024
Merged

Slightly increase bicycle priority classification on "good" highway=track#3022
karussell merged 2 commits intographhopper:masterfrom
ratrun:parallel_cycle_track_priority_improvement

Conversation

@ratrun
Copy link
Contributor

@ratrun ratrun commented Jun 14, 2024

Triggered by the discussion in the forum about cycleway-like infrastructure along a highway=secondary I created this PR, which slightly increases the bicycle priority classification on highway=track with good surface.

Here is the link I used for verification that the changes improve the current route.

Copy link
Member

@karussell karussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! See my comments.

@karussell karussell added this to the 10.0 milestone Jun 21, 2024
@karussell karussell merged commit f280340 into graphhopper:master Jun 21, 2024
@karussell
Copy link
Member

karussell commented Jun 22, 2024

Unfortunately it is not yet fully fixed. See this route.

This way with highway=track, bicycle=designated, foot=designated, segregated=no, smoothness=good, surface=sett, tracktype=grade1
vs
this way with highway=secondary, maxspeed=50, surface=asphalt.

The reason seems to be surface=sett leading to a speed of only 10km/h but with smoothness=good this should not happen. But IMO it is a bit tricky to decide between the three tags (tracktype, surface, smoothness) or mix them somehow together. Maybe we should use the surfaceSpeed only if smoothness == Smoothness.MISSING? Or maybe we should always prefer the tracktype speed if it exists for highway=track instead of surface?

Also this route is a bit strange, but probably this is because of this way with highway=path only but maybe should be preferred over highway=secondary when surface=asphalt? Currently it seems to be intended to be a pushing section:

// use pushing section
way.clearTags();
way.setTag("highway", "path");
way.setTag("surface", "paved");
assertPriorityAndSpeed(SLIGHT_AVOID, PUSHING_SECTION_SPEED, way);

@ratrun
Copy link
Contributor Author

ratrun commented Jun 23, 2024

The reason seems to be surface=sett leading to a speed of only 10km/h but with smoothness=good this should not happen.

I intentionally didn't change anything for the first case yet. Here is what we are talking about: See https://wiki.openstreetmap.org/wiki/DE:Tag:surface%3Dsett . From my experience such surface is annoying and you are slow, even with a trekking bike. And I also think that it is OK to assume a default smoothness factor of setSmoothnessSpeedFactor(Smoothness.GOOD, 1.0d);. But maybe the 10 km/h is too pessimistic, we could simply change it to 12. I just verified that in the provided example it would give the intended result.

The second example it is OK from my perspective as we need to assume that one is not allowed to cycle there.

@karussell
Copy link
Member

karussell commented Jul 6, 2024

I just verified that in the provided example it would give the intended result.

Thanks for trying this out. We can surely change that.

Additionally we should think about the surface/tracktype/smoothness handling and making it more consistent. E.g. surface can only decrease the speed but tracktype will increase it. Maybe we set a higher default speed for highway=track and lower the speed only (i.e. the same procedure as for surface)?

Also it seems tracktype is also used for many other highway tags not just track. So maybe we should always refer to surface and if it does not exist we should use the tracktype speed? As increasing the speed is probably not a good solution for other highway types we have to fix this before allowing tracktype for other highway types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants