Fixing way sectioning bug in raw atlas (#230)

* Fixing way sectioning bug in raw atlas * Updating closed ring logic for consistency * removing intermediate duplicate variable and performing occurrence calculation inline
osmlab · Oct 1, 2018 · 5dc1ae8 · 5dc1ae8
1 parent 6809924
commit 5dc1ae8
Show file tree

Hide file tree

Showing 5 changed files with 56 additions and 27 deletions.
diff --git a/...main/java/org/openstreetmap/atlas/geography/atlas/raw/sectioning/WaySectionProcessor.java b/...main/java/org/openstreetmap/atlas/geography/atlas/raw/sectioning/WaySectionProcessor.java
@@ -1063,23 +1063,16 @@ private List<TemporaryEdge> splitNonRingLineIntoEdges(final WaySectionChangeSet
                         continue;
                     }
 
-                    // Update end point occurrence to factor in duplicates
-                    final Integer duplicates = duplicateLocations.get(end.getIdentifier());
-                    if (duplicates != null)
-                    {
-                        for (int duplicate = 0; duplicate < duplicates; duplicate++)
-                        {
-                            nodesToSectionAt.incrementOccurrence(end);
-                        }
-                    }
-
                     // We found the end node, create the edge. Note: using occurrence minus one
                     // since PolyLine uses zero-based numbering. We are incrementing only the
                     // start node occurrence, since the end node will either be used as a future
                     // start node or be the end of the way, in which case we don't care.
                     final int startOccurrence = nodesToSectionAt.getOccurrence(startNode.get()) - 1;
                     nodesToSectionAt.incrementOccurrence(startNode.get());
-                    final int endOccurrence = nodesToSectionAt.getOccurrence(end) - 1;
+
+                    // Update end point occurrence to factor in any duplicates in the polyline
+                    final int endOccurrence = duplicateLocations.getOrDefault(end.getIdentifier(),
+                            0) + nodesToSectionAt.getOccurrence(end) - 1;
 
                     // Build the underlying polyline and reverse it, if necessary
                     final PolyLine rawPolyline = polyline.between(polyline.get(startIndex),
@@ -1220,30 +1213,27 @@ private List<TemporaryEdge> splitRingLineIntoEdges(final WaySectionChangeSet cha
                                 continue;
                             }
 
-                            // Update end point occurrence to factor in duplicates
-                            final Integer duplicates = duplicateLocations.get(currentLocation);
-                            if (duplicates != null)
-                            {
-                                for (int duplicate = 0; duplicate < duplicates; duplicate++)
-                                {
-                                    nodesToSectionAt.incrementOccurrence(end);
-                                }
-                            }
+                            // Handle start occurrence
+                            final int startOccurrence = nodesToSectionAt
+                                    .getOccurrence(startNode.get()) - 1;
+                            nodesToSectionAt.incrementOccurrence(startNode.get());
+
+                            // Update end point occurrence to factor in any duplicates in the
+                            // polyline
+                            final int endOccurrence = duplicateLocations
+                                    .getOrDefault(currentLocation, 0)
+                                    + nodesToSectionAt.getOccurrence(end) - 1;
 
                             // We only want to create an edge if we've started from a node. If we've
                             // started from a shape point, we've just encountered our first node.
                             final PolyLine rawPolyline = polyline.between(polyline.get(startIndex),
-                                    nodesToSectionAt.getOccurrence(startNode.get()) - 1,
-                                    polyline.get(index), nodesToSectionAt.getOccurrence(end) - 1);
+                                    startOccurrence, polyline.get(index), endOccurrence);
                             final PolyLine edgePolyline = isReversed ? rawPolyline.reversed()
                                     : rawPolyline;
 
                             newEdgesForLine
                                     .add(new TemporaryEdge(identifierFactory.nextIdentifier(),
                                             edgePolyline, line.getTags(), hasReverseEdge));
-
-                            // Increment start node occurrence
-                            nodesToSectionAt.incrementOccurrence(startNode.get());
                         }
 
                         // Update starting points
@@ -1275,12 +1265,12 @@ private List<TemporaryEdge> splitRingLineIntoEdges(final WaySectionChangeSet cha
                         }
 
                         // Get the raw polyline from the last node to the last(first) location
-                        final int endOccurence = duplicateLocations.containsKey(currentLocation)
+                        final int endOccurrence = duplicateLocations.containsKey(currentLocation)
                                 ? duplicateLocations.get(currentLocation) : 1;
                         final PolyLine rawPolylineFromLastNodeToLastLocation = polyline.between(
                                 polyline.get(startIndex),
                                 nodesToSectionAt.getOccurrence(startNode.get()) - 1,
-                                currentLocation, endOccurence);
+                                currentLocation, endOccurrence);
 
                         final PolyLine edgePolyLine;
                         if (isReversed)

diff --git a/.../java/org/openstreetmap/atlas/geography/atlas/raw/sectioning/WaySectionProcessorTest.java b/.../java/org/openstreetmap/atlas/geography/atlas/raw/sectioning/WaySectionProcessorTest.java
@@ -208,6 +208,22 @@ public void testLoopWithRepeatedLocation()
                 .size(finalAtlas.edges(edge -> edge.getOsmIdentifier() == 488453376L)) == 2);
     }
 
