Skip to content

Commit c235e30

Browse files
easbarkarussell
easbar
authored and
karussell
committed
Extracts new ShortcutUnpacker from Path4CH. (graphhopper#1603)
* Improves error message and adds comment. * Adds some tests. * Extracts new ShortcutUnpacker from Path4CH.
1 parent 74d9cef commit c235e30

File tree

7 files changed

+401
-72
lines changed

7 files changed

+401
-72
lines changed

‎core/src/main/java/com/graphhopper/routing/ch/NodeBasedNodeContractor.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,7 @@ private long findShortcuts(ShortcutHandler sch) {
172172
continue;
173173

174174
final double incomingEdgeWeight = prepareWeighting.calcWeight(incomingEdges, true, EdgeIterator.NO_EDGE);
175+
// this check is important to prevent calling calcMillis on inaccessible edges and also allows early exit
175176
if (Double.isInfinite(incomingEdgeWeight)) {
176177
continue;
177178
}
@@ -360,7 +361,7 @@ public String toString() {
360361
else
361362
str = from + "->";
362363

363-
return str + to + ", weight:" + weight + " (" + skippedEdge1 + "," + skippedEdge2 + ")";
364+
return str + to + ", weight:" + weight + " (" + skippedEdge1 + "," + skippedEdge2 + "), dist: " + dist;
364365
}
365366
}
366367

‎core/src/main/java/com/graphhopper/routing/ch/Path4CH.java

Lines changed: 13 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -20,81 +20,29 @@
2020
import com.graphhopper.routing.PathBidirRef;
2121
import com.graphhopper.routing.weighting.Weighting;
2222
import com.graphhopper.storage.Graph;
23-
import com.graphhopper.util.CHEdgeIteratorState;
23+
import com.graphhopper.storage.ShortcutUnpacker;
2424
import com.graphhopper.util.EdgeIterator;
25+
import com.graphhopper.util.EdgeIteratorState;
2526

26-
import java.util.Locale;
27-
28-
/**
29-
* Recursively unpack shortcuts.
30-
* <p>
31-
*
32-
* @author Peter Karich
33-
* @see PrepareContractionHierarchies
34-
*/
3527
public class Path4CH extends PathBidirRef {
36-
private final Graph routingGraph;
28+
private final ShortcutUnpacker shortcutUnpacker;
3729

38-
public Path4CH(Graph routingGraph, Graph baseGraph, Weighting weighting) {
30+
public Path4CH(Graph routingGraph, Graph baseGraph, final Weighting weighting) {
3931
super(baseGraph, weighting);
40-
this.routingGraph = routingGraph;
32+
this.shortcutUnpacker = new ShortcutUnpacker(routingGraph, new ShortcutUnpacker.Visitor() {
33+
@Override
34+
public void visit(EdgeIteratorState edge, boolean reverse, int prevOrNextEdgeId) {
35+
distance += edge.getDistance();
36+
time += weighting.calcMillis(edge, reverse, EdgeIterator.NO_EDGE);
37+
addEdge(edge.getEdge());
38+
}
39+
});
4140
}
4241

4342
@Override
4443
protected final void processEdge(int edgeId, int endNode, int prevEdgeId) {
4544
// Shortcuts do only contain valid weight so first expand before adding
4645
// to distance and time
47-
expandEdge(getEdge(edgeId, endNode), false);
48-
}
49-
50-
private void expandEdge(CHEdgeIteratorState edge, boolean reverse) {
51-
if (!edge.isShortcut()) {
52-
distance += edge.getDistance();
53-
time += weighting.calcMillis(edge, reverse, EdgeIterator.NO_EDGE);
54-
addEdge(edge.getEdge());
55-
return;
56-
}
57-
expandSkippedEdges(edge.getSkippedEdge1(), edge.getSkippedEdge2(), edge.getBaseNode(), edge.getAdjNode(), reverse);
58-
}
59-
60-
private void expandSkippedEdges(int skippedEdge1, int skippedEdge2, int from, int to, boolean reverse) {
61-
// for edge-based CH we need to take special care for loop shortcuts
62-
if (from != to) {
63-
// get properties like speed of the edge in the correct direction
64-
if (reverseOrder == reverse) {
65-
int tmp = from;
66-
from = to;
67-
to = tmp;
68-
}
69-
CHEdgeIteratorState sk2to = getEdge(skippedEdge2, to);
70-
if (sk2to != null) {
71-
expandEdge(sk2to, !reverseOrder);
72-
expandEdge(getEdge(skippedEdge1, from), reverseOrder);
73-
} else {
74-
expandEdge(getEdge(skippedEdge1, to), !reverseOrder);
75-
expandEdge(getEdge(skippedEdge2, from), reverseOrder);
76-
}
77-
} else {
78-
CHEdgeIteratorState sk1 = getEdge(skippedEdge1, from);
79-
CHEdgeIteratorState sk2 = getEdge(skippedEdge2, from);
80-
if (sk1.getAdjNode() == sk1.getBaseNode() || sk2.getAdjNode() == sk2.getBaseNode()) {
81-
// this is a loop where both skipped edges are loops. but this should never happen.
82-
throw new IllegalStateException(String.format(Locale.ROOT,
83-
"error: detected edge where both skipped edges are loops. from: %d, to: %d, " +
84-
"skip-edge1: %d, skip-edge2: %d, reverse: %b", from, to, skippedEdge1, skippedEdge2, reverse));
85-
}
86-
87-
if (!reverseOrder) {
88-
expandEdge(sk1, !reverse);
89-
expandEdge(sk2, reverse);
90-
} else {
91-
expandEdge(sk2, reverse);
92-
expandEdge(sk1, !reverse);
93-
}
94-
}
95-
}
96-
97-
private CHEdgeIteratorState getEdge(int edgeId, int adjNode) {
98-
return (CHEdgeIteratorState) routingGraph.getEdgeIteratorState(edgeId, adjNode);
46+
shortcutUnpacker.visitOriginalEdges(edgeId, endNode, reverseOrder);
9947
}
10048
}

‎core/src/main/java/com/graphhopper/routing/weighting/AbstractWeighting.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,8 @@ public long calcMillis(EdgeIteratorState edgeState, boolean reverse, int prevOrN
4949
if (reverse && !edgeState.getReverse(accessEnc) || !reverse && !edgeState.get(accessEnc))
5050
throw new IllegalStateException("Calculating time should not require to read speed from edge in wrong direction. " +
5151
"(" + edgeState.getBaseNode() + " - " + edgeState.getAdjNode() + ") "
52-
+ edgeState.fetchWayGeometry(3) + " " + edgeState.getDistance() + " "
53-
+ "Reverse:" + reverse + ", fwd:" + edgeState.get(accessEnc) + ", bwd:" + edgeState.getReverse(accessEnc));
52+
+ edgeState.fetchWayGeometry(3) + ", dist: " + edgeState.getDistance() + " "
53+
+ "Reverse:" + reverse + ", fwd:" + edgeState.get(accessEnc) + ", bwd:" + edgeState.getReverse(accessEnc) + ", fwd-speed: " + edgeState.get(avSpeedEnc) + ", bwd-speed: " + edgeState.getReverse(avSpeedEnc));
5454

5555
double speed = reverse ? edgeState.getReverse(avSpeedEnc) : edgeState.get(avSpeedEnc);
5656
if (Double.isInfinite(speed) || Double.isNaN(speed) || speed < 0)

‎core/src/main/java/com/graphhopper/storage/ShortcutUnpacker.java

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
package com.graphhopper.storage;
2+
3+
import com.graphhopper.routing.ch.PrepareContractionHierarchies;
4+
import com.graphhopper.util.CHEdgeIteratorState;
5+
import com.graphhopper.util.EdgeIterator;
6+
import com.graphhopper.util.EdgeIteratorState;
7+
8+
import java.util.Locale;
9+
10+
/**
11+
* Recursively unpack shortcuts.
12+
* <p>
13+
*
14+
* @author Peter Karich
15+
* @author easbar
16+
* @see PrepareContractionHierarchies
17+
*/
18+
public class ShortcutUnpacker {
19+
private final Graph graph;
20+
private final Visitor visitor;
21+
private boolean reverseOrder;
22+
23+
public ShortcutUnpacker(Graph graph, Visitor visitor) {
24+
this.graph = graph;
25+
this.visitor = visitor;
26+
}
27+
28+
/**
29+
* Finds an edge/shortcut with the given id and adjNode and calls the visitor for each original edge that is
30+
* packed inside this shortcut (or if an original edge is given simply calls the visitor on it).
31+
*
32+
* @param reverseOrder if true the original edges will be traversed in reverse order
33+
*/
34+
public void visitOriginalEdges(int edgeId, int adjNode, boolean reverseOrder) {
35+
this.reverseOrder = reverseOrder;
36+
CHEdgeIteratorState edge = getEdge(edgeId, adjNode);
37+
if (edge == null) {
38+
throw new IllegalArgumentException("Edge with id: " + edgeId + " does not exist or does not touch node " + adjNode);
39+
}
40+
expandEdge(edge, false);
41+
}
42+
43+
private void expandEdge(CHEdgeIteratorState edge, boolean reverse) {
44+
if (!edge.isShortcut()) {
45+
// todo: should properly pass previous edge here. for example this is important for turn cost time evaluation
46+
// with edge-based CH, #1585
47+
visitor.visit(edge, reverse, EdgeIterator.NO_EDGE);
48+
return;
49+
}
50+
expandSkippedEdges(edge.getSkippedEdge1(), edge.getSkippedEdge2(), edge.getBaseNode(), edge.getAdjNode(), reverse);
51+
}
52+
53+
private void expandSkippedEdges(int skippedEdge1, int skippedEdge2, int from, int to, boolean reverse) {
54+
// for edge-based CH we need to take special care for loop shortcuts
55+
if (from == to) {
56+
CHEdgeIteratorState sk1 = getEdge(skippedEdge1, from);
57+
CHEdgeIteratorState sk2 = getEdge(skippedEdge2, from);
58+
if (sk1.getAdjNode() == sk1.getBaseNode() || sk2.getAdjNode() == sk2.getBaseNode()) {
59+
// this is a loop where both skipped edges are loops. but this should never happen.
60+
throw new IllegalStateException(String.format(Locale.ROOT,
61+
"error: detected edge where both skipped edges are loops. from: %d, to: %d, " +
62+
"skip-edge1: %d, skip-edge2: %d, reverse: %b", from, to, skippedEdge1, skippedEdge2, reverse));
63+
}
64+
65+
if (reverseOrder == reverse) {
66+
expandEdge(sk1, !reverseOrder);
67+
expandEdge(sk2, reverseOrder);
68+
} else {
69+
expandEdge(sk2, !reverseOrder);
70+
expandEdge(sk1, reverseOrder);
71+
}
72+
} else {
73+
// get properties like speed of the edge in the correct direction
74+
if (reverseOrder != reverse) {
75+
int tmp = from;
76+
from = to;
77+
to = tmp;
78+
}
79+
CHEdgeIteratorState sk2to = getEdge(skippedEdge2, to);
80+
if (sk2to != null) {
81+
expandEdge(getEdge(skippedEdge1, from), !reverseOrder);
82+
expandEdge(sk2to, reverseOrder);
83+
} else {
84+
expandEdge(getEdge(skippedEdge2, from), !reverseOrder);
85+
expandEdge(getEdge(skippedEdge1, to), reverseOrder);
86+
}
87+
}
88+
}
89+
90+
private CHEdgeIteratorState getEdge(int edgeId, int adjNode) {
91+
return (CHEdgeIteratorState) graph.getEdgeIteratorState(edgeId, adjNode);
92+
}
93+
94+
public interface Visitor {
95+
void visit(EdgeIteratorState edge, boolean reverse, int prevOrNextEdgeId);
96+
}
97+
}

‎core/src/test/java/com/graphhopper/routing/CHQueryWithTurnCostsTest.java

Lines changed: 75 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,7 @@
2727
import com.graphhopper.routing.weighting.ShortestWeighting;
2828
import com.graphhopper.routing.weighting.TurnWeighting;
2929
import com.graphhopper.routing.weighting.Weighting;
30-
import com.graphhopper.storage.CHGraph;
31-
import com.graphhopper.storage.GraphBuilder;
32-
import com.graphhopper.storage.GraphHopperStorage;
33-
import com.graphhopper.storage.TurnCostExtension;
30+
import com.graphhopper.storage.*;
3431
import com.graphhopper.util.CHEdgeIteratorState;
3532
import com.graphhopper.util.EdgeIteratorState;
3633
import com.graphhopper.util.GHUtility;
@@ -111,13 +108,87 @@ public void testFindPathWithTurnCosts_bidirected_no_shortcuts() {
111108
// contraction yields no shortcuts
112109
setLevelEqualToNodeIdForAllNodes();
113110

111+
// note that we are using the shortest weighting but turn cost times are included whatsoever, see #1590
114112
testPathCalculation(0, 1, 40, IntArrayList.from(0, 2, 4, 6, 5, 3, 1));
115113
testPathCalculation(1, 0, 28, IntArrayList.from(1, 3, 5, 6, 4, 2, 0));
116114
testPathCalculation(4, 3, 23, IntArrayList.from(4, 6, 5, 3));
117115
testPathCalculation(0, 0, 0, IntArrayList.from(0));
118116
testPathCalculation(4, 4, 0, IntArrayList.from(4));
119117
}
120118

119+
@Test
120+
public void testFindPathWithTurnCosts_loopShortcutBwdSearch() {
121+
// the loop shortcut 4-4 will be encountered during the bwd search
122+
// 3
123+
// / \
124+
// 1 2
125+
// \ /
126+
// 0 - 7 - 8 - 4 - 6 - 5
127+
graph.edge(0, 7, 1, false);
128+
graph.edge(7, 8, 1, false);
129+
graph.edge(8, 4, 1, false);
130+
graph.edge(4, 1, 1, false);
131+
graph.edge(1, 3, 1, false);
132+
graph.edge(3, 2, 1, false);
133+
graph.edge(2, 4, 1, false);
134+
graph.edge(4, 6, 1, false);
135+
graph.edge(6, 5, 1, false);
136+
addRestriction(8, 4, 6);
137+
addRestriction(8, 4, 2);
138+
addRestriction(1, 4, 6);
139+
140+
graph.freeze();
141+
142+
// from contracting node 1
143+
addShortcut(4, 3, 3, 4, 3, 4, 2);
144+
// from contracting node 2
145+
addShortcut(3, 4, 5, 6, 5, 6, 2);
146+
// from contracting node 3
147+
addShortcut(4, 4, 3, 6, 9, 10, 4);
148+
// from contracting node 4
149+
addShortcut(8, 4, 2, 6, 2, 11, 5);
150+
addShortcut(8, 6, 2, 7, 12, 7, 6);
151+
setLevelEqualToNodeIdForAllNodes();
152+
153+
testPathCalculation(0, 5, 9, IntArrayList.from(0, 7, 8, 4, 1, 3, 2, 4, 6, 5));
154+
}
155+
156+
@Test
157+
public void testFindPathWithTurnCosts_loopShortcutFwdSearch() {
158+
// the loop shortcut 4-4 will be encountered during the fwd search
159+
// 3
160+
// / \
161+
// 1 2
162+
// \ /
163+
// 5 - 6 - 4 - 7 - 8 - 0
164+
graph.edge(5, 6, 1, false);
165+
graph.edge(6, 4, 1, false);
166+
graph.edge(4, 1, 1, false);
167+
graph.edge(1, 3, 1, false);
168+
graph.edge(3, 2, 1, false);
169+
graph.edge(2, 4, 1, false);
170+
graph.edge(4, 7, 1, false);
171+
graph.edge(7, 8, 1, false);
172+
graph.edge(8, 0, 1, false);
173+
addRestriction(6, 4, 7);
174+
addRestriction(6, 4, 2);
175+
addRestriction(1, 4, 7);
176+
graph.freeze();
177+
178+
// from contracting node 1
179+
addShortcut(4, 3, 2, 3, 2, 3, 2);
180+
// from contracting node 2
181+
addShortcut(3, 4, 4, 5, 4, 5, 2);
182+
// from contracting node 3
183+
addShortcut(4, 4, 2, 5, 9, 10, 4);
184+
// from contracting node 4
185+
addShortcut(6, 4, 1, 5, 1, 11, 5);
186+
addShortcut(6, 7, 1, 6, 12, 6, 6);
187+
setLevelEqualToNodeIdForAllNodes();
188+
189+
testPathCalculation(5, 0, 9, IntArrayList.from(5, 6, 4, 1, 3, 2, 4, 7, 8, 0));
190+
}
191+
121192
@Test
122193
public void testFindPathWithTurnCosts_directed_single_shortcut() {
123194
// 2 3

‎core/src/test/java/com/graphhopper/routing/ch/CHTurnCostTest.java

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -607,6 +607,29 @@ public void testFindPath_bug2() {
607607
compareCHWithDijkstra(1000, contractionOrder);
608608
}
609609

610+
@Test
611+
public void testFindPath_loop() {
612+
// 3
613+
// / \
614+
// 1 2
615+
// \ /
616+
// 0 - 7 - 8 - 4 - 6 - 5
617+
graph.edge(0, 7, 1, false);
618+
graph.edge(7, 8, 1, false);
619+
graph.edge(8, 4, 1, false);
620+
graph.edge(4, 1, 1, false);
621+
graph.edge(1, 3, 1, false);
622+
graph.edge(3, 2, 1, false);
623+
graph.edge(2, 4, 1, false);
624+
graph.edge(4, 6, 1, false);
625+
graph.edge(6, 5, 1, false);
626+
addRestriction(8, 4, 6);
627+
graph.freeze();
628+
629+
RoutingAlgorithmFactory factory = prepareCH(Arrays.asList(0, 1, 2, 3, 4, 5, 6, 7, 8));
630+
compareCHQueryWithDijkstra(factory, 0, 5);
631+
}
632+
610633
@Test
611634
public void testFindPath_loopsMustAlwaysBeAccepted() {
612635
// ---

0 commit comments

Comments
 (0)