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

Implements correct time calculation for edge-based CH. #1611

Merged
merged 4 commits into from May 16, 2019

Conversation

@easbar
Copy link
Collaborator

easbar commented May 7, 2019

This PR adjusts the path unpacking such that turn cost times are included (previously they were ignored) for edge-based CH, see #1585.

protected void processEdgeBwd(int edgeId, int adjNode, int nextEdgeId) {
EdgeIteratorState edge = graph.getEdgeIteratorState(edgeId, adjNode);
distance += edge.getDistance();
// special case for loop edges: since they do not have a meaningful direction we always need to read them

This comment has been minimized.

Copy link
@easbar

easbar May 7, 2019

Author Collaborator

Here I wonder if we should do the loop-handling within calcMillis. But maybe in a separate PR along with u-turn cleanup.

return;
}
if (weighting instanceof TurnWeighting) {
time += ((TurnWeighting) weighting).calcTurnWeight(inEdge, viaNode, outEdge) * 1000;

This comment has been minimized.

Copy link
@easbar

easbar May 7, 2019

Author Collaborator

Casting here is ugly, I wonder if calcTurnWeight should be in the Weighting interface.

This comment has been minimized.

Copy link
@karussell

karussell May 16, 2019

Member

Probably yes, but later?

This comment has been minimized.

Copy link
@karussell

karussell May 16, 2019

Member

btw: instanceof was really expensive, so having it here would be ugly as this is kind of a loop. But in recent JVMs (at least server-side) this shouldn't be a problem

* Equivalent to java 8 String#join
*/
public static String join(String delimiter, List<String> strings) {
StringJoiner joiner = new StringJoiner(delimiter);

This comment has been minimized.

Copy link
@devemux86

devemux86 May 7, 2019

Contributor

That would cause problem on Android compatibility, since StringJoiner being a Java 8 class, was added in API level 24 (Android 7 Nougat).

This comment has been minimized.

Copy link
@easbar

easbar May 7, 2019

Author Collaborator

Oh right! Thanks for pointing this out, will replace.

expandEdge(sk2, !reverseOrder);
expandEdge(sk1, reverseOrder);
}
private void expandSkippedEdgesNodeBased(int skippedEdge1, int skippedEdge2, int base, int adj, boolean reverse) {

This comment has been minimized.

Copy link
@easbar

easbar May 7, 2019

Author Collaborator

I changed this in the following way: Previously we got the CHEdgeIteratorState like this: base<-mid->adj and then passed on the reverse flags such that the base<-mid edge was read 'in reverse'/base->mid. Now I get the states like this: base->mid->adj from the beginning, which I think makes it a lot easier to understand.

// since the first/last orig edge is not stateful (just like skipped1/2) we have to find out which one
// is attached to adjNode, similar as we do for skipped1/2.
// todo: it is wasteful to create a CHEdgeIteratorState object just to check which edge we have to use!
return graph.getEdgeIteratorState(edgeState.getOrigEdgeLast(), adjNode) == null

This comment has been minimized.

Copy link
@easbar

easbar May 7, 2019

Author Collaborator

Maybe add a special method to CHGraph here to prevent creating the edge iterator state object ?

This comment has been minimized.

Copy link
@easbar

easbar May 16, 2019

Author Collaborator

done, by adding isAdjacentNode method to Graph.

tmpEdge = currEdge.edge;
}

private void extractBwdPath() {

This comment has been minimized.

Copy link
@easbar

easbar May 7, 2019

Author Collaborator

I changed the way the edges are traversed here to make sure we are always in the same situation as in the backward search: adj-->base-nextEdgeId. Otherwise the 'incEdge' of the prev/next SPTEntry would not be the one we need to calculate the turn cost.

easbar added 2 commits May 16, 2019
…unpacking.
"never happen because edge-based CH does not use bidirectional shortcuts at the moment";
CHEdgeIteratorState sk1 = getEdge(skippedEdge1, sk2.getBaseNode());
if (base == adj && (sk1.getAdjNode() == sk1.getBaseNode() || sk2.getAdjNode() == sk2.getBaseNode())) {
throw new IllegalStateException(String.format(Locale.ROOT,

This comment has been minimized.

Copy link
@karussell

karussell May 16, 2019

Member

Does this never happen because we remove loops in the import? Or this is something else as we still have loop support elsewhere like in getShortcutUnpacker?

This comment has been minimized.

Copy link
@easbar

easbar May 16, 2019

Author Collaborator

I think a skipped edge that is a loop is impossible unless there is a bug in CH preparation. I think this check has been there more or less since the very beginning and I think I never saw this error :)

This comment has been minimized.

Copy link
@easbar

easbar May 16, 2019

Author Collaborator

I did not see this error before and after we excluded loops during import.

This comment has been minimized.

Copy link
@easbar

easbar May 16, 2019

Author Collaborator

Regarding loop support I think our algorithms can handle them in most cases but what is not possible is to decide in which direction a loop should be read (problematic when there is geometry or different fwd/bwd speeds etc.)

This comment has been minimized.

Copy link
@karussell

karussell May 16, 2019

Member

Ok, thanks for the explanation!

(problematic when there is geometry or different fwd/bwd speeds etc.)

Should we create a separate issue for this? I think there is a solution (although not without problems) #1525 (comment)

This comment has been minimized.

Copy link
@easbar

easbar May 16, 2019

Author Collaborator

Yes! And another one for some turn-cost cleanup stuff, like u-turn in calcMillis and calcTurnCost in Weighting. Will do.

This comment has been minimized.

Copy link
@karussell

karussell May 16, 2019

Member

Thanks, so from my perspective you can merge (have not fully understood everything but is also not required yet ;) )

This comment has been minimized.

Copy link
@easbar

easbar May 16, 2019

Author Collaborator

Ok thanks for the review! The changes in shortcutunpacker and pathbidirref are really not easy to understand without thinking about it in detail, sketching some examples etc. Was not so easy to get it right to be honest. I might post a little drawing to make it clear.

@karussell karussell added this to the 0.13 milestone May 16, 2019
@karussell karussell added the bug label May 16, 2019
@easbar easbar merged commit 559f7c3 into graphhopper:master May 16, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@easbar easbar deleted the easbar:edge_based_ch_turn_cost_times branch May 16, 2019
@easbar easbar referenced this pull request May 16, 2019
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.