Changeset 17393 in josm

Timestamp:

2020-12-07T11:52:15+01:00 (4 years ago)

Author:

GerdP

Message:

see #10205: Strange align nodes in circle behavior

• allow to define center node also when a single unclosed way is selected
• add robustness and more unit tests for evaluation of valid selections
• if only nodes and no way are selected, order the nodes using the angle to the agv. east-north position. This should produce a predictable result
• if way(s) are selected the order the nodes is determined by the occurence in the way(s). Self-Intersecting polygons are rejected if no center node is given
• don't throw InvalidSelection when selection is valid but no point was moved, let buildCommand() return null instead

With a selection that gives a center point and way(s) which are not even close to a circular shape the result might still be surprising. Sometimes the way nodes are arranged around the center node, sometimes they are moved so that a circle arc with nearly the same length is produced. The result changes significantly when the way nodes are also selected. Subject to further improvements.

Location:

trunk

Files:

• src/org/openstreetmap/josm/actions/AlignInCircleAction.java (modified) (13 diffs)
• src/org/openstreetmap/josm/data/validation/tests/CrossingWays.java (modified) (2 diffs)
• test/data/alignCircleCases.osm (added)
• test/data/regress/20041 (added)
• test/data/regress/20041/circle-action.osm (added)
• test/unit/org/openstreetmap/josm/actions/AlignInCircleActionTest.java (modified) (5 diffs)

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

@@ -14 +14 @@
 import java.util.List;
 import java.util.Set;
+import java.util.SortedMap;
+import java.util.TreeMap;
 import java.util.stream.Collectors;
 
@@ -30 +32 @@
 import org.openstreetmap.josm.data.osm.visitor.paint.relations.Multipolygon;
 import org.openstreetmap.josm.data.osm.visitor.paint.relations.Multipolygon.JoinedWay;
+import org.openstreetmap.josm.data.validation.tests.CrossingWays;
 import org.openstreetmap.josm.data.validation.tests.SelfIntersectingWay;
 import org.openstreetmap.josm.gui.Notification;
@@ -43 +46 @@
  * @author Teemu Koskinen
  * @author Alain Delplanque
+ * @author Gerd Petermann
  *
  * @since 146
@@ -107 +111 @@
         try {
             Command cmd = buildCommand(getLayerManager().getEditDataSet());
-            UndoRedoHandler.getInstance().add(cmd);
+            if (cmd != null)
+                UndoRedoHandler.getInstance().add(cmd);
+            else {
+                new Notification(tr("nothing changed"))
+                .setIcon(JOptionPane.INFORMATION_MESSAGE)
+                .setDuration(Notification.TIME_SHORT)
+                .show();
+
+            }
         } catch (InvalidSelection except) {
             Logging.debug(except);
@@ -115 +127 @@
                 .show();
         }
-
     }
 
@@ -140 +151 @@
      * --> Align these nodes, all are fix
      * @param ds data set in which the command operates
-     * @return the resulting command to execute to perform action
+     * @return the resulting command to execute to perform action, or null if nothing was changed
      * @throws InvalidSelection if selection cannot be used
      * @since 17386
