Index: src/org/openstreetmap/josm/actions/AlignInCircleAction.java =================================================================== --- src/org/openstreetmap/josm/actions/AlignInCircleAction.java (revision 17386) +++ src/org/openstreetmap/josm/actions/AlignInCircleAction.java (working copy) @@ -29,6 +29,7 @@ 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,6 +43,7 @@ * @author Petr Dlouhý * @author Teemu Koskinen * @author Alain Delplanque + * @author Gerd Petermann * * @since 146 */ @@ -136,8 +138,6 @@ * In all cases, selected nodes are fix, nodes with more than one referrers are fix * (first referrer is the selected way) *

- * Case 3: Only nodes are selected - * --> 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,45 +146,44 @@ */ public static Command buildCommand(DataSet ds) throws InvalidSelection { Collection sel = ds.getSelected(); - List nodes = new LinkedList<>(); + List selectedNodes = new LinkedList<>(); // fixNodes: All nodes for which the angle relative to center should not be modified - Set fixNodes = new HashSet<>(); List 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 fixNodes = new HashSet<>(); + EastNorth center = null; + double radius = 0; + final List 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 inside = new ArrayList<>(); List 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,32 +203,31 @@ } 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,6 +283,20 @@ return new SequenceCommand(tr("Align Nodes in Circle"), cmds); } + private static boolean validateGeometry(List 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,5 +355,4 @@ } return true; } - } Index: src/org/openstreetmap/josm/data/validation/tests/CrossingWays.java =================================================================== --- src/org/openstreetmap/josm/data/validation/tests/CrossingWays.java (revision 17386) +++ src/org/openstreetmap/josm/data/validation/tests/CrossingWays.java (working copy) @@ -26,6 +26,7 @@ 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,4 +469,17 @@ } } + /** + * 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(); + } } Index: test/data/regress/20041/circle-action.osm =================================================================== --- test/data/regress/20041/circle-action.osm (nonexistent) +++ test/data/regress/20041/circle-action.osm (working copy) @@ -0,0 +1,41 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Index: test/unit/org/openstreetmap/josm/actions/AlignInCircleActionTest.java =================================================================== --- test/unit/org/openstreetmap/josm/actions/AlignInCircleActionTest.java (revision 17386) +++ test/unit/org/openstreetmap/josm/actions/AlignInCircleActionTest.java (working copy) @@ -92,40 +92,6 @@ } /** - * 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,4 +115,13 @@ } } + /** + * 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(); + + } }