Ticket #10205: 10205-beta.patch

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

@@ -29 +29 @@
 import org.openstreetmap.josm.data.osm.Way;
 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;
 import org.openstreetmap.josm.tools.Geometry;
@@ -42 +43 @@
  * @author Petr Dlouhý
  * @author Teemu Koskinen
  * @author Alain Delplanque
+ * @author Gerd Petermann
  *
  * @since 146
  */
@@ -136 +138 @@
      * In all cases, selected nodes are fix, nodes with more than one referrers are fix
      * (first referrer is the selected way)
      * <p>
-     * Case 3: Only nodes are selected
-     * --&gt; Align these nodes, all are fix
      * @param ds data set in which the command operates
      * @return the resulting command to execute to perform action
      * @throws InvalidSelection if selection cannot be used
@@ -146 +146 @@
      */
     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<>();
         List<Way> ways = new LinkedList<>();
-        EastNorth center = null;
-        double radius = 0;
 
         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);
             }
         }
+        if (ways.isEmpty()) {
+            throw new InvalidSelection(tr("Please select at least one way."));
+        }
 
+        Set<Node> fixNodes = new HashSet<>();
+        EastNorth center = null;
+        double radius = 0;
+        final List<Node> nodes;
         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(selectedNodes);
             // 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)) {
+        } else if (checkWaysArePolygon(ways)) {
             // Case 2
+            // nodes on selected ways
             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)
+            for (Node n: selectedNodes) {
+                if (ways.stream().anyMatch(w -> w.containsNode(n)))
                     inside.add(n);
                 else
                     outside.add(n);
@@ -204 +203 @@
             }
 
             fixNodes.addAll(inside);
-            fixNodes.addAll(collectNodesWithExternReferrers(ways));
             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));
 
+        if (nodes.size() < 4) {
+            throw new InvalidSelection(tr("Not enough nodes in selected ways."));
+        }
         // Check if one or more nodes are outside of download area
         if (nodes.stream().anyMatch(Node::isOutsideDownloadArea))
             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 (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."));
             }
         }
+        Logging.debug("center: {0}", new Node(center).getCoor().toString());
 
         // Now calculate the average distance to each node from the
         // center. This method is ok as long as distances are short
@@ -285 +283 @@
         return new SequenceCommand(tr("Align Nodes in Circle"), cmds);
     }
 