@@ -147 +158 @@
     public static Command buildCommand(DataSet ds) throws InvalidSelection {
         Collection<OsmPrimitive> sel = ds.getSelected();
-        List<Node> nodes = new LinkedList<>();
+        List<Node> selectedNodes = new LinkedList<>();
         // fixNodes: All nodes for which the angle relative to center should not be modified
         Set<Node> fixNodes = new HashSet<>();
@@ -156 +167 @@
         for (OsmPrimitive osm : sel) {
             if (osm instanceof Node) {
-                nodes.add((Node) osm);
+                selectedNodes.add((Node) osm);
             } else if (osm instanceof Way) {
                 ways.add((Way) osm);
@@ -162 +173 @@
         }
 
-        if (ways.size() == 1 && !ways.get(0).isClosed()) {
+        // nodes on selected ways
+        List<Node> onWay = new ArrayList<>();
+        if (!ways.isEmpty()) {
+            List<Node> potentialCenter = new ArrayList<>();
+            for (Node n : selectedNodes) {
+                if (ways.stream().anyMatch(w -> w.containsNode(n))) {
+                    onWay.add(n);
+                } else {
+                    potentialCenter.add(n);
+                }
+            }
+            if (potentialCenter.size() == 1) {
+                // center is given
+                center = potentialCenter.get(0).getEastNorth();
+                if (onWay.size() == 1) {
+                    radius = center.distance(onWay.get(0).getEastNorth());
+                }
+            } else if (potentialCenter.size() > 1) {
+                throw new InvalidSelection(tr("Please select only one node as center."));
+            }
+
+        }
+
+        final List<Node> nodes;
+        if (ways.isEmpty()) {
+            nodes = sortByAngle(selectedNodes);
+            fixNodes.addAll(nodes);
+        } else if (ways.size() == 1 && !ways.get(0).isClosed()) {
             // Case 1
             Way w = ways.get(0);
-            if (SelfIntersectingWay.isSelfIntersecting(w)) {
-                throw new InvalidSelection(tr("Self-intersecting way"));
-            }
-
             fixNodes.add(w.firstNode());
             fixNodes.add(w.lastNode());
-            fixNodes.addAll(nodes);
-            fixNodes.addAll(collectNodesWithExternReferrers(ways));
+            fixNodes.addAll(onWay);
             // Temporary closed way used to reorder nodes
             Way closedWay = new Way(w);
-            closedWay.addNode(w.firstNode());
-            nodes = collectNodesAnticlockwise(Collections.singletonList(closedWay));
-            closedWay.setNodes(null); // see #19885
-        } else if (!ways.isEmpty() && checkWaysArePolygon(ways)) {
-            // Case 2
-            List<Node> inside = new ArrayList<>();
-            List<Node> outside = new ArrayList<>();
-
-            for (Node n: nodes) {
-                boolean isInside = ways.stream().anyMatch(w -> w.getNodes().contains(n));
-                if (isInside)
-                    inside.add(n);
-                else
-                    outside.add(n);
-            }
-
-            if (outside.size() == 1 && inside.isEmpty()) {
-                center = outside.get(0).getEastNorth();
-            } else if (outside.size() == 1 && inside.size() == 1) {
-                center = outside.get(0).getEastNorth();
-                radius = center.distance(inside.get(0).getEastNorth());
-            } else if (inside.size() == 2 && outside.isEmpty()) {
-                // 2 nodes inside, define diameter
-                EastNorth en0 = inside.get(0).getEastNorth();
-                EastNorth en1 = inside.get(1).getEastNorth();
-                center = new EastNorth((en0.east() + en1.east()) / 2, (en0.north() + en1.north()) / 2);
+            try {
+                closedWay.addNode(w.firstNode());
+                nodes = collectNodesAnticlockwise(Collections.singletonList(closedWay));
+            } finally {
+                closedWay.setNodes(null); // see #19885
+            }
+        } else if (Multipolygon.joinWays(ways).size() == 1) {
+            // Case 2:
+            if (onWay.size() == 2) {
+                // 2 way nodes define diameter
+                EastNorth en0 = onWay.get(0).getEastNorth();
+                EastNorth en1 = onWay.get(1).getEastNorth();
                 radius = en0.distance(en1) / 2;
-            }
-
-            fixNodes.addAll(inside);
-            fixNodes.addAll(collectNodesWithExternReferrers(ways));
+                if (center == null) {
+                    center = en0.getCenter(en1);
+                }
+            }
+            fixNodes.addAll(onWay);
             nodes = collectNodesAnticlockwise(ways);
-            if (nodes.size() < 4) {
-                throw new InvalidSelection(tr("Not enough nodes in selected ways."));
-            }
-        } else if (ways.isEmpty() && nodes.size() > 3) {
-            // Case 3
-            fixNodes.addAll(nodes);
-            // No need to reorder nodes since all are fix
         } else {
-            if (ways.isEmpty() && nodes.size() <= 3)
-                throw new InvalidSelection(tr("Please select at least four nodes."));
             throw new InvalidSelection();
         }
+        fixNodes.addAll(collectNodesWithExternReferrers(ways));
 
         // Check if one or more nodes are outside of download area
@@ -224 +236 @@
             throw new InvalidSelection(tr("One or more nodes involved in this action is outside of the downloaded area."));
 
+
         if (center == null) {
-            // Compute the center of nodes
-            center = Geometry.getCenter(nodes);
+            if (nodes.size() < 4) {
+                throw new InvalidSelection(tr("Not enough nodes to calculate center."));
+            }
+            if (validateGeometry(nodes)) {
+                // Compute the center of nodes
+                center = Geometry.getCenter(nodes);
+            }
             if (center == null) {
-                throw new InvalidSelection(tr("Cannot determine center of selected nodes."));
+                throw new InvalidSelection(tr("Cannot determine center of circle for this geometry."));
             }
         }
@@ -282 +300 @@
         }
         if (cmds.isEmpty())
-            throw new InvalidSelection(tr("nothing changed"));
+            return null;
         return new SequenceCommand(tr("Align Nodes in Circle"), cmds);
+    }
+
+    private static List<Node> sortByAngle(final List<Node> nodes) {
+        EastNorth sum = new EastNorth(0, 0);
+        for (Node n : nodes) {
+            EastNorth en = n.getEastNorth();
+            sum = sum.add(en.east(), en.north());
+        }
+        final EastNorth simpleCenter = new EastNorth(sum.east()/nodes.size(), sum.north()/nodes.size());
+
+        SortedMap<Double, List<Node>> orderedMap = new TreeMap<>();
+        for (Node n : nodes) {
+            double angle = new PolarCoor(n.getEastNorth(), simpleCenter).angle;
+            orderedMap.computeIfAbsent(angle, k-> new ArrayList<>()).add(n);
+        }
+        return orderedMap.values().stream().flatMap(List<Node>::stream).collect(Collectors.toList());
+    }
+
+    private static boolean validateGeometry(List<Node> nodes) {
+        Way test = new Way();
+        test.setNodes(nodes);
+        if (!test.isClosed()) {
+            test.addNode(test.firstNode());
+        }
+
+        try {
+            if (CrossingWays.isSelfCrossing(test))
+                return false;
+            return !SelfIntersectingWay.isSelfIntersecting(test);
+        } finally {
+            test.setNodes(null); // see #19855
+        }
     }
 
@@ -304 +354 @@
         Collection<JoinedWay> rings = Multipolygon.joinWays(ways);
         if (rings.size() != 1)
-            throw new InvalidSelection();
+            throw new InvalidSelection(); // we should never get here
         List<Node> nodes = new ArrayList<>(rings.iterator().next().getNodes());
-        if (nodes.get(0) != nodes.get(nodes.size()-1))
+        if (nodes.get(0) != nodes.get(nodes.size() - 1))
             throw new InvalidSelection();
         if (Geometry.isClockwise(nodes))
@@ -324 +374 @@
         updateEnabledStateOnModifiableSelection(selection);
     }
