From 90896db0553d849428c5bfbfbb65b49596b6227c Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Sun, 8 Oct 2023 14:16:04 +0200 Subject: [PATCH 01/10] Apply patch without modification from https://josm.openstreetmap.de/attachment/ticket/22799/josm22799_smart_distribute_v2.patch --- .../josm/actions/DistributeAction.java | 67 ++++++++++++++++--- 1 file changed, 58 insertions(+), 9 deletions(-) diff --git a/src/org/openstreetmap/josm/actions/DistributeAction.java b/src/org/openstreetmap/josm/actions/DistributeAction.java index 75f2c766814..2ecf6b0d6bb 100644 --- a/src/org/openstreetmap/josm/actions/DistributeAction.java +++ b/src/org/openstreetmap/josm/actions/DistributeAction.java @@ -6,6 +6,7 @@ import java.awt.event.ActionEvent; import java.awt.event.KeyEvent; +import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; import java.util.Iterator; @@ -49,7 +50,9 @@ public DistributeAction() { * Select method according to user selection. * Case 1: One Way (no self-crossing) and at most 2 nodes contains by this way: * Distribute nodes keeping order along the way - * Case 2: Other + * Case 2: One Node part of at least one way, not a start or end node + * Distribute the selected node relative to neighbors + * Case 3: Other * Distribute nodes */ @Override @@ -59,8 +62,8 @@ public void actionPerformed(ActionEvent e) { // Collect user selected objects Collection selected = getLayerManager().getEditDataSet().getSelected(); - Collection ways = new LinkedList<>(); - Collection nodes = new HashSet<>(); + Collection ways = new ArrayList<>(); + Collection nodes = new ArrayList<>(); for (OsmPrimitive osm : selected) { if (osm instanceof Node) { nodes.add((Node) osm); @@ -81,13 +84,15 @@ public void actionPerformed(ActionEvent e) { cmds = distributeWay(ways, nodes); } else if (checkDistributeNodes(ways, nodes)) { cmds = distributeNodes(nodes); + } else if (checkDistributeNode(nodes)){ + cmds = distributeNode(nodes); } else { new Notification( - tr("Please select :\n" + - "* One no self-crossing way with at most two of its nodes;\n" + - "* Three nodes.")) + tr("Please select:
    " + + "
  • One no self-crossing way with at most two of its nodes;
    • " + + "
  • One node in the middle of a way;
    • " + + "
  • Three nodes.
