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 */ @@ -146,7 +148,7 @@ */ 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<>(); @@ -155,79 +157,81 @@ 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); } } + 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)) { + 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 + + // 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); } + if (outside.size() == 1) { + // center is given + center = outside.get(0).getEastNorth(); + if (inside.size() == 1) { + radius = center.distance(inside.get(0).getEastNorth()); + } + } else if (outside.size() > 1) { + throw new InvalidSelection(); + } - 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()) { + if (inside.size() == 2) { // 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); radius = en0.distance(en1) / 2; + if (center == null) { + center= en0.getCenter(en1); + } } - 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.")); } } @@ -285,6 +289,22 @@ return new SequenceCommand(tr("Align Nodes in Circle"), cmds); } + private static boolean validateGeometry(List 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 + } + } + /** * Collect all nodes with more than one referrer. * @param ways Ways from witch nodes are selected @@ -303,9 +323,9 @@ private static List collectNodesAnticlockwise(List ways) throws InvalidSelection { Collection rings = Multipolygon.joinWays(ways); if (rings.size() != 1) - throw new InvalidSelection(); + throw new InvalidSelection(); // we should never get here List 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)) Collections.reverse(nodes); @@ -323,25 +343,4 @@ protected void updateEnabledState(Collection selection) { 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 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; - } - } 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/alignCircleCases.osm =================================================================== --- test/data/alignCircleCases.osm (nonexistent) +++ test/data/alignCircleCases.osm (working copy) @@ -0,0 +1,956 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + 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) @@ -4,6 +4,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.nio.file.Files; import java.nio.file.Paths; @@ -13,9 +14,11 @@ import org.junit.jupiter.api.Test; 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; @@ -92,40 +95,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 +118,29 @@ } } + /** + * 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 < 60; i++) { + final String selVal = Integer.toString(i); + Set 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 { + assertNotNull(AlignInCircleAction.buildCommand(ds), "sel=" + selVal); + assertTrue (selValid, "sel=" + selVal + " should be valid "); + } catch (InvalidSelection e) { + assertTrue (!selValid, "sel=" + selVal + " should be invalid "); + } + } + } }