-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Remove banDiscouragedCycling and banDiscouragedWalking #5341
Remove banDiscouragedCycling and banDiscouragedWalking #5341
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #5341 +/- ##
=============================================
+ Coverage 66.35% 66.38% +0.02%
- Complexity 15182 15187 +5
=============================================
Files 1787 1787
Lines 69263 69241 -22
Branches 7339 7337 -2
=============================================
+ Hits 45957 45963 +6
+ Misses 20825 20796 -29
- Partials 2481 2482 +1
☔ View full report in Codecov by Sentry. |
void footDiscouraged() { | ||
var regular = WayTestData.pedestrianTunnel(); | ||
var props = wps.getDataForWay(regular); | ||
assertEquals(PEDESTRIAN_AND_BICYCLE, props.getPermission()); |
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.
It is a bit surprising that highway=footway gets BICYCLE permission by default, even when bicycle=no is set. Anyway, that issue is out of the scope of this 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.
Where do you see bicycle=no
?
Lines 24 to 34 in 3d17964
public static OSMWithTags pedestrianTunnel() { | |
// https://www.openstreetmap.org/way/127288293 | |
OSMWithTags tunnel = new OSMWithTags(); | |
tunnel.addTag("highway", "footway"); | |
tunnel.addTag("indoor", "yes"); | |
tunnel.addTag("layer", "-1"); | |
tunnel.addTag("lit", "yes"); | |
tunnel.addTag("name", "Lamar Tunnel"); | |
tunnel.addTag("tunnel", "yes"); | |
return tunnel; | |
} |
However, highway=footway
should probably not allow bicycles: https://wiki.openstreetmap.org/wiki/Tag:highway=footway?uselang=en
We should indeed change it in a separate 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.
I tested adding bicycle=no myself, just for curiosity :) It did not change anything.
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.
I believe that it's this piece of code taking care of it:
OpenTripPlanner/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java
Lines 250 to 256 in 73d873b
StreetTraversalPermission permissions = OsmFilter.getPermissionsForWay( | |
way, | |
wayData.getPermission(), | |
params.banDiscouragedWalking(), | |
params.banDiscouragedBiking(), | |
issueStore | |
); |
I would be in favour of moving that somehow together with the other code figuring out the permissions.
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.
I started working on that, and also fixing some known errors in traversal permission processing.
public void addTag(String key, String value) { | ||
if (key == null || value == null) return; | ||
public OSMWithTags addTag(String key, String value) { | ||
if (key == null || value == null) return this; |
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.
Can you split the return to its own line or at least wrap it with {}
public void addTag(String key, String value) { | ||
if (key == null || value == null) return; | ||
public OSMWithTags addTag(String key, String value) { | ||
if (key == null || value == null) return this; | ||
|
||
if (tags == null) tags = new HashMap<>(); |
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.
Same here
props.setMixinProperties("foot=discouraged", ofWalkSafety(3)); | ||
props.setMixinProperties("bicycle=discouraged", ofBicycleSafety(3)); |
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.
I checked that this wasn't handled in the NorwayMapper either and it doesn't rely on the default mapper. I can ask if the norwegians have interest in this too.
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.
Sure, you can ask but I see very, very few instances of this tag in the Nordics: https://taginfo.openstreetmap.org/tags/bicycle=discouraged#map
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.
I asked Entur if they needed this change in norway mapper as well but since the tagging is rarely ever used it's not needed.
Summary
This PR removes the configuration options
banDiscouragedWalking
andbanDiscouragedCycling
and replaces them with safety factors in theDefaultMapper
.This is more to the spirit of the tag which means that these ways are avoided if possible.
Unit tests
Added.
Documentation
Automatically generated.