-
-    /**
-     * Determines if ways can be joined into a single polygon.
-     * @param ways The ways collection to check
-     * @return true if all ways can be joined into a single polygon
-     */
-    private static boolean checkWaysArePolygon(Collection<Way> ways) {
-        if (Multipolygon.joinWays(ways).size() != 1)
-            return false;
-        // For each way, nodes strictly between first and last should't be reference by an other way
-        for (Way way: ways) {
-            for (Node node: way.getNodes()) {
-                if (way.isFirstLastNode(node)) continue;
-                if (ways.stream().filter(wayOther -> way != wayOther).anyMatch(wayOther -> node.getReferrers().contains(wayOther))) {
-                    return false;
-                }
-            }
-        }
-        return true;
-    }
-
 }

trunk/src/org/openstreetmap/josm/data/validation/tests/CrossingWays.java

@@ -27 +27 @@
 import org.openstreetmap.josm.data.validation.util.ValUtil;
 import org.openstreetmap.josm.gui.progress.ProgressMonitor;
+import org.openstreetmap.josm.tools.CheckParameterUtil;
 import org.openstreetmap.josm.tools.Logging;
 
@@ -469 +470 @@
     }
 
+    /**
+     * Check if the given way is self crossing
+     * @param way the way to check
+     * @return {@code true} if one or more segments of the way are crossing
+     * @see SelfIntersectingWay
+     * @since xxx
+     */
+    public static boolean isSelfCrossing(Way way) {
+        CheckParameterUtil.ensureParameterNotNull(way, "way");
+        SelfCrossing test = new SelfCrossing();
+        test.visit(way);
+        return !test.getErrors().isEmpty();
+    }
 }

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

