Skip to content

Apply turn restrictions to split edges#3414

Merged
gmellemstrand merged 8 commits into
opentripplanner:dev-2.xfrom
mfdz:split-edge-turn-restrictions
Apr 14, 2021
Merged

Apply turn restrictions to split edges#3414
gmellemstrand merged 8 commits into
opentripplanner:dev-2.xfrom
mfdz:split-edge-turn-restrictions

Conversation

@leonardehrenfried

@leonardehrenfried leonardehrenfried commented Apr 12, 2021

Copy link
Copy Markdown
Member

Summary

So far turn restrictions were not applied correctly to edges split due to being close to a transit stop. This PR fixes it.

Issue

Fixes #2965
Fixes #3402

Unit tests

I've created two tests that take real cases from the Stuttgart, Germany area and do routing on a tiny graph.

Code style

Documentation

n/a

Changelog

@leonardehrenfried leonardehrenfried requested a review from a team April 12, 2021 12:37
@leonardehrenfried

Copy link
Copy Markdown
Member Author

@gmellemstrand If you'd like to review this, I'd be very happy.

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

This looks good from a functionality standpoint. It is also good that you have added tests using minimal OSM test data sets.

What I would like is to have a single place where attributes from StreetEdges (and their vertices) are propagated down to the split edges. There is code at the bottom of the StreetEdge splitDestructively and splitNonDestructively methods that apply some of these parameters. I think those could be replaced with a method call that also applies the turn restrictions.

That way we could keep the VertexLinker a bit simpler, and it will not have to concern itself with the details of how the StreetEdge is implemented.

@leonardehrenfried leonardehrenfried changed the title Apply turn restrictions to split vertices Apply turn restrictions to split edges Apr 13, 2021
@leonardehrenfried

Copy link
Copy Markdown
Member Author

@gmellemstrand Thanks for the review. I've moved the code into the StreetEdge class.

Since adding and removing turn restrictions requires a Graph instance, the code isn't quite as self-contained as I'd hoped. You still have to pass a graph in.

@gmellemstrand

Copy link
Copy Markdown
Contributor

It is unfortunate that you have to pass in the graph instance, but changing this is a bigger refactor, that I think is outside the scope of this PR. It is a problem with the model in general that you have to pass in the graph in many places. Ideally the StreetEdge should point to its own turn restrictions.

I have not checked the turn restriction code in detail, but since it is relatively self-contained and tested, I will approve this PR. Feel free to merge.

@leonardehrenfried

Copy link
Copy Markdown
Member Author

Thanks!

I don't have merge permissions so @t2gran has to take care of it.

@gmellemstrand gmellemstrand merged commit 62139ce into opentripplanner:dev-2.x Apr 14, 2021
@leonardehrenfried leonardehrenfried deleted the split-edge-turn-restrictions branch April 14, 2021 08:17
@t2gran t2gran added this to the 2.1 milestone Apr 20, 2021
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.

Upgrade to JUnit 5 Turn restrictions not propagated to split street edges

3 participants