-
Notifications
You must be signed in to change notification settings - Fork 75
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
base: dev
Are you sure you want to change the base?
Conversation
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
Also adds tests
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.
Also tests added.
Also tests are added
@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. |
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. |
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. |
The only thing @landonreed changed when resolving conflicts in his merge 35342b9 was a few imports. |
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. |
Accidentally closed when cleaning up branches. I would like to revisit these old pull requests and determine whether they're still relevant. |
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.