Skip to content

German way property set and improvements to bicycle routes - #3405

Merged
t2gran merged 28 commits into
opentripplanner:dev-2.xfrom
mfdz:german-way-properties
Apr 22, 2021
Merged

German way property set and improvements to bicycle routes#3405
t2gran merged 28 commits into
opentripplanner:dev-2.xfrom
mfdz:german-way-properties

Conversation

@leonardehrenfried

@leonardehrenfried leonardehrenfried commented Apr 7, 2021

Copy link
Copy Markdown
Member

Summary

This PR improves the cycling algorithm by making the following improvements:

  • it applies the tags lcn, rcn, ncn from OSM cycling network relations to the relation's members
  • since ways can be on all of these networks at the same time a logical OR matcher was introduced for the way property sets
  • a German way property set was introduced that fine-tunes the routing for Germany but also takes advantage of the above code
  • a more realistic turn cost model for bicycles that avoids left turns
  • makes the car speed parsing more robust and allow single digit speeds

Issue

closes #3397
closes #3398
closes #3402

Unit tests

There are a tests covering the way property set and the logical OR.

I've also added a new test for applying the bike route relation tags to the ways.

Code style

Documentation

I've added the way property set to the documentation and updated the documentation about how to prepare OSM data to include bicycle routes.

Changelog

@leonardehrenfried
leonardehrenfried requested a review from a team April 7, 2021 20:28
@leonardehrenfried
leonardehrenfried force-pushed the german-way-properties branch 4 times, most recently from fca4368 to 0c87cc6 Compare April 8, 2021 08:23
@optionsome

Copy link
Copy Markdown
Member

Haven't taken a deep look at this but is "a more realistic turn cost model for bicycles that avoids left turns" still a valid concern in countries that use left-hand traffic?

@t2gran t2gran added this to the 2.1 milestone Apr 8, 2021
@t2gran t2gran added the !Improvement A functional improvement or micro feature label Apr 8, 2021
@leonardehrenfried

Copy link
Copy Markdown
Member Author

This exact question came up in the dev meeting and I will make it respect left hand traffic.

@@ -101,12 +108,25 @@ private double computeCyclingTraversalCost(IntersectionVertex v, StreetEdge from
var turnAngle = calculateTurnAngle(from, to, request);

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.

RoutingRequest also contains a field for the driving direction (driveOnRight), which is used by calculateTurnAngle(). The code here seems to work independently of that.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well spotted!

Am I reading it correctly that when you set the driving direction then to left hand side, then a left turn is magically turned into a right hand one? If this is the case then I don't need change anything.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ok, after a bit of investigation, this does exactly what we need. I've added tests to verify.

@leonardehrenfried
leonardehrenfried force-pushed the german-way-properties branch 2 times, most recently from fd95fc3 to 38033fe Compare April 8, 2021 14:45
@leonardehrenfried

Copy link
Copy Markdown
Member Author

It turns out that left/hand right hand traffic was already taken into account however the method and variable names made it hard to find out. I've added tests to make sure that it really is the case.

I've also incorporated @abyrd's feedback.

@leonardehrenfried leonardehrenfried changed the title German way property set to improve bicycle routes German way property set and improvements to bicycle routes Apr 9, 2021
@leonardehrenfried
leonardehrenfried force-pushed the german-way-properties branch 3 times, most recently from e19d1f1 to 106f26b Compare April 13, 2021 13:13
@leonardehrenfried

leonardehrenfried commented Apr 13, 2021

Copy link
Copy Markdown
Member Author

@abyrd I've removed the the request parameter driveOnRight and replaced it with an enum derived from the way property sets. I've also refactored the code that deals with left/right turns, removed the magic and made it generally more clear what is happening.

I've caught more cases where someone uses logical ORs with an unsupported combination and an exception is thrown.

@t2gran @abyrd @optionsome @gmellemstrand Since the improvements of the bicycle routing rests mainly on adding the mixin for lcn/rcn/ncn ways, should I add this to the default way property set?

@t2gran t2gran closed this Apr 15, 2021
@t2gran t2gran reopened this Apr 15, 2021
@leonardehrenfried

Copy link
Copy Markdown
Member Author

I've just taken on the feedback from today's meeting and the formatting feedback by @t2gran.

I think this is good for another round of reviews.

@leonardehrenfried

Copy link
Copy Markdown
Member Author

Unless there are more review comments, I think this one is ready to merge, isn't it?

@t2gran
t2gran merged commit 8adb559 into opentripplanner:dev-2.x Apr 22, 2021
@leonardehrenfried
leonardehrenfried deleted the german-way-properties branch April 22, 2021 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Improvement A functional improvement or micro feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgrade to JUnit 5 Separate intersection traversal cost model for bicycles Taking cycling network relations into account

4 participants