Skip to content
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

Support OSM highway=razed tag #2660

Merged
merged 2 commits into from Dec 11, 2018

Conversation

t2gran
Copy link
Member

@t2gran t2gran commented Oct 23, 2018

  • 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 as a code owner October 23, 2018 16:09
@abyrd
Copy link
Member

abyrd commented Nov 30, 2018

@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
Copy link
Member

abyrd commented Nov 30, 2018

@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.

Copy link
Contributor

@sdjacobs sdjacobs left a comment

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.

None yet

3 participants