+    @Test
+    public void testMalformedPolyLine()
+    {
+        // Based on a prior version of https://www.openstreetmap.org/way/621043891
+        final Atlas slicedRawAtlas = this.setup.getMalformedPolyLineAtlas();
+        final CountryBoundaryMap boundaryMap = CountryBoundaryMap
+                .fromPlainText(new InputStreamResource(() -> WaySectionProcessorTest.class
+                        .getResourceAsStream("malformedPolyLineBoundaryMap.txt")));
+        final Atlas finalAtlas = new WaySectionProcessor(slicedRawAtlas,
+                AtlasLoadingOption.createOptionWithAllEnabled(boundaryMap)).run();
+
+        Assert.assertEquals("Six edges, each having a reverse counterpart", 12,
+                finalAtlas.numberOfEdges());
+        Assert.assertEquals("Four nodes", 4, finalAtlas.numberOfNodes());
+    }
+
     @Test
     public void testOneWayRing()
     {

diff --git a/...a/org/openstreetmap/atlas/geography/atlas/raw/sectioning/WaySectionProcessorTestRule.java b/...a/org/openstreetmap/atlas/geography/atlas/raw/sectioning/WaySectionProcessorTestRule.java
@@ -11,6 +11,9 @@
  */
 public class WaySectionProcessorTestRule extends CoreTestRule
 {
+    @TestAtlas(loadFromTextResource = "malformedPolyLine.atlas.txt")
+    private Atlas malformedPolyLineAtlas;
+
     @TestAtlas(loadFromTextResource = "bidirectionalRing.atlas.txt")
     private Atlas bidirectioalRingAtlas;
 
@@ -102,6 +105,11 @@ public Atlas getLoopWithRepeatedLocationAtlas()
         return this.loopWithRepeatedLocation;
     }
 
+    public Atlas getMalformedPolyLineAtlas()
+    {
+        return this.malformedPolyLineAtlas;
+    }
+
     public Atlas getOneWayRingAtlas()
     {
         return this.oneWayRing;

diff --git a/...ources/org/openstreetmap/atlas/geography/atlas/raw/sectioning/malformedPolyLine.atlas.txt b/...ources/org/openstreetmap/atlas/geography/atlas/raw/sectioning/malformedPolyLine.atlas.txt
@@ -0,0 +1,14 @@
+# Nodes
+# Edges
+# Areas
+# Lines
+100000000 && -2.5184858,32.4625827:-2.5184958,32.4625727:-2.5184978,32.4625686:-2.5185019,32.4625707:-2.5185019,32.4625707:-2.5185039,32.4625666:-2.5185039,32.4625666:-2.5184978,32.4625666:-2.5184978,32.4625666:-2.5185039,32.4625666:-2.5185039,32.4625666:-2.5185039,32.4625666:-2.5184998,32.4625666:-2.5184978,32.4625686:-2.5185039,32.4625666 && last_edit_user_name -> 1 || last_edit_changeset -> 0 || last_edit_time -> 1530212492000 || last_edit_user_id -> 1 || iso_country_code -> TZA || highway -> road || last_edit_version -> 1
+# Points
+1000000 && -2.5184858,32.4625827 && last_edit_user_name -> 1 || last_edit_changeset -> 0 || last_edit_time -> 1515321311000 || last_edit_user_id -> 1 || iso_country_code -> TZA || last_edit_version -> 1
+2000000 && -2.5184958,32.4625727 && last_edit_user_name -> 1 || last_edit_changeset -> 0 || last_edit_time -> 1515321311000 || last_edit_user_id -> 1 || iso_country_code -> TZA || last_edit_version -> 1
+3000000 && -2.5184978,32.4625686 && last_edit_user_name -> 1 || last_edit_changeset -> 0 || synthetic_duplicate_osm_node -> YES || last_edit_time -> 1515321311000 || last_edit_user_id -> 1 || iso_country_code -> TZA || last_edit_version -> 1
+4000000 && -2.5185019,32.4625707 && last_edit_user_name -> 1 || last_edit_changeset -> 0 || synthetic_duplicate_osm_node -> YES || last_edit_time -> 1515321311000 || last_edit_user_id -> 1 || iso_country_code -> TZA || last_edit_version -> 1
+6000000 && -2.5185039,32.4625666 && last_edit_user_name -> 1 || last_edit_changeset -> 0 || synthetic_duplicate_osm_node -> YES || last_edit_time -> 1515321311000 || last_edit_user_id -> 1 || iso_country_code -> TZA || last_edit_version -> 1
+8000000 && -2.5184978,32.4625666 && last_edit_user_name -> 1 || last_edit_changeset -> 0 || synthetic_duplicate_osm_node -> YES || last_edit_time -> 1515321311000 || last_edit_user_id -> 1 || iso_country_code -> TZA || last_edit_version -> 1
+13000000 && -2.5184998,32.4625666 && last_edit_user_name -> 1 || last_edit_changeset -> 0 || last_edit_time -> 1515321311000 || last_edit_user_id -> 1 || iso_country_code -> TZA || last_edit_version -> 1
+# Relations
diff --git a/...s/org/openstreetmap/atlas/geography/atlas/raw/sectioning/malformedPolyLineBoundaryMap.txt b/...s/org/openstreetmap/atlas/geography/atlas/raw/sectioning/malformedPolyLineBoundaryMap.txt
@@ -0,0 +1 @@
+TZA||POLYGON ((30.933931 -1.007137, 33.933931 -1.007137, 33.933931 -6.007137, 30.933931 -6.007137, 30.933931 -1.007137))
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		TZA\|\|POLYGON ((30.933931 -1.007137, 33.933931 -1.007137, 33.933931 -6.007137, 30.933931 -6.007137, 30.933931 -1.007137))