@@ -3 +3 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.nio.file.Files;
@@ -14 +16 @@
 import org.junit.jupiter.api.extension.RegisterExtension;
 import org.openstreetmap.josm.TestUtils;
+import org.openstreetmap.josm.actions.AlignInCircleAction.InvalidSelection;
 import org.openstreetmap.josm.command.Command;
 import org.openstreetmap.josm.data.osm.DataSet;
 import org.openstreetmap.josm.data.osm.Node;
+import org.openstreetmap.josm.data.osm.OsmPrimitive;
 import org.openstreetmap.josm.data.osm.Way;
 import org.openstreetmap.josm.io.OsmReader;
 import org.openstreetmap.josm.testutils.JOSMTestRules;
+import org.opentest4j.AssertionFailedError;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
@@ -71 +76 @@
     /**
      * Non-regression test for <a href="https://josm.openstreetmap.de/ticket/20041">Bug #20041</a>.
-     * Don't create move commands when no node is visibly moved
+     * Don't create move commands when no node is visibly moved.
      * @throws Exception if an error occurs
      */
@@ -88 +93 @@
         if (roundabout != null) {
             ds.setSelected(roundabout);
-            assertThrows(AlignInCircleAction.InvalidSelection.class, () -> AlignInCircleAction.buildCommand(ds));
+            assertNull(AlignInCircleAction.buildCommand(ds));
         }
     }
@@ -150 +155 @@
     }
 
+    /**
+     * Various cases of selections in file
+     * @throws Exception if an error occurs
+     */
+    @Test
+    void testSelectionEvaluation() throws Exception {
+        DataSet ds = OsmReader.parseDataSet(
+                Files.newInputStream(Paths.get(TestUtils.getTestDataRoot(), "alignCircleCases.osm")), null);
+
+        for (int i = 0; i < 80; i++) {
+            final String selVal = Integer.toString(i);
+            Set<OsmPrimitive> sel = ds.allPrimitives().stream().filter(p -> p.hasTag("sel", selVal))
+                    .collect(Collectors.toSet());
+            if (sel.isEmpty())
+                continue;
+            ds.setSelected(sel);
+            boolean selValid = sel.stream().noneMatch(p -> p.hasKey("invalid-selection"));
+            try {
+                AlignInCircleAction.buildCommand(ds);
+                assertTrue(selValid, "sel=" + selVal + " is not valid?");
+            } catch (InvalidSelection e) {
+                assertFalse(selValid, "sel=" + selVal + " is not invalid?");
+            } catch (Exception e) {
+                throw new AssertionFailedError("test failed: sel=" + selVal,e);
+            }
+        }
+    }
 }