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

Turn restriction when edges are split #262

Open
wants to merge 17 commits into
base: dev
Choose a base branch
from
Open

Conversation

buma
Copy link
Contributor

@buma buma commented Apr 11, 2017

Fixes problems when turn restrictions aren't applied if from/via edge is split.

Also support splitting edges in scenarios when stops are added. If this happens original network stays the same since turnRestriction lists are copied. TurnRestrictions itself are copied only if from/via edge need to be changed.

If fromedge in turnRestrictions are split in getOrCreateVertexNear then
turn restrictions don't work. This can happen when linking P+R nodes,
bike shares and transit stops.
This adds tests and updates getOrCreateVertexNear so that split edges
have turn restrictions same as when linking P+R areas.
Moved restrictTurn to own class so that it can be used in multiple tests
Previously when via Turn restriction edges were split. Turn
restrictions didn't apply anymore since via edges weren't complete.
Since splitting creates new edges which wasn't in turn restrictions.

Commit fixes that and also adds tests.
This is also needed for turn restriction tests. Default is true. As it
was now. So nothing should change.
If adding stops in scenario splits from/to/via edges Turn restrictions
need to be updated in new transportNetwor. But they need to stay the
same in original. For this we copy all 4 turn restriction fields. And
make copy of each TurnRestriction which needs new from/to/via edges in
this copied list. We could copy on write since chance that turn
restrictions needs to be copied is small, but Turn Restrictions are
small and if need arises that COW is needed we would do it.

Tests are also created
@buma buma changed the title Turn restriction road split Turn restriction when edges are split Apr 11, 2017
It could happen that two different tests that were using FakeGraph with
different OSM data were actually using the same data, since both create
osm.mapdb in /tmp. And if file already exists it is assumed it's from
previous run, but it's actually different graph. This adds temporary
parameter to TransportNetwork#fromFiles which creates temporary
osm.mapdb which is removed after creation if temporary is true. Default
is false since removal should only happen in tests.
@abyrd
Copy link
Member

abyrd commented Apr 23, 2017

@buma will you be adding more commits to this PR/branch since we talked about simplifying all turn restrictions (forward/backward) as "no-turn" restrictions, with no "only" restrictions? I just wanted to check before I review / consider merging this.

@buma
Copy link
Contributor Author

buma commented Apr 24, 2017

I think this can be merged. I'll do only turn -> no turn change on different branch, since I need to firstly simplify splitting. (Currently we have getOrCreateVertex and splitEdge) which both do basically the same. Only second is used only in PR linking.

@abyrd
Copy link
Member

abyrd commented Jun 5, 2017

Hi @buma I'm merging some pull requests now, but unfortunately won't have time to review this in detail right away. Hopefully I will merge it tomorrow in a point release. I want to make sure we get one release out with no deep changes to street routing, just so we don't risk accidentally changing analysis results.

@abyrd
Copy link
Member

abyrd commented Jul 27, 2017

The only thing @landonreed changed when resolving conflicts in his merge 35342b9 was a few imports.

@abyrd
Copy link
Member

abyrd commented Oct 13, 2020

Revisiting this in October 2020, after combining r5 with analysis-backend. These changes may still be applicable but it will take some time to evaluate correctness and determine whether this will necessitate changes elsewhere in the system. This is getting into some complex areas that probably do deserve attention but may take several days to work through properly.

@abyrd abyrd closed this Apr 9, 2021
@abyrd abyrd deleted the branch dev April 9, 2021 04:50
@abyrd abyrd reopened this Apr 9, 2021
@abyrd abyrd changed the base branch from master to dev April 9, 2021 05:20
@abyrd
Copy link
Member

abyrd commented Apr 9, 2021

Accidentally closed when cleaning up branches. I would like to revisit these old pull requests and determine whether they're still relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants