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

Fixes edge-based CH problem with u-turns at virtual edges, fixes #1593 #1596

Merged
merged 14 commits into from Apr 11, 2019

Conversation

@easbar
Copy link
Collaborator

easbar commented Apr 10, 2019

Fixes #1593.

The problem is this:
image

Let's say there are edges like this: 0-1-2 with edge-ids 0:0-1 and 1:1-2 and a shortcut gets introduced from 0 to 2 (skipping 1) (with id 2). Then there is a virtual node (id 3) between 1 and 2 like this: 1-(3)-2 which will add some virtual edges 3:1-3 and 4:3-2. When we detect u-turns we compare the edge ids. So if we travel via the shortcut from 0 to 2 we arrive at 2 with the shortcut id 2, but to calculate the turn costs we use the 'origEdgeLast' (here this is the edge 1-2 with id 1). When we check if going to the virtual node 3 is a u-turn we used to check 1==4 which tells us it is no u-turn (when in reality it is). So instead of the virtual edge id 4 we have to use the original edge id 1 for the comparison.

An important case is also checking for a u-turn at node 3 when coming from node 1 and going to node 2. In this case we must not use the original edge id (1) for both virtual edges, but instead compare 3==4.

In this PR I have:

  • cleaned-up and extended RandomCHRoutingTest for edge-based CH. In the past there were tests on random graphs, but they did not include QueryGraph so such issues with virtual edges did not come up (similar to #1574).

  • fixed the issue by adding a isUTurn() method to TurnCostExtension

Ultimately I think the u-turn detection needs some more sophisticated refactoring and we should get rid of the EDGE_BASED_2DIR_UTURN traversal mode (#1520), but for now I think it is most important to fix the bugs we already know.

easbar added 12 commits Apr 3, 2019
…ds that failed in the pass.
easbar added 2 commits Apr 10, 2019
@easbar easbar changed the title Fixes edge-based CH problem with turn restrictions at virtual edges. Fixes edge-based CH problem with u-turns at virtual edges. Apr 10, 2019
@easbar easbar changed the title Fixes edge-based CH problem with u-turns at virtual edges. Fixes edge-based CH problem with u-turns at virtual edges, fixes #1593 Apr 10, 2019
@karussell

This comment has been minimized.

Copy link
Member

karussell commented Apr 10, 2019

Wow the necessary detailed tests are many. Thanks for accepting this challenge :)

BTW: We could need a cleaner solution to this virtual edges problem that bites us again and again :/ ... but any solution that comes to my mind (that does not use virtual edges) becomes tricky with all the special cases like many virtual nodes on one original edge etc and then we have this in every algorithm instead just in the QueryGraph and only some "tiny tricks" in the algorithm like here.

@@ -197,6 +197,10 @@ public long getTurnCostFlags(int edgeFrom, int nodeVia, int edgeTo) {
return nextCostFlags(edgeFrom, nodeVia, edgeTo);
}

public boolean isUTurn(int edgeFrom, int edgeTo) {
return edgeFrom == edgeTo;
}

This comment has been minimized.

Copy link
@karussell

karussell Apr 10, 2019

Member

Wouldn't it make more sense in the TurnWeighting as you wrote in #1593?

This comment has been minimized.

Copy link
@easbar

easbar Apr 10, 2019

Author Collaborator

Yes I was tempted to do it, especially since AbstractBidirectionEdgeCHNoSOD already has a reference to a TurnWeighting. Then again to make it really consistent we should also use isUTurn in AbstractRoutingAlgorithm#accept (but I haven't), but this one only has a reference to Weighting (while it could obtain a TurnCostExtension reference from Graph.getExtension). In any case TurnCostExtension needs this method, so we can override it in QueryGraphTurnExt. The routing algorithms could call it via something like TurnWeighting#isUTurn. I don't know. Either way is fine for me. Maybe postpone this decision until doing a proper u-turn clean up in #1520 ?

This comment has been minimized.

Copy link
@karussell

karussell Apr 10, 2019

Member

Thanks for the explanation - makes sense

Maybe postpone this decision until doing a proper u-turn clean up in #1520 ?

👍

@easbar

This comment has been minimized.

Copy link
Collaborator Author

easbar commented Apr 10, 2019

Wow the necessary detailed tests are many.

I would not say they are all strictly necessary and maybe they could even be simplified / made a bit more to the point. During development I had a few more and already deleted some again. The most important is the random one, because you can just have it running for a while and it spits out some failing cases, which was really helpful to arrive at a solution (now I think it is green).

We could need a cleaner solution to this virtual edges problem that bites us again and again :/ ...

I think the QueryGraph approach is a good one and the reason #1593 and #1574 came up was mostly that we had no tests that included virtual via nodes before. Taking this into account I think we did not get bitten so often after all :)

@karussell karussell added the bug label Apr 10, 2019
@karussell karussell added this to the 0.13 milestone Apr 10, 2019
@easbar easbar merged commit 7b71a24 into master Apr 11, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@karussell karussell deleted the edge_based_ch_error branch Apr 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.