Skip to content

Support OSM highway=razed tag - #2660

Merged
abyrd merged 2 commits into
opentripplanner:masterfrom
entur:fix-support-osm-highway-razed-tag
Dec 11, 2018
Merged

abyrd merged 2 commits into
opentripplanner:masterfrom
entur:fix-support-osm-highway-razed-tag

Conversation

@t2gran

@t2gran t2gran commented Oct 23, 2018

Copy link
Copy Markdown
Member
  • issue: Closes Support OSM "highway=razed" tag  #2659
  • roadmap: Bugfix not on roadmap.
  • tests: No tests added - sorry. There is a test TestUnroutable test the highway=construction tag(equivalent tag in the same list). See more on this below below.
  • formatting: Tried to make the code more readable.

Testing OSM filtering rules

The tag filtering logic happens in many places witch for me is very hard to understand. It needs refactoring, and a natural start to this, is to create a unit test with the contract/rules. But this is a big task. When I tried to fix this issue, I first added one line to the DefaultWayPropertySetSource: (props.setProperties("highway=razed", StreetTraversalPermission.NONE);). This did not have the expected result. I debugged this until I understood that there is no clear precedence between the rules - at least for me to grasp in a few hours. The OSMFilter#getPermissionsForEntity method might return not NONE even if the input parameter StreetTraversalPermission def is NONE. This is just a small pice of the puzzle, so I went on and fixed it another way (see code).

To be completed by @opentripplanner/plc:

  • reviews and approvals by 2 members, ideally from different organizations
  • before merging: add a bullet point to the changelog file with description and link to the linked issue
  • after merging: update the relevant card on the roadmap

@t2gran
t2gran requested a review from a team October 23, 2018 16:09
@abyrd

abyrd commented Nov 30, 2018

Copy link
Copy Markdown
Member

@t2gran the method you use here is the same way I've filtered out non-existing highways before, which don't belong in the graph at all. The WayPropertySet is more targeted at roads which will have different characteristics for different modes or directions.

Point taken though that it's not obvious how the WayPropertySet works. I asked the people at TriMet who fine tuned this, and they gave me an explanation a while back (maybe on the OTP mailing list). We should move that into the official documentation - but that's a separate issue.

@abyrd

abyrd commented Nov 30, 2018

Copy link
Copy Markdown
Member

@sdjacobs, @irees, or @fpurcell could you also give this an official review, even if based strictly on a read-through of the changed code? It's essentially a one-liner change with some formatting.

@sdjacobs sdjacobs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good!

@abyrd
abyrd merged commit 289dac3 into opentripplanner:master Dec 11, 2018
@t2gran
t2gran deleted the fix-support-osm-highway-razed-tag branch January 10, 2020 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants