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) @@ -13,6 +13,8 @@ import java.util.LinkedList; import java.util.List; import java.util.Set; +import java.util.SortedMap; +import java.util.TreeMap; import java.util.stream.Collectors; import javax.swing.JOptionPane; @@ -29,6 +31,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 +45,7 @@ * @author Petr Dlouhý * @author Teemu Koskinen * @author Alain Delplanque + * @author Gerd Petermann * * @since 146 */ @@ -146,7 +150,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 +159,86 @@ 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.size() == 1 && !ways.get(0).isClosed()) { + // nodes on selected ways + List onWay = new ArrayList<>(); + if (!ways.isEmpty()) { + List 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 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 inside = new ArrayList<>(); - List 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); + try { + closedWay.addNode(w.firstNode()); + nodes = collectNodesAnticlockwise(Collections.singletonList(closedWay)); + } finally { + closedWay.setNodes(null); // see #19885 } - - 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); + } 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; + if (center == null) { + center = en0.getCenter(en1); + } } - - fixNodes.addAll(inside); - fixNodes.addAll(collectNodesWithExternReferrers(ways)); + 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 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 (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.")); } } @@ -285,6 +296,38 @@ return new SequenceCommand(tr("Align Nodes in Circle"), cmds); } + private static List sortByAngle(final List 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> 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::stream).collect(Collectors.toList()); + } + + 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 +346,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 +366,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,1033 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + 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; @@ -149,4 +152,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 < 80; 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 "); + } + } + } }