")) .setIcon(JOptionPane.INFORMATION_MESSAGE) - .setDuration(Notification.TIME_SHORT) .show(); return; } @@ -184,6 +189,50 @@ private static Collection distributeWay(Collection ways, return cmds; } + /** + * Test if single node oriented algorithm applies to the selection. + * @param nodes The selected node. Collection type and naming kept for compatibility with similar methods. + * @return true in this case + */ + private static boolean checkDistributeNode(Collection nodes) { + if (nodes.size() == 1) { + Node node = nodes.iterator().next(); + int goodWays = 0; + for (Way way : node.getParentWays()) { + // the algorithm is applicable only if there is one way which: + // - is open and the selected node is a middle node, or + // - is closed and has at least 4 nodes (as 3 doesn't make sense and error-prone) + if (!way.isFirstLastNode(node) || (way.isClosed() && way.getRealNodesCount() > 3)) + goodWays++; + } + return goodWays == 1; + } + return false; + } + + /** + * Distribute a single node relative to way neighbours. + * @see DistributeAction#distributeNodes(Collection) + * @param nodes nodes to distribute + * @return Commands to execute to perform action + */ + private static Collection distributeNode(Collection nodes) { + final Node node = nodes.iterator().next(); + Way parent = node.getParentWays().iterator().next(); + + // make the user selected node the middle in the output variable + nodes.clear(); + + List neighbours = new ArrayList<>(parent.getNeighbours(node)); + + nodes.add(neighbours.get(0)); + nodes.add(node); + nodes.add(neighbours.get(1)); + + // call the distribution method with 3 nodes + return distributeNodes(nodes); + } + /** * Test if nodes oriented algorithm applies to the selection. * @param ways Selected ways @@ -197,7 +246,7 @@ private static boolean checkDistributeNodes(Collection ways, Collection distributeNodes(Collection nodes) { // A list of commands to do Collection cmds = new LinkedList<>(); - // Amount of nodes between A and B plus 1 + // Number of nodes between A and B plus 1 int num = nodes.size()+1; // Current number of node From 4e8bef4a50d201e44246f55eb867c28d391a20f7 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Sun, 8 Oct 2023 15:57:29 +0200 Subject: [PATCH 02/10] Add unit test for DistributeAction --- .../josm/actions/DistributeActionTest.java | 132 ++++++++++++++++++ 1 file changed, 132 insertions(+) create mode 100644 test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java diff --git a/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java b/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java new file mode 100644 index 00000000000..03c0db4a425 --- /dev/null +++ b/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java @@ -0,0 +1,132 @@ +package org.openstreetmap.josm.actions; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.openstreetmap.josm.data.coor.LatLon; +import org.openstreetmap.josm.data.osm.DataSet; +import org.openstreetmap.josm.data.osm.Node; +import org.openstreetmap.josm.data.osm.Way; +import org.openstreetmap.josm.gui.MainApplication; +import org.openstreetmap.josm.gui.layer.OsmDataLayer; +import org.openstreetmap.josm.testutils.annotations.Main; +import org.openstreetmap.josm.testutils.annotations.Projection; + +import java.util.Random; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +/** + * Unit tests for class {@link DistributeAction}. + */ +@Main +@Projection +class DistributeActionTest { + + private static final Random random = new Random(); + private DataSet ds = new DataSet(); + + @BeforeEach + void setUp() { + ds = new DataSet(); + } + + @Test + void testWholeWayAlignment() { + Way way = new Way(); + final int totalNodeCount = 11; // should be in range [2,180]! + final int innerNodeCount = totalNodeCount - 2; + final int lastLon = totalNodeCount - 1; + + // add first node + Node n = new Node(new LatLon(LatLon.ZERO)); + ds.addPrimitive(n); + way.addNode(n); + + // add interim nodes + for (int i = 0; i < innerNodeCount; i++) { + n = new Node(new LatLon(0, getRandomDoubleInRange(0, lastLon))); + ds.addPrimitive(n); + way.addNode(n); + } + + // add last node + n = new Node(new LatLon(0, lastLon)); + ds.addPrimitive(n); + way.addNode(n); + ds.addPrimitive(way); + + + OsmDataLayer layer = new OsmDataLayer(ds, "", null); + MainApplication.getLayerManager().addLayer(layer); + assertNotNull(MainApplication.getLayerManager().getActiveLayer()); + + // select the way + layer.getDataSet().setSelected(way.getPrimitiveId()); + assertEquals(1, layer.getDataSet().getSelected().size()); + + new DistributeAction().actionPerformed(null); + + for (int i = 0; i < totalNodeCount; i++) { + assertEquals( + (double) (1 / lastLon) + i, + way.getNode(i).lon(), + 1e-7 + ); + } + } + + @Test + void testSingleNodeAlignment() { + Way way = new Way(); + final int totalNodeCount = 11; // should be in range [3,180]! + final int lastLon = totalNodeCount - 1; + + Node n; + Node selectedNode = null; + + // add nodes except the last one + for (int i = 0; i < totalNodeCount - 1; i++) { + if (i == 1) { + n = new Node(new LatLon(0, 0.1)); + selectedNode = n; + } else { + n = new Node(new LatLon(0, i)); + } + ds.addPrimitive(n); + way.addNode(n); + } + + // add last node + n = new Node(new LatLon(0, lastLon)); + ds.addPrimitive(n); + way.addNode(n); + ds.addPrimitive(way); + + + OsmDataLayer layer = new OsmDataLayer(ds, "", null); + MainApplication.getLayerManager().addLayer(layer); + assertNotNull(MainApplication.getLayerManager().getActiveLayer()); + + // select a single node + layer.getDataSet().setSelected(selectedNode.getPrimitiveId()); + assertEquals(1, layer.getDataSet().getSelected().size()); + + new DistributeAction().actionPerformed(null); + + for (int i = 0; i < totalNodeCount; i++) { + assertEquals( + (double) (1 / lastLon) + i, + way.getNode(i).lon(), + 1e-7 + ); + } + } + + private static double getRandomDoubleInRange(double min, double max) { + if (min >= max) { + throw new IllegalArgumentException("Invalid range. Max must be greater than min."); + } + return min + (max - min) * random.nextDouble(); + } +} \ No newline at end of file From 5face9f08d1105005b3ba5750e82fe5ec7199097 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Sun, 8 Oct 2023 16:49:00 +0200 Subject: [PATCH 03/10] Add single node unit test for DistributeAction It is failing, since the DistributeAction works that way by design, but it can cause overlapping way issues. Not sure what to do. --- .../josm/actions/DistributeActionTest.java | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java b/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java index 03c0db4a425..9203394fb7a 100644 --- a/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java +++ b/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java @@ -76,6 +76,51 @@ void testWholeWayAlignment() { } } + @Test + void testNodesAlignment() { + Way way = new Way(); + final int totalNodeCount = 11; // should be in range [2,180]! + final int innerNodeCount = totalNodeCount - 2; + final int lastLon = totalNodeCount - 1; + + // add first node + Node n = new Node(new LatLon(LatLon.ZERO)); + ds.addPrimitive(n); + way.addNode(n); + + // add interim nodes + for (int i = 0; i < innerNodeCount; i++) { + n = new Node(new LatLon(0, getRandomDoubleInRange(0, lastLon))); + ds.addPrimitive(n); + way.addNode(n); + } + + // add last node + n = new Node(new LatLon(0, lastLon)); + ds.addPrimitive(n); + way.addNode(n); + ds.addPrimitive(way); + + + OsmDataLayer layer = new OsmDataLayer(ds, "", null); + MainApplication.getLayerManager().addLayer(layer); + assertNotNull(MainApplication.getLayerManager().getActiveLayer()); + + // select all nodes on the way + layer.getDataSet().setSelected(way.getNodes()); + assertEquals(way.getNodes().size(), layer.getDataSet().getSelected().size()); + + new DistributeAction().actionPerformed(null); + + for (int i = 0; i < totalNodeCount; i++) { + assertEquals( + (double) (1 / lastLon) + i, + way.getNode(i).lon(), + 1e-7 + ); + } + } + @Test void testSingleNodeAlignment() { Way way = new Way(); From 09ec5ca636096e01cf114c5d07698378cb2de8d5 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Thu, 12 Oct 2023 21:03:04 +0200 Subject: [PATCH 04/10] Use List interface instead of Collection --- .../josm/actions/DistributeAction.java | 28 ++++++++----------- .../josm/actions/DistributeActionTest.java | 21 ++++++++++++++ 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/src/org/openstreetmap/josm/actions/DistributeAction.java b/src/org/openstreetmap/josm/actions/DistributeAction.java index 2ecf6b0d6bb..d70d63e3378 100644 --- a/src/org/openstreetmap/josm/actions/DistributeAction.java +++ b/src/org/openstreetmap/josm/actions/DistributeAction.java @@ -62,8 +62,8 @@ public void actionPerformed(ActionEvent e) { // Collect user selected objects Collection selected = getLayerManager().getEditDataSet().getSelected(); - Collection ways = new ArrayList<>(); - Collection nodes = new ArrayList<>(); + List ways = new ArrayList<>(); + List nodes = new ArrayList<>(); for (OsmPrimitive osm : selected) { if (osm instanceof Node) { nodes.add((Node) osm); @@ -194,9 +194,9 @@ private static Collection distributeWay(Collection ways, * @param nodes The selected node. Collection type and naming kept for compatibility with similar methods. * @return true in this case */ - private static boolean checkDistributeNode(Collection nodes) { + private static boolean checkDistributeNode(List nodes) { if (nodes.size() == 1) { - Node node = nodes.iterator().next(); + Node node = nodes.get(0); int goodWays = 0; for (Way way : node.getParentWays()) { // the algorithm is applicable only if there is one way which: @@ -213,24 +213,20 @@ private static boolean checkDistributeNode(Collection nodes) { /** * Distribute a single node relative to way neighbours. * @see DistributeAction#distributeNodes(Collection) - * @param nodes nodes to distribute + * @param nodes a single node in a collection to distribute * @return Commands to execute to perform action */ - private static Collection distributeNode(Collection nodes) { - final Node node = nodes.iterator().next(); - Way parent = node.getParentWays().iterator().next(); - - // make the user selected node the middle in the output variable - nodes.clear(); + private static Collection distributeNode(List nodes) { + final Node nodeToDistribute = nodes.get(0); + Way parent = nodeToDistribute.getParentWays().get(0); - List neighbours = new ArrayList<>(parent.getNeighbours(node)); + List neighbours = new ArrayList<>(parent.getNeighbours(nodeToDistribute)); - nodes.add(neighbours.get(0)); - nodes.add(node); - nodes.add(neighbours.get(1)); + // insert in the middle + neighbours.add(1, nodeToDistribute); // call the distribution method with 3 nodes - return distributeNodes(nodes); + return distributeNodes(neighbours); } /** diff --git a/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java b/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java index 9203394fb7a..fa169b84ebb 100644 --- a/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java +++ b/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java @@ -31,6 +31,24 @@ void setUp() { ds = new DataSet(); } + @Test + void testNoAlignment() { + Node n = new Node(LatLon.ZERO); + ds.addPrimitive(n); + + OsmDataLayer layer = new OsmDataLayer(ds, "", null); + MainApplication.getLayerManager().addLayer(layer); + assertNotNull(MainApplication.getLayerManager().getActiveLayer()); + + // select a single node + layer.getDataSet().setSelected(n.getPrimitiveId()); + assertEquals(1, layer.getDataSet().getSelected().size()); + + new DistributeAction().actionPerformed(null); + assertEquals(n.lat(), LatLon.ZERO.lat()); + assertEquals(n.lon(), LatLon.ZERO.lon()); + } + @Test void testWholeWayAlignment() { Way way = new Way(); @@ -112,6 +130,9 @@ void testNodesAlignment() { new DistributeAction().actionPerformed(null); + // FIXME: the assertion will most likely fail due to the current core implementation: + // the two *furthest nodes* are selected as alignment base, then they evenly ordered along a virtual way. + // Test expectation: the *end nodes* of a virtual way are distribution basis. for (int i = 0; i < totalNodeCount; i++) { assertEquals( (double) (1 / lastLon) + i, From b495fdfa4190b343e3e16b554735d563c00f4fe7 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Wed, 25 Oct 2023 22:17:08 +0200 Subject: [PATCH 05/10] Fix typo and documentation --- .../josm/actions/AlignInLineAction.java | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/org/openstreetmap/josm/actions/AlignInLineAction.java b/src/org/openstreetmap/josm/actions/AlignInLineAction.java index 9d85c4f565e..8332011dedb 100644 --- a/src/org/openstreetmap/josm/actions/AlignInLineAction.java +++ b/src/org/openstreetmap/josm/actions/AlignInLineAction.java @@ -81,11 +81,11 @@ static class InvalidSelection extends Exception { } /** - * Return 2 nodes making up the line along which provided nodes must be aligned. + * Return two nodes making up the line along which provided nodes must be aligned. * * @param nodes Nodes to be aligned. - * @return A array of two nodes. - * @throws IllegalArgumentException if nodes is empty + * @return An array of two nodes. + * @throws IllegalArgumentException if nodes parameter is empty */ private static Node[] nodePairFurthestApart(List nodes) { // Detect if selected nodes are on the same way. @@ -104,16 +104,16 @@ private static Node[] nodePairFurthestApart(List nodes) { throw new IllegalArgumentException(); } - // Nodes belongs to multiple ways, return most distant nodes. + // Nodes belong to multiple ways, return the most distant nodes. if (waysRef.size() != 1) - return nodeFurthestAppart(nodes); + return nodeFurthestApart(nodes); // All nodes are part of the same way. See #9605. Way way = waysRef.iterator().next(); if (way.isClosed()) { // Align these nodes on the line passing through the most distant nodes. - return nodeFurthestAppart(nodes); + return nodeFurthestApart(nodes); } Node nodea = null; @@ -143,9 +143,9 @@ private static Node[] nodePairFurthestApart(List nodes) { * @param nodes List of nodes to analyze. * @return An array containing the two most distant nodes. */ - private static Node[] nodeFurthestAppart(List nodes) { + private static Node[] nodeFurthestApart(List nodes) { Node node1 = null, node2 = null; - double minSqDistance = 0; + double minSqDistance = Double.NEGATIVE_INFINITY; int nb; nb = nodes.size(); @@ -190,7 +190,7 @@ public void actionPerformed(ActionEvent e) { * Builds "align in line" command depending on the selected objects. * @param ds data set in which the command operates * @return the resulting command to execute to perform action - * @throws InvalidSelection if a polygon is selected, or if a node is used by 3 or more ways + * @throws InvalidSelection if a polygon is selected, or if a node is used by three or more ways * @since 13108 */ public Command buildCommand(DataSet ds) throws InvalidSelection { @@ -226,7 +226,7 @@ public Command buildCommand(DataSet ds) throws InvalidSelection { } /** - * Align nodes in case 3 or more nodes are selected. + * Align nodes in case three or more nodes are selected. * * @param nodes Nodes to be aligned. * @return Command that perform action. From c076aa8d294a5717ede0a545b74038b26f828050 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Thu, 26 Oct 2023 00:03:09 +0200 Subject: [PATCH 06/10] Move nodePairFurthestApart() methods to Geometry utility class --- .../josm/actions/AlignInLineAction.java | 86 +------------------ .../openstreetmap/josm/tools/Geometry.java | 86 +++++++++++++++++++ 2 files changed, 87 insertions(+), 85 deletions(-) diff --git a/src/org/openstreetmap/josm/actions/AlignInLineAction.java b/src/org/openstreetmap/josm/actions/AlignInLineAction.java index 8332011dedb..55d6fc61564 100644 --- a/src/org/openstreetmap/josm/actions/AlignInLineAction.java +++ b/src/org/openstreetmap/josm/actions/AlignInLineAction.java @@ -2,6 +2,7 @@ package org.openstreetmap.josm.actions; import static org.openstreetmap.josm.gui.help.HelpUtil.ht; +import static org.openstreetmap.josm.tools.Geometry.nodePairFurthestApart; import static org.openstreetmap.josm.tools.I18n.tr; import java.awt.event.ActionEvent; @@ -80,91 +81,6 @@ static class InvalidSelection extends Exception { } } - /** - * Return two nodes making up the line along which provided nodes must be aligned. - * - * @param nodes Nodes to be aligned. - * @return An array of two nodes. - * @throws IllegalArgumentException if nodes parameter is empty - */ - private static Node[] nodePairFurthestApart(List nodes) { - // Detect if selected nodes are on the same way. - - // Get ways passing though all selected nodes. - Set waysRef = null; - for (Node n: nodes) { - Collection ref = n.getParentWays(); - if (waysRef == null) - waysRef = new HashSet<>(ref); - else - waysRef.retainAll(ref); - } - - if (waysRef == null) { - throw new IllegalArgumentException(); - } - - // Nodes belong to multiple ways, return the most distant nodes. - if (waysRef.size() != 1) - return nodeFurthestApart(nodes); - - // All nodes are part of the same way. See #9605. - Way way = waysRef.iterator().next(); - - if (way.isClosed()) { - // Align these nodes on the line passing through the most distant nodes. - return nodeFurthestApart(nodes); - } - - Node nodea = null; - Node nodeb = null; - - // The way is open, align nodes on the line passing through the extremity nodes (most distant in the way - // sequence). See #9605#comment:3. - Set remainNodes = new HashSet<>(nodes); - for (Node n : way.getNodes()) { - if (!remainNodes.contains(n)) - continue; - if (nodea == null) - nodea = n; - if (remainNodes.size() == 1) { - nodeb = remainNodes.iterator().next(); - break; - } - remainNodes.remove(n); - } - - return new Node[] {nodea, nodeb}; - } - - /** - * Return the two nodes the most distant from the provided list. - * - * @param nodes List of nodes to analyze. - * @return An array containing the two most distant nodes. - */ - private static Node[] nodeFurthestApart(List nodes) { - Node node1 = null, node2 = null; - double minSqDistance = Double.NEGATIVE_INFINITY; - int nb; - - nb = nodes.size(); - for (int i = 0; i < nb - 1; i++) { - Node n = nodes.get(i); - for (int j = i + 1; j < nb; j++) { - Node m = nodes.get(j); - double sqDist = n.getEastNorth().distanceSq(m.getEastNorth()); - if (sqDist > minSqDistance) { - node1 = n; - node2 = m; - minSqDistance = sqDist; - } - } - } - - return new Node[] {node1, node2}; - } - /** * Operation depends on the selected objects: */ diff --git a/src/org/openstreetmap/josm/tools/Geometry.java b/src/org/openstreetmap/josm/tools/Geometry.java index 98cd6c5a34f..2a347b054ac 100644 --- a/src/org/openstreetmap/josm/tools/Geometry.java +++ b/src/org/openstreetmap/josm/tools/Geometry.java @@ -14,6 +14,7 @@ import java.util.Collection; import java.util.Collections; import java.util.Comparator; +import java.util.HashSet; import java.util.Iterator; import java.util.LinkedHashSet; import java.util.List; @@ -1645,6 +1646,91 @@ public static ILatLon getLatLonFrom(final ILatLon original, final double angle, return new LatLon(Math.toDegrees(lat), Math.toDegrees(lon)); } + /** + * Return two nodes making up the line along which provided nodes must be aligned. + * + * @param nodes Nodes to be aligned. + * @return An array of two nodes. + * @throws IllegalArgumentException if nodes parameter is empty + */ + public static Node[] nodePairFurthestApart(List nodes) { + // Detect if selected nodes are on the same way. + + // Get ways passing though all selected nodes. + Set waysRef = null; + for (Node n: nodes) { + Collection ref = n.getParentWays(); + if (waysRef == null) + waysRef = new HashSet<>(ref); + else + waysRef.retainAll(ref); + } + + if (waysRef == null) { + throw new IllegalArgumentException(); + } + + // Nodes belong to multiple ways, return the most distant nodes. + if (waysRef.size() != 1) + return nodeFurthestApart(nodes); + + // All nodes are part of the same way. See #9605. + Way way = waysRef.iterator().next(); + + if (way.isClosed()) { + // Align these nodes on the line passing through the most distant nodes. + return nodeFurthestApart(nodes); + } + + Node nodea = null; + Node nodeb = null; + + // The way is open, align nodes on the line passing through the extremity nodes (most distant in the way + // sequence). See #9605#comment:3. + Set remainNodes = new HashSet<>(nodes); + for (Node n : way.getNodes()) { + if (!remainNodes.contains(n)) + continue; + if (nodea == null) + nodea = n; + if (remainNodes.size() == 1) { + nodeb = remainNodes.iterator().next(); + break; + } + remainNodes.remove(n); + } + + return new Node[] {nodea, nodeb}; + } + + /** + * Return the two nodes the most distant from the provided list. + * + * @param nodes List of nodes to analyze. + * @return An array containing the two most distant nodes. + */ + public static Node[] nodeFurthestApart(List nodes) { + Node node1 = null, node2 = null; + double minSqDistance = Double.NEGATIVE_INFINITY; + int nb; + + nb = nodes.size(); + for (int i = 0; i < nb - 1; i++) { + Node n = nodes.get(i); + for (int j = i + 1; j < nb; j++) { + Node m = nodes.get(j); + double sqDist = n.getEastNorth().distanceSq(m.getEastNorth()); + if (sqDist > minSqDistance) { + node1 = n; + node2 = m; + minSqDistance = sqDist; + } + } + } + + return new Node[] {node1, node2}; + } + /** * Calculate closest distance between a line segment s1-s2 and a point p * @param s1 start of segment From 24a43fe1736df651487b7980ce25d2f0c62dec68 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Thu, 26 Oct 2023 00:34:48 +0200 Subject: [PATCH 07/10] Add a todo before I forget --- .../org/openstreetmap/josm/actions/DistributeActionTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java b/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java index fa169b84ebb..8c1e6dcbadd 100644 --- a/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java +++ b/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java @@ -97,6 +97,7 @@ void testWholeWayAlignment() { @Test void testNodesAlignment() { Way way = new Way(); + // TODO: make it less restricted in range final int totalNodeCount = 11; // should be in range [2,180]! final int innerNodeCount = totalNodeCount - 2; final int lastLon = totalNodeCount - 1; From c9d0d474650b13a2dc11fe5fb30d462f1d143ed9 Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Thu, 26 Oct 2023 17:57:04 +0200 Subject: [PATCH 08/10] Make test method parameters more flexible --- .../josm/actions/DistributeActionTest.java | 51 +++++++++---------- 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java b/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java index 8c1e6dcbadd..f3448a28d99 100644 --- a/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java +++ b/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java @@ -52,9 +52,9 @@ void testNoAlignment() { @Test void testWholeWayAlignment() { Way way = new Way(); - final int totalNodeCount = 11; // should be in range [2,180]! + final int totalNodeCount = 1200; // should be at least three final int innerNodeCount = totalNodeCount - 2; - final int lastLon = totalNodeCount - 1; + final int lonLimit = 10; // add first node Node n = new Node(new LatLon(LatLon.ZERO)); @@ -63,13 +63,14 @@ void testWholeWayAlignment() { // add interim nodes for (int i = 0; i < innerNodeCount; i++) { - n = new Node(new LatLon(0, getRandomDoubleInRange(0, lastLon))); + // 0.001 subtracted to avoid cases where the random double == lonLimit + n = new Node(new LatLon(0, getRandomDoubleInRange(0, lonLimit - 0.001))); ds.addPrimitive(n); way.addNode(n); } // add last node - n = new Node(new LatLon(0, lastLon)); + n = new Node(new LatLon(0, lonLimit)); ds.addPrimitive(n); way.addNode(n); ds.addPrimitive(way); @@ -86,21 +87,17 @@ void testWholeWayAlignment() { new DistributeAction().actionPerformed(null); for (int i = 0; i < totalNodeCount; i++) { - assertEquals( - (double) (1 / lastLon) + i, - way.getNode(i).lon(), - 1e-7 - ); + double evenGapNthNode = ((double) lonLimit / (totalNodeCount - 1)) * i; + assertEquals(evenGapNthNode, way.getNode(i).lon(), 1e-7); } } @Test void testNodesAlignment() { Way way = new Way(); - // TODO: make it less restricted in range - final int totalNodeCount = 11; // should be in range [2,180]! + final int totalNodeCount = 1100; // should be at least three final int innerNodeCount = totalNodeCount - 2; - final int lastLon = totalNodeCount - 1; + final int lonLimit = 10; // add first node Node n = new Node(new LatLon(LatLon.ZERO)); @@ -109,13 +106,14 @@ void testNodesAlignment() { // add interim nodes for (int i = 0; i < innerNodeCount; i++) { - n = new Node(new LatLon(0, getRandomDoubleInRange(0, lastLon))); + // 0.001 subtracted to avoid cases where the random double == lonLimit + n = new Node(new LatLon(0, getRandomDoubleInRange(0, lonLimit - 0.001))); ds.addPrimitive(n); way.addNode(n); } // add last node - n = new Node(new LatLon(0, lastLon)); + n = new Node(new LatLon(0, lonLimit)); ds.addPrimitive(n); way.addNode(n); ds.addPrimitive(way); @@ -135,11 +133,8 @@ void testNodesAlignment() { // the two *furthest nodes* are selected as alignment base, then they evenly ordered along a virtual way. // Test expectation: the *end nodes* of a virtual way are distribution basis. for (int i = 0; i < totalNodeCount; i++) { - assertEquals( - (double) (1 / lastLon) + i, - way.getNode(i).lon(), - 1e-7 - ); + double evenGapNthNode = ((double) lonLimit / (totalNodeCount - 1)) * i; + assertEquals(evenGapNthNode, way.getNode(i).lon(), 1e-7); } } @@ -147,7 +142,7 @@ void testNodesAlignment() { void testSingleNodeAlignment() { Way way = new Way(); final int totalNodeCount = 11; // should be in range [3,180]! - final int lastLon = totalNodeCount - 1; + final int lonLimit = totalNodeCount - 1; Node n; Node selectedNode = null; @@ -165,7 +160,7 @@ void testSingleNodeAlignment() { } // add last node - n = new Node(new LatLon(0, lastLon)); + n = new Node(new LatLon(0, lonLimit)); ds.addPrimitive(n); way.addNode(n); ds.addPrimitive(way); @@ -182,14 +177,18 @@ void testSingleNodeAlignment() { new DistributeAction().actionPerformed(null); for (int i = 0; i < totalNodeCount; i++) { - assertEquals( - (double) (1 / lastLon) + i, - way.getNode(i).lon(), - 1e-7 - ); + double evenGapNthNode = ((double) lonLimit / (totalNodeCount - 1)) * i; + assertEquals(evenGapNthNode, way.getNode(i).lon(), 1e-7); } } + + /** + * @param min range lower value + * @param max range upper value + * @return a single double in the given range + * @throws IllegalArgumentException if min {@code >=} max + */ private static double getRandomDoubleInRange(double min, double max) { if (min >= max) { throw new IllegalArgumentException("Invalid range. Max must be greater than min."); From f5ac297c62e66cfd8bac2fe0c56258c3c057ed1c Mon Sep 17 00:00:00 2001 From: gaben <23311361+gabortim@users.noreply.github.com> Date: Thu, 26 Oct 2023 20:22:38 +0200 Subject: [PATCH 09/10] Simplify DistributeAction by using Geometry class methods ...plus some minor documentation fixes as usual --- .../josm/actions/DistributeAction.java | 34 ++------- .../openstreetmap/josm/tools/Geometry.java | 69 ++++++++++--------- .../josm/actions/DistributeActionTest.java | 2 +- 3 files changed, 43 insertions(+), 62 deletions(-) diff --git a/src/org/openstreetmap/josm/actions/DistributeAction.java b/src/org/openstreetmap/josm/actions/DistributeAction.java index d70d63e3378..9b246d1f827 100644 --- a/src/org/openstreetmap/josm/actions/DistributeAction.java +++ b/src/org/openstreetmap/josm/actions/DistributeAction.java @@ -2,6 +2,8 @@ package org.openstreetmap.josm.actions; import static org.openstreetmap.josm.gui.help.HelpUtil.ht; +import static org.openstreetmap.josm.tools.Geometry.getFurthestPrimitive; +import static org.openstreetmap.josm.tools.Geometry.nodeFurthestApart; import static org.openstreetmap.josm.tools.I18n.tr; import java.awt.event.ActionEvent; @@ -84,7 +86,7 @@ public void actionPerformed(ActionEvent e) { cmds = distributeWay(ways, nodes); } else if (checkDistributeNodes(ways, nodes)) { cmds = distributeNodes(nodes); - } else if (checkDistributeNode(nodes)){ + } else if (checkDistributeNode(nodes)) { cmds = distributeNode(nodes); } else { new Notification( @@ -251,23 +253,9 @@ private static boolean checkDistributeNodes(Collection ways, Collection distributeNodes(Collection nodes) { // Find from the selected nodes two that are the furthest apart. // Let's call them A and B. - double distance = Double.NEGATIVE_INFINITY; - - Node nodea = null; - Node nodeb = null; - - Collection itnodes = new LinkedList<>(nodes); - for (Node n : nodes) { - itnodes.remove(n); - for (Node m : itnodes) { - double dist = Math.sqrt(n.getEastNorth().distance(m.getEastNorth())); - if (dist > distance) { - nodea = n; - nodeb = m; - distance = dist; - } - } - } + Node[] furthestApart = nodeFurthestApart(new ArrayList<>(nodes)); + Node nodea = furthestApart[0]; + Node nodeb = furthestApart[1]; if (nodea == null || nodeb == null) { throw new IllegalArgumentException(); @@ -293,17 +281,9 @@ private static Collection distributeNodes(Collection nodes) { int pos = 0; while (!nodes.isEmpty()) { pos++; - Node s = null; // Find the node that is furthest from B (i.e. closest to A) - distance = Double.NEGATIVE_INFINITY; - for (Node n : nodes) { - double dist = Math.sqrt(nodeb.getEastNorth().distance(n.getEastNorth())); - if (dist > distance) { - s = n; - distance = dist; - } - } + Node s = getFurthestPrimitive(nodeb, nodes); if (s != null) { // First move the node to A's position, then move it towards B diff --git a/src/org/openstreetmap/josm/tools/Geometry.java b/src/org/openstreetmap/josm/tools/Geometry.java index 2a347b054ac..868ad929d2d 100644 --- a/src/org/openstreetmap/josm/tools/Geometry.java +++ b/src/org/openstreetmap/josm/tools/Geometry.java @@ -86,10 +86,10 @@ public enum PolygonIntersection { public static final double INTERSECTION_EPS_EAST_NORTH = 1e-4; /** - * Will find all intersection and add nodes there for list of given ways. + * Will find all intersections and add nodes there for list of given ways. * Handles self-intersections too. * And makes commands to add the intersection points to ways. - * + *

