Changeset 15883 in josm

Timestamp:

2020-02-18T15:02:41+01:00 (4 years ago)

Author:

GerdP

Message:

fix #9599: Join Overlapping Areas" creates a new way when joining 2 buildings
Two different problems are solved with this:

• when inner loops are removed it was possible that the old way was removed and only a new part remained
• when ways are joined with CombineWaysAction, the first old way was kept, not the oldest one

Location:

trunk

Files:

• src/org/openstreetmap/josm/actions/JoinAreasAction.java (modified) (8 diffs)
• test/data/regress/9599 (added)
• test/data/regress/9599/ex5.osm (added)
• test/data/regress/9599/three_old.osm (added)
• test/unit/org/openstreetmap/josm/actions/JoinAreasActionTest.java (modified) (1 diff)

trunk/src/org/openstreetmap/josm/actions/JoinAreasAction.java

@@ -480 +480 @@
     public void actionPerformed(ActionEvent e) {
         join(getLayerManager().getEditDataSet().getSelectedWays());
+        clearFields();
+    }
+
+    private void clearFields() {
+        ds = null;
+        cmdsCount = 0;
+        cmds.clear();
+        addedRelations.clear();
     }
 
@@ -488 +496 @@
      */
     public void join(Collection<Way> ways) {
-        cmdsCount = 0;
-        addedRelations.clear();
+        clearFields();
 
         if (ways.isEmpty()) {
@@ -675 +682 @@
         List<WayInPolygon> preparedWays = new ArrayList<>();
 
+        // maps oldest way referring to start of each part
+        Map<Node, Way> oldestWayMap = new HashMap<>();
+
         for (Way way : outerStartingWays) {
             List<Way> splitWays = splitWayOnNodes(way, nodes);
+
+            // see #9599
+            if (!way.isNew() && splitWays.size() > 1) {
+                for (Way part : splitWays) {
+                    Node n = part.firstNode();
+                    Way old = oldestWayMap.get(n);
+                    if (old == null || old.getUniqueId() > way.getUniqueId()) {
+                        oldestWayMap.put(n, way);
+                    }
+                }
+            }
+
             preparedWays.addAll(markWayInsideSide(splitWays, false));
         }
@@ -721 +743 @@
 
         commitCommands(marktr("Delete relations"));
+
+        // see #9599: result should contain original way(s) where possible
+        if (discardedWays.stream().anyMatch(w -> !w.isNew())) {
+            for (int i = 0; i < polygons.size(); i++) {
+                Multipolygon mp = polygons.get(i);
+                for (int k = 0; k < mp.getInnerWays().size(); k++) {
+                    Way inner = mp.getInnerWays().get(k);
+                    Way older = keepOlder(inner, oldestWayMap, discardedWays);
+                    if (inner != older) {
+                        mp.getInnerWays().set(k, older);
+                    }
+                }
+                Way older = keepOlder(mp.outerWay, oldestWayMap, discardedWays);
+                if (older != mp.outerWay) {
+                    Multipolygon mpNew = new Multipolygon(older);
+                    mpNew.innerWays.addAll(mp.getInnerWays());
+                    polygons.set(i, mpNew);
+                }
+            }
+            commitCommands(marktr("Keep older versions"));
+        }
 
         // Delete the discarded inner ways
@@ -740 +783 @@
 
         return new JoinAreasResult(true, polygons);
+    }
+
+    /**
+     * Create copy of given way using an older id so that we don't create a new way instead of a modified old one.
+     * @param way the way to check
+     * @param oldestWayMap  nodes from old ways
+     * @param discardedWays collection of ways which will be deleted (modified)
+     * @return a copy of the way with an older id or the way itself
+     */
+    private Way keepOlder(Way way, Map<Node, Way> oldestWayMap, List<Way> discardedWays) {
+        Way oldest = null;
+        for (Node n : way.getNodes()) {
+            Way orig = oldestWayMap .get(n);
+            if (orig != null && (oldest == null || oldest.getUniqueId() > orig.getUniqueId())
+                    && discardedWays.contains(orig)) {
+                oldest = orig;
+            }
+        }
+        if (oldest != null) {
+            discardedWays.remove(oldest);
+            discardedWays.add(way);
+            Way copy = new Way(oldest);
+            copy.setNodes(way.getNodes());
+            cmds.add(new ChangeCommand(oldest, copy));
+            return copy;
+        }
+        return way;
     }
 
@@ -1056 +1126 @@
      * @param way way to split
      * @param nodes split points
-     * @return list of split ways (or original ways if no splitting is done).
+     * @return list of split ways (or original way if no splitting is done).
      */
     private List<Way> splitWayOnNodes(Way way, Set<Node> nodes) {
@@ -1373 +1443 @@
         //TODO: ReverseWay and Combine way are really slow and we use them a lot here. This slows down large joins.
         List<Way> actionWays = new ArrayList<>(ways.size());
-
+        int oldestPos = 0;
+        Way oldest = ways.get(0).way;
         for (WayInPolygon way : ways) {
             actionWays.add(way.way);
+            if (oldest.isNew() || !way.way.isNew() && oldest.getUniqueId() > way.way.getUniqueId()) {
+                oldest = way.way;
+                oldestPos = actionWays.size() - 1;
+            }
 
             if (!way.insideToTheRight) {
@@ -1383 +1458 @@
             }
         }
+
+        // see #9599: Help CombineWayAction to use the oldest way
+        Collections.rotate(actionWays, actionWays.size() - oldestPos);
 
         Pair<Way, Command> result = CombineWayAction.combineWaysWorker(actionWays);

trunk/test/unit/org/openstreetmap/josm/actions/JoinAreasActionTest.java

@@ -52 +52 @@
 
     /**
+     * Non-regression test for bug #9599.
+     * @throws IOException if any I/O error occurs
+     * @throws IllegalDataException if OSM parsing fails
+     */
+    @Test
+    public void testTicket9599() throws IOException, IllegalDataException {
+        try (InputStream is = TestUtils.getRegressionDataStream(9599, "ex5.osm")) {
+            DataSet ds = OsmReader.parseDataSet(is, null);
+            Layer layer = new OsmDataLayer(ds, null, null);
+            MainApplication.getLayerManager().addLayer(layer);
+            try {
+                new JoinAreasAction(false).join(ds.getWays());
+                Collection<IPrimitive> found = SearchAction.searchAndReturn("type:way", SearchMode.replace);
+                assertEquals(1, found.size());
+                assertEquals(257786939, found.iterator().next().getUniqueId());
+            } finally {
+                // Ensure we clean the place before leaving, even if test fails.
+                MainApplication.getLayerManager().removeLayer(layer);
+            }
+        }
+    }
+
+    /**
+     * Non-regression test for bug #9599.
+     * @throws IOException if any I/O error occurs
+     * @throws IllegalDataException if OSM parsing fails
+     */
+    @Test
+    public void testTicket9599Simple() throws IOException, IllegalDataException {
+        try (InputStream is = TestUtils.getRegressionDataStream(9599, "three_old.osm")) {
+            DataSet ds = OsmReader.parseDataSet(is, null);
+            Layer layer = new OsmDataLayer(ds, null, null);
+            MainApplication.getLayerManager().addLayer(layer);
+            try {
+                new JoinAreasAction(false).join(ds.getWays());
+                Collection<IPrimitive> found = SearchAction.searchAndReturn("type:way", SearchMode.replace);
+                assertEquals(1, found.size());
+                assertEquals(31567319, found.iterator().next().getUniqueId());
+            } finally {
+                // Ensure we clean the place before leaving, even if test fails.
+                MainApplication.getLayerManager().removeLayer(layer);
+            }
+        }
+    }
+
+    /**
      * Non-regression test for bug #10511.
      * @throws IOException if any I/O error occurs