+    private static boolean validateGeometry(List<Node> nodes) {
+        Way test = new Way();
+        try {
+            test.setNodes(nodes);
+            if (!test.isClosed())
+                test.addNode(test.firstNode());
+            if (CrossingWays.isSelfCrossing(test))
+                return false;
+            return !SelfIntersectingWay.isSelfIntersecting(test);
+        } finally {
+            test.setNodes(null); // see #19855
+        }
+    }
+
     /**
      * Collect all nodes with more than one referrer.
      * @param ways Ways from witch nodes are selected
@@ -343 +355 @@
         }
         return true;
     }
-
 }

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

@@ -26 +26 @@
 import org.openstreetmap.josm.data.validation.TestError;
 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;
 
 /**
@@ -468 +469 @@
         }
     }
 
+    /**
+     * 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();
+    }
 }

test/data/regress/20041/circle-action.osm

@@ +1 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<osm version='0.6' generator='JOSM'>
+  <node id='3353142168' action='modify' timestamp='2015-02-15T23:33:27Z' uid='715371' user='715371' visible='true' version='1' changeset='28875323' lat='52.90029704945' lon='8.44321406009' />
+  <node id='3353142170' action='modify' timestamp='2015-02-15T23:33:27Z' uid='715371' user='715371' visible='true' version='1' changeset='28875323' lat='52.90031807489' lon='8.44326193915' />
+  <node id='3353142171' action='modify' timestamp='2015-02-15T23:33:27Z' uid='715371' user='715371' visible='true' version='1' changeset='28875323' lat='52.90030468523' lon='8.44306677855' />
+  <node id='3353142174' action='modify' timestamp='2015-02-15T23:33:27Z' uid='715371' user='715371' visible='true' version='1' changeset='28875323' lat='52.90033729149' lon='8.44301884234' />
+  <node id='3353142175' action='modify' timestamp='2015-02-15T23:33:27Z' uid='715371' user='715371' visible='true' version='1' changeset='28875323' lat='52.90034976283' lon='8.44329321571' />
+  <node id='3353142183' action='modify' timestamp='2015-02-15T23:33:27Z' uid='715371' user='715371' visible='true' version='1' changeset='28875323' lat='52.90040118461' lon='8.44329685665' />
+  <node id='3353142184' action='modify' timestamp='2015-02-15T23:33:27Z' uid='715371' user='715371' visible='true' version='1' changeset='28875323' lat='52.9004078959' lon='8.44300955386' />
+  <node id='3353142189' action='modify' timestamp='2015-02-15T23:33:27Z' uid='715371' user='715371' visible='true' version='1' changeset='28875323' lat='52.90046511808' lon='8.44319779138' />
+  <node id='3353142190' action='modify' timestamp='2015-02-15T23:33:27Z' uid='715371' user='715371' visible='true' version='1' changeset='28875323' lat='52.90044790476' lon='8.44305434646' />
+  <node id='3353142192' action='modify' timestamp='2015-02-15T23:33:27Z' uid='715371' user='715371' visible='true' version='1' changeset='28875323' lat='52.90046284581' lon='8.44309505092' />
+  <way id='-104527' action='modify' visible='true'>
+    <nd ref='3353142171' />
+    <nd ref='3353142168' />
+    <nd ref='3353142170' />
+    <nd ref='3353142175' />
+    <nd ref='3353142183' />
+    <nd ref='3353142189' />
+    <nd ref='3353142192' />
+    <nd ref='3353142190' />
+    <nd ref='3353142184' />
+    <nd ref='3353142174' />
+    <nd ref='3353142171' />
+    <tag k='barrier' v='fence' />
+  </way>
+  <way id='328483730' timestamp='2015-02-15T23:33:35Z' uid='715371' user='715371' visible='true' version='1' changeset='28875323'>
+    <nd ref='3353142174' />
+    <nd ref='3353142184' />
+    <nd ref='3353142190' />
+    <nd ref='3353142192' />
+    <nd ref='3353142189' />
+    <nd ref='3353142183' />
+    <nd ref='3353142175' />
+    <nd ref='3353142170' />
+    <nd ref='3353142168' />
+    <nd ref='3353142171' />
+    <nd ref='3353142174' />
+    <tag k='natural' v='water' />
+  </way>
+</osm>

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

@@ -92 +92 @@
     }
 
     /**
-     * Test case: way with several nodes selected
-     * @throws Exception if an error occurs
-     */
-    @Test
-    void testNodesSelected() throws Exception {
-        DataSet ds = OsmReader.parseDataSet(Files.newInputStream(Paths.get(TestUtils.getTestDataRoot(), "alignCircleBefore.osm")), null);
-        DataSet ds2 = OsmReader.parseDataSet(Files.newInputStream(Paths.get(TestUtils.getTestDataRoot(), "alignCircleAfter2.osm")), null);
-
-        Way circularWay = null;
-        for (Way w : ds.getWays()) {
-            if ("roundabout".equals(w.get("junction"))) {
-                circularWay = w;
-                break;
-            }
-        }
-        assertNotNull(circularWay);
-        if (circularWay != null) {
-            ds.setSelected(circularWay.getNodes());
-            Command c = AlignInCircleAction.buildCommand(ds);
-            assertNotNull(c);
-            c.executeCommand();
-            Way expected = (Way) ds2.getPrimitiveById(circularWay);
-            assertNotNull(expected);
-            assertEquals(expected, circularWay);
-            assertEquals(expected.getNodesCount(), circularWay.getNodesCount());
-            for (Node n1 : circularWay.getNodes()) {
-                Node n2 = (Node) ds2.getPrimitiveById(n1);
-                assertEquals(n1.lat(), n2.lat(), 1e-5);
-                assertEquals(n1.lon(), n2.lon(), 1e-5);
-            }
-        }
-    }
-
-    /**
      * Test case: original roundabout was split, two ways selected, they build a closed ring
      * @throws Exception if an error occurs
      */
@@ -149 +115 @@
         }
     }
 
+    /**
+     * Test case: original roundabout was split, two ways selected, they build a closed ring
+     * @throws Exception if an error occurs
+     */
+    @Test
+    void testPShapedWay() throws Exception {
+        DataSet ds = new DataSet();
+
+    }
 }