Skip to content

Commit f00960c

Browse files
karusselleasbar
karussell
and
easbar
authored
trying to fix problems with incorrectly mapped loop edges (graphhopper#1535)
* trying to fix graphhopper#1533 * Adds visualization of OSM loop. * minor fix in test data Co-authored-by: easbar <[email protected]>
1 parent 396873c commit f00960c

File tree

5 files changed

+178
-12
lines changed

5 files changed

+178
-12
lines changed

‎reader-osm/src/main/java/com/graphhopper/reader/osm/OSMReader.java

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -654,9 +654,16 @@ Collection<EdgeIteratorState> addOSMWay(final LongIndexedContainer osmNodeIds, f
654654
tmpNode = -tmpNode - 3;
655655

656656
if (firstNode >= 0 && firstNode == tmpNode) {
657-
// loop detected. See #1525. Insert last OSM ID as tower node. Do this for all loops so that users can manipulate loops later arbitrarily.
657+
// loop detected. See #1525 and #1533. Insert last OSM ID as tower node. Do this for all loops so that users can manipulate loops later arbitrarily.
658658
long lastOsmNodeId = osmNodeIds.get(i - 1);
659-
int newEndNode = -handlePillarNode(getNodeMap().get(lastOsmNodeId), lastOsmNodeId, pointList, true) - 3;
659+
int lastGHNodeId = getNodeMap().get(lastOsmNodeId);
660+
if (lastGHNodeId < TOWER_NODE) {
661+
LOGGER.warn("Pillar node " + lastOsmNodeId + " is already a tower node and used in loop, see #1533. " +
662+
"Fix mapping for way " + wayOsmId + ", nodes:" + osmNodeIds);
663+
break;
664+
}
665+
666+
int newEndNode = -handlePillarNode(lastGHNodeId, lastOsmNodeId, pointList, true) - 3;
660667
newEdges.add(addEdge(firstNode, newEndNode, pointList, flags, wayOsmId));
661668
pointList.clear();
662669
pointList.add(nodeAccess, newEndNode);

‎reader-osm/src/test/java/com/graphhopper/reader/osm/OSMReaderTest.java

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,6 @@ public class OSMReaderTest {
6565
private final String fileBarriers = "test-barriers.xml";
6666
private final String fileTurnRestrictions = "test-restrictions.xml";
6767
private final String fileRoadAttributes = "test-road-attributes.xml";
68-
private final String fileAvoidLoops = "test-avoid-loops.xml";
6968
private final String dir = "./target/tmp/test-db";
7069
private CarFlagEncoder carEncoder;
7170
private BikeFlagEncoder bikeEncoder;
@@ -394,12 +393,16 @@ public void testBarriers() {
394393
}
395394

396395
@Test
397-
public void avoidsLoopEdges() {
396+
public void avoidsLoopEdges_1525() {
398397
// loops in OSM should be avoided by adding additional tower node (see #1525, #1531)
399398
// C - D
400399
// \ /
401400
// A - B - E
402-
GraphHopper hopper = new GraphHopperFacade(fileAvoidLoops).importOrLoad();
401+
GraphHopper hopper = new GraphHopperFacade("test-avoid-loops.xml").importOrLoad();
402+
checkLoop(hopper);
403+
}
404+
405+
void checkLoop(GraphHopper hopper) {
403406
GraphHopperStorage graph = hopper.getGraphHopperStorage();
404407

405408
// A, B, E and one of C or D should be tower nodes, in any case C and D should not be collapsed entirely
@@ -411,14 +414,31 @@ public void avoidsLoopEdges() {
411414
while (iter.next()) {
412415
assertTrue("found a loop", iter.getAdjNode() != iter.getBaseNode());
413416
}
414-
int nodeB = -1;
415-
for (int i = 0; i < graph.getNodes(); i++) {
416-
if (Math.abs(graph.getNodeAccess().getLat(i) - 12) < 1.e-3) {
417-
nodeB = i;
418-
}
417+
int nodeB = AbstractGraphStorageTester.getIdOf(graph, 12);
418+
assertTrue("could not find OSM node B", nodeB > -1);
419+
assertEquals(4, GHUtility.count(graph.createEdgeExplorer().setBaseNode(nodeB)));
420+
}
421+
422+
@Test
423+
public void avoidsLoopEdgesIdenticalLatLon_1533() {
424+
checkLoop(new GraphHopperFacade("test-avoid-loops2.xml").importOrLoad());
425+
}
426+
427+
@Test
428+
public void avoidsLoopEdgesIdenticalNodeIds_1533() {
429+
// We can handle the following case with the proper result:
430+
checkLoop(new GraphHopperFacade("test-avoid-loops3.xml").importOrLoad());
431+
// We cannot handle the following case, i.e. no loop is created. so we only check that there are no loops
432+
GraphHopper hopper = new GraphHopperFacade("test-avoid-loops4.xml").importOrLoad();
433+
GraphHopperStorage graph = hopper.getGraphHopperStorage();
434+
AllEdgesIterator iter = graph.getAllEdges();
435+
assertEquals(2, iter.length());
436+
while (iter.next()) {
437+
assertTrue("found a loop", iter.getAdjNode() != iter.getBaseNode());
419438
}
439+
int nodeB = AbstractGraphStorageTester.getIdOf(graph, 12);
420440
assertTrue("could not find OSM node B", nodeB > -1);
421-
assertEquals(3, GHUtility.getNeighbors(graph.createEdgeExplorer().setBaseNode(nodeB)).size());
441+
assertEquals(2, GHUtility.count(graph.createEdgeExplorer().setBaseNode(nodeB)));
422442
}
423443

424444
@Test
@@ -555,7 +575,7 @@ public void testRoadAttributes() {
555575
int nb = AbstractGraphStorageTester.getIdOf(graph, 12, 51);
556576
int nc = AbstractGraphStorageTester.getIdOf(graph, 11.2, 52);
557577
int nd = AbstractGraphStorageTester.getIdOf(graph, 11.3, 51);
558-
int ne = AbstractGraphStorageTester.getIdOf( graph, 10, 51 );
578+
int ne = AbstractGraphStorageTester.getIdOf(graph, 10, 51);
559579

560580
EdgeIteratorState edge_ab = GHUtility.getEdge(graph, na, nb);
561581
EdgeIteratorState edge_ad = GHUtility.getEdge(graph, na, nd);

‎reader-osm/src/test/resources/com/graphhopper/reader/osm/test-avoid-loops2.xml

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
<?xml version='1.0' encoding='UTF-8'?>
2+
<osm version="0.6" generator="pbf2osm">
3+
4+
<!--
5+
C - D
6+
\ /
7+
A - B - E
8+
-->
9+
10+
<node id="1" lat="11" lon="50" uid="A">
11+
<tag k="name" v="A"/>
12+
</node>
13+
<node id="2" lat="12" lon="51" uid="B">
14+
<tag k="name" v="B"/>
15+
</node>
16+
<node id="6" lat="12" lon="51" uid="B2">
17+
<tag k="name" v="B2"/>
18+
</node>
19+
<node id="3" lat="13" lon="52" uid="C">
20+
<tag k="name" v="C"/>
21+
</node>
22+
<node id="4" lat="14" lon="51" uid="D">
23+
<tag k="name" v="D"/>
24+
</node>
25+
<node id="5" lat="15" lon="51" uid="E">
26+
<tag k="name" v="E"/>
27+
</node>
28+
29+
<way id="10" uid="85761">
30+
<nd ref="1"/>
31+
<nd ref="2"/>
32+
<nd ref="5"/>
33+
<tag k="name" v="A B E"/>
34+
<tag k="highway" v="secondary"/>
35+
</way>
36+
37+
<way id="12" uid="85762">
38+
<nd ref="2"/>
39+
<nd ref="6"/>
40+
<nd ref="3"/>
41+
<nd ref="4"/>
42+
<nd ref="2"/>
43+
<tag k="name" v="B B2 C D B"/>
44+
<tag k="highway" v="secondary"/>
45+
</way>
46+
47+
</osm>

‎reader-osm/src/test/resources/com/graphhopper/reader/osm/test-avoid-loops3.xml

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
<?xml version='1.0' encoding='UTF-8'?>
2+
<osm version="0.6" generator="pbf2osm">
3+
4+
<!--
5+
C - D
6+
\ /
7+
A - B - E
8+
/ \
9+
~~~
10+
-->
11+
12+
<node id="1" lat="11" lon="50" uid="A">
13+
<tag k="name" v="A"/>
14+
</node>
15+
<node id="2" lat="12" lon="51" uid="B">
16+
<tag k="name" v="B"/>
17+
</node>
18+
<node id="3" lat="13" lon="52" uid="C">
19+
<tag k="name" v="C"/>
20+
</node>
21+
<node id="4" lat="14" lon="51" uid="D">
22+
<tag k="name" v="D"/>
23+
</node>
24+
<node id="5" lat="15" lon="51" uid="E">
25+
<tag k="name" v="E"/>
26+
</node>
27+
28+
<way id="10" uid="85761">
29+
<nd ref="1"/>
30+
<nd ref="2"/>
31+
<nd ref="5"/>
32+
<tag k="name" v="A B E"/>
33+
<tag k="highway" v="secondary"/>
34+
</way>
35+
36+
<way id="12" uid="85762">
37+
<nd ref="2"/>
38+
<nd ref="4"/>
39+
<nd ref="3"/>
40+
<nd ref="2"/>
41+
<nd ref="2"/>
42+
<tag k="name" v="B D C B B"/>
43+
<tag k="highway" v="secondary"/>
44+
</way>
45+
46+
</osm>

‎reader-osm/src/test/resources/com/graphhopper/reader/osm/test-avoid-loops4.xml

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
<?xml version='1.0' encoding='UTF-8'?>
2+
<osm version="0.6" generator="pbf2osm">
3+
4+
<!--
5+
C - D
6+
\ /
7+
A - B - E
8+
/ \
9+
~~~
10+
-->
11+
12+
<node id="1" lat="11" lon="50" uid="A">
13+
<tag k="name" v="A"/>
14+
</node>
15+
<node id="2" lat="12" lon="51" uid="B">
16+
<tag k="name" v="B"/>
17+
</node>
18+
<node id="3" lat="13" lon="52" uid="C">
19+
<tag k="name" v="C"/>
20+
</node>
21+
<node id="4" lat="14" lon="51" uid="D">
22+
<tag k="name" v="D"/>
23+
</node>
24+
<node id="5" lat="15" lon="51" uid="E">
25+
<tag k="name" v="E"/>
26+
</node>
27+
28+
<way id="10" uid="85761">
29+
<nd ref="1"/>
30+
<nd ref="2"/>
31+
<nd ref="5"/>
32+
<tag k="name" v="A B E"/>
33+
<tag k="highway" v="secondary"/>
34+
</way>
35+
36+
<way id="12" uid="85762">
37+
<nd ref="2"/>
38+
<nd ref="2"/>
39+
<nd ref="3"/>
40+
<nd ref="4"/>
41+
<nd ref="2"/>
42+
<tag k="name" v="B B C D B"/>
43+
<tag k="highway" v="secondary"/>
44+
</way>
45+
46+
</osm>

0 commit comments

Comments
 (0)