* Prerequisite: no two nodes have the same coordinates. * * @param ways a list of ways to test @@ -244,7 +244,7 @@ public static Set addIntersections(List ways, boolean test, List * (Imagine the path is continued beyond the endpoints, so you get two rays * starting from lineP2 and going through lineP1 and lineP3 respectively * which divide the plane into two parts. The test returns true, if testPoint @@ -532,7 +532,7 @@ public static EastNorth closestPointToLine(EastNorth lineP1, EastNorth lineP2, E /** * This method tests if secondNode is clockwise to first node. - * + *

* The line through the two points commonNode and firstNode divides the * plane into two parts. The test returns true, if secondNode lies in * the part that is to the right when traveling in the direction from @@ -842,11 +842,11 @@ public static Double computeArea(IPrimitive osm) { /** * Determines whether a way is oriented clockwise. - * + *

* Internals: Assuming a closed non-looping way, compute twice the area * of the polygon using the formula {@code 2 * area = sum (X[n] * Y[n+1] - X[n+1] * Y[n])}. - * If the area is negative the way is ordered in a clockwise direction. - * + * If the area is negative, the way is ordered in a clockwise direction. + *

* See http://paulbourke.net/geometry/polyarea/ * * @param w the way to be checked. @@ -994,10 +994,10 @@ public static EastNorth getCentroidEN(List nodes) { /** * Compute the center of the circle closest to different nodes. - * - * Ensure exact center computation in case nodes are already aligned in circle. + *

+ * Ensure exact center computations in case nodes are already aligned in circle. * This is done by least square method. - * Let be a_i x + b_i y + c_i = 0 equations of bisectors of each edges. + * Let be a_i x + b_i y + c_i = 0 equations of bisectors of each edge. * Center must be intersection of all bisectors. * 

      *          [ a1  b1  ]         [ -c1 ]
@@ -1013,12 +1013,12 @@ public static EastNorth getCentroidEN(List nodes) {
     public static EastNorth getCenter(List nodes) {
         int nc = nodes.size();
         if (nc < 3) return null;
-        /**
-         * Equation of each bisector ax + by + c = 0
-         */
+
+        // Equation of each bisector ax + by + c = 0
         double[] a = new double[nc];
         double[] b = new double[nc];
         double[] c = new double[nc];
+
         // Compute equation of bisector
         for (int i = 0; i < nc; i++) {
             EastNorth pt1 = nodes.get(i).getEastNorth();
@@ -1282,8 +1282,8 @@ public double getPerimeter() {
     }
 
     /**
-     * Calculate area and perimeter length of a polygon.
-     *
+     * Calculate the area and perimeter length of a polygon.
+     * 

      * Uses current projection; units are that of the projected coordinates.
      *
      * @param nodes the list of nodes representing the polygon
@@ -1294,7 +1294,7 @@ public static AreaAndPerimeter getAreaAndPerimeter(List nodes
     }
 
     /**
-     * Calculate area and perimeter length of a polygon in the given projection.
+     * Calculate the area and perimeter length of a polygon in the given projection.
      *
      * @param nodes the list of nodes representing the polygon
      * @param projection the projection to use for the calculation, {@code null} defaults to {@link ProjectionRegistry#getProjection()}
@@ -1326,10 +1326,10 @@ public static AreaAndPerimeter getAreaAndPerimeter(List nodes
 
     /**
      * Get the closest primitive to {@code osm} from the collection of
-     * OsmPrimitive {@code primitives}
-     *
+     * OsmPrimitive {@code primitives}.
+     * 

      * The {@code primitives} should be fully downloaded to ensure accuracy.
-     *
+     * 

      * Note: The complexity of this method is O(n*m), where n is the number of
      * children {@code osm} has plus 1, m is the number of children the
      * collection of primitives have plus the number of primitives in the
@@ -1350,10 +1350,10 @@ public static  T getClosestPrimitive(OsmPrimitive osm, C
 
     /**
      * Get the closest primitives to {@code osm} from the collection of
-     * OsmPrimitive {@code primitives}
-     *
+     * OsmPrimitive {@code primitives}.
+     * 

      * The {@code primitives} should be fully downloaded to ensure accuracy.
-     *
+     * 

      * Note: The complexity of this method is O(n*m), where n is the number of
      * children {@code osm} has plus 1, m is the number of children the
      * collection of primitives have plus the number of primitives in the
@@ -1385,13 +1385,13 @@ public static  Collection getClosestPrimitives(OsmPri
 
     /**
      * Get the furthest primitive to {@code osm} from the collection of
-     * OsmPrimitive {@code primitives}
-     *
+     * OsmPrimitive {@code primitives}.
+     * 

      * The {@code primitives} should be fully downloaded to ensure accuracy.
-     *
+     * 

      * It does NOT give the furthest primitive based off of the furthest
      * part of that primitive
-     *
+     * 

      * Note: The complexity of this method is O(n*m), where n is the number of
      * children {@code osm} has plus 1, m is the number of children the
      * collection of primitives have plus the number of primitives in the
@@ -1411,13 +1411,13 @@ public static  T getFurthestPrimitive(OsmPrimitive osm,
 
     /**
      * Get the furthest primitives to {@code osm} from the collection of
-     * OsmPrimitive {@code primitives}
-     *
+     * OsmPrimitive {@code primitives}.
+     * 

      * The {@code primitives} should be fully downloaded to ensure accuracy.
-     *
+     * 

      * It does NOT give the furthest primitive based off of the furthest
      * part of that primitive
-     *
+     * 

      * Note: The complexity of this method is O(n*m), where n is the number of
      * children {@code osm} has plus 1, m is the number of children the
      * collection of primitives have plus the number of primitives in the
@@ -1432,8 +1432,9 @@ public static  T getFurthestPrimitive(OsmPrimitive osm,
     public static  Collection getFurthestPrimitives(OsmPrimitive osm, Collection primitives) {
         double furthestDistance = Double.NEGATIVE_INFINITY;
         TreeSet furthest = new TreeSet<>();
+        double distance;
         for (T primitive : primitives) {
-            double distance = getDistance(osm, primitive);
+            distance = getDistance(osm, primitive);
             if (Double.isNaN(distance)) continue;
             int comp = Double.compare(distance, furthestDistance);
             if (comp > 0) {
@@ -1454,7 +1455,7 @@ public static  Collection getFurthestPrimitives(OsmPr
      * @return The distance between the primitives in meters
      * (or the unit of the current projection, see {@link Projection}).
      * May return {@link Double#NaN} if one of the primitives is incomplete.
-     *
+     * 

      * Note: The complexity is O(n*m), where (n,m) are the number of child
      * objects the {@link OsmPrimitive}s have.
      * @since 15035
@@ -1732,11 +1733,11 @@ public static Node[] nodeFurthestApart(List nodes) {
     }
 
     /**
-     * Calculate closest distance between a line segment s1-s2 and a point p
+     * Calculate the closest distance between a line segment s1-s2 and a point p
      * @param s1 start of segment
      * @param s2 end of segment
      * @param p the point
-     * @return the square of the euclidean distance from p to the closest point on the segment
+     * @return the square of the Euclidean distance from p to the closest point on the segment
      */
     private static double getSegmentNodeDistSq(EastNorth s1, EastNorth s2, EastNorth p) {
         EastNorth c1 = closestPointTo(s1, s2, p, true);
diff --git a/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java b/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java
index f3448a28d99..4ed6554e0b1 100644
--- a/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java
+++ b/test/unit/org/openstreetmap/josm/actions/DistributeActionTest.java
@@ -52,7 +52,7 @@ void testNoAlignment() {
     @Test
     void testWholeWayAlignment() {
         Way way = new Way();
-        final int totalNodeCount = 1200;  // should be at least three
+        final int totalNodeCount = 120;  // should be at least three
         final int innerNodeCount = totalNodeCount - 2;
         final int lonLimit = 10;
 

From c8e3d6598b7425385b88bf60f77b18503e91c1eb Mon Sep 17 00:00:00 2001
From: gaben <23311361+gabortim@users.noreply.github.com>
Date: Sun, 29 Oct 2023 18:56:05 +0100
Subject: [PATCH 10/10] Rename local variables

---
 .../josm/actions/DistributeAction.java        | 20 +++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/org/openstreetmap/josm/actions/DistributeAction.java b/src/org/openstreetmap/josm/actions/DistributeAction.java
index 9b246d1f827..cc63aeb024d 100644
--- a/src/org/openstreetmap/josm/actions/DistributeAction.java
+++ b/src/org/openstreetmap/josm/actions/DistributeAction.java
@@ -254,22 +254,22 @@ private static Collection distributeNodes(Collection nodes) {
         // Find from the selected nodes two that are the furthest apart.
         // Let's call them A and B.
         Node[] furthestApart = nodeFurthestApart(new ArrayList<>(nodes));
-        Node nodea = furthestApart[0];
-        Node nodeb = furthestApart[1];
+        Node nodeA = furthestApart[0];
+        Node nodeB = furthestApart[1];
 
-        if (nodea == null || nodeb == null) {
+        if (nodeA == null || nodeB == null) {
             throw new IllegalArgumentException();
         }
 
         // Remove the nodes A and B from the list of nodes to move
-        nodes.remove(nodea);
-        nodes.remove(nodeb);
+        nodes.remove(nodeA);
+        nodes.remove(nodeB);
 
         // Find out co-ords of A and B
-        double ax = nodea.getEastNorth().east();
-        double ay = nodea.getEastNorth().north();
-        double bx = nodeb.getEastNorth().east();
-        double by = nodeb.getEastNorth().north();
+        double ax = nodeA.getEastNorth().east();
+        double ay = nodeA.getEastNorth().north();
+        double bx = nodeB.getEastNorth().east();
+        double by = nodeB.getEastNorth().north();
 
         // A list of commands to do
         Collection cmds = new LinkedList<>();
@@ -283,7 +283,7 @@ private static Collection distributeNodes(Collection nodes) {
             pos++;
 
             // Find the node that is furthest from B (i.e. closest to A)
-            Node s = getFurthestPrimitive(nodeb, nodes);
+            Node s = getFurthestPrimitive(nodeB, nodes);
 
             if (s != null) {
                 // First move the node to A's position, then move it towards B