Changeset 15610 in josm

Timestamp:

2019-12-24T11:01:03+01:00 (4 years ago)

Author:

GerdP

Message:

fix #18420: Move node onto ways not working anymore

• sort result from NavigatableComponent again using higher precision
• improve handling of multiple target ways with similar distance so that the node is added to the closest segment and also to segments with almost the same position but NOT to segments further away or with a similar distance but different position.
• fix possible error when multiple nodes are selected
• use LinkedHashMap for field data to avoid random results from iterator (hope this fixes the unit test problems)

Location:

trunk

Files:

• src/org/openstreetmap/josm/actions/JoinNodeWayAction.java (modified) (5 diffs)
• test/data/regress/18189/moveontocrossing.osm (modified) (1 diff)
• test/data/regress/18420 (added)
• test/data/regress/18420/user-sample.osm (added)
• test/unit/org/openstreetmap/josm/actions/JoinNodeWayActionTest.java (modified) (7 diffs)

trunk/src/org/openstreetmap/josm/actions/JoinNodeWayAction.java

@@ -13 +13 @@
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Set;
+import java.util.TreeMap;
 
 import javax.swing.JOptionPane;
@@ -91 +94 @@
         Collection<Node> selectedNodes = ds.getSelectedNodes();
         Collection<Command> cmds = new LinkedList<>();
-        Map<Way, MultiMap<Integer, Node>> data = new HashMap<>();
+        Map<Way, MultiMap<Integer, Node>> data = new LinkedHashMap<>();
 
         // If the user has selected some ways, only join the node to these.
@@ -100 +103 @@
         for (Node node : selectedNodes) {
             List<WaySegment> wss = mapView.getNearestWaySegments(mapView.getPoint(node), OsmPrimitive::isSelectable);
-            Set<Way> seenWays = new HashSet<>();
+            // we cannot trust the order of elements in wss because it was calculated based on rounded position value of node
+            TreeMap<Double, List<WaySegment>> nearestMap = new TreeMap<>();
+            EastNorth en = node.getEastNorth();
             for (WaySegment ws : wss) {
                 // Maybe cleaner to pass a "isSelected" predicate to getNearestWaySegments, but this is less invasive.
@@ -106 +111 @@
                     continue;
                 }
-                // only use the closest WaySegment of each way and ignore those that already contain the node
-                if (!ws.getFirstNode().equals(node) && !ws.getSecondNode().equals(node)
-                        && !seenWays.contains(ws.way)) {
-                    MultiMap<Integer, Node> innerMap = data.get(ws.way);
-                    if (innerMap == null) {
-                        innerMap = new MultiMap<>();
-                        data.put(ws.way, innerMap);
+                /* perpendicular distance squared
+                 * loose some precision to account for possible deviations in the calculation above
+                 * e.g. if identical (A and B) come about reversed in another way, values may differ
+                 * -- zero out least significant 32 dual digits of mantissa..
+                 */
+                double distSq = en.distanceSq(Geometry.closestPointToSegment(ws.getFirstNode().getEastNorth(),
+                        ws.getSecondNode().getEastNorth(), en));
+                // resolution in numbers with large exponent not needed here..
+                distSq = Double.longBitsToDouble(Double.doubleToLongBits(distSq) >> 32 << 32);
+                List<WaySegment> wslist = nearestMap.computeIfAbsent(distSq, k -> new LinkedList<>());
+                wslist.add(ws);
+            }
+            Set<Way> seenWays = new HashSet<>();
+            Double usedDist = null;
+            while (!nearestMap.isEmpty()) {
+                Entry<Double, List<WaySegment>> entry = nearestMap.pollFirstEntry();
+                if (usedDist != null) {
+                    double delta = entry.getKey() - usedDist;
+                    if (delta > 1e-4)
+                        break;
+                }
+                for (WaySegment ws : entry.getValue()) {
+                    // only use the closest WaySegment of each way and ignore those that already contain the node
+                    if (!ws.getFirstNode().equals(node) && !ws.getSecondNode().equals(node)
+                            && !seenWays.contains(ws.way)) {
+                        if (usedDist == null)
+                            usedDist = entry.getKey();
+                        MultiMap<Integer, Node> innerMap = data.get(ws.way);
+                        if (innerMap == null) {
+                            innerMap = new MultiMap<>();
+                            data.put(ws.way, innerMap);
+                        }
+                        innerMap.put(ws.lowerIndex, node);
                     }
-                    innerMap.put(ws.lowerIndex, node);
                     seenWays.add(ws.way);
                 }
@@ -142 +172 @@
                         if (prevMove != null) {
                             if (!prevMove.equalsEpsilon(newPosition, 1e-4)) {
+                                // very unlikely: node has same distance to multiple ways which are not nearly overlapping
                                 new Notification(tr("Multiple target ways, no common point found. Nothing was changed."))
                                         .setIcon(JOptionPane.INFORMATION_MESSAGE)

trunk/test/data/regress/18189/moveontocrossing.osm

@@ -1 +1 @@
 <?xml version='1.0' encoding='UTF-8'?>
 <osm version='0.6' upload='never' generator='JOSM'>
-  <node id='-117814' action='modify' visible='true' lat='-21.08763547741' lon='-50.39117567184' />
-  <node id='-117816' action='modify' visible='true' lat='-21.09088329715' lon='-50.38820000246' />
-  <node id='-117818' action='modify' visible='true' lat='-21.09002420335' lon='-50.39142270855' />
-  <node id='-117820' action='modify' visible='true' lat='-21.08836886226' lon='-50.38800911046' />
-  <node id='-117822' action='modify' visible='true' lat='-21.089215371' lon='-50.38971309121'>
+  <node id='-117848' action='modify' lat='-21.08763547741' lon='-50.39117567184' />
+  <node id='-117850' action='modify' lat='-21.09088329715' lon='-50.38820000246' />
+  <node id='-117852' action='modify' lat='-21.09002420335' lon='-50.39142270855' />
+  <node id='-117854' action='modify' lat='-21.08836886226' lon='-50.38800911046' />
+  <node id='-117856' action='modify' lat='-21.08922243916' lon='-50.38958051884'>
     <tag k='name' v='select me and press N' />
   </node>
-  <node id='-117824' action='modify' visible='true' lat='-21.08920644697' lon='-50.38973634946' />
-  <way id='-117825' action='modify' visible='true'>
-    <nd ref='-117814' />
-    <nd ref='-117824' />
-    <nd ref='-117816' />
+  <node id='-117858' action='modify' lat='-21.08920644697' lon='-50.38973634946' />
+  <way id='-117859' action='modify'>
+    <nd ref='-117848' />
+    <nd ref='-117858' />
+    <nd ref='-117850' />
     <tag k='name' v='w1' />
   </way>
-  <way id='-117826' action='modify' visible='true'>
-    <nd ref='-117818' />
-    <nd ref='-117824' />
-    <nd ref='-117820' />
+  <way id='-117860' action='modify'>
+    <nd ref='-117852' />
+    <nd ref='-117858' />
+    <nd ref='-117854' />
     <tag k='name' v='w2' />
   </way>

trunk/test/unit/org/openstreetmap/josm/actions/JoinNodeWayActionTest.java

@@ -6 +6 @@
 import java.awt.Rectangle;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 import java.util.stream.Collectors;
 
-import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
@@ -37 +37 @@
     @Rule
     @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
-    public JOSMTestRules test = new JOSMTestRules().projection().main().preferences().projection();
+    public JOSMTestRules test = new JOSMTestRules().projection().main().preferences();
 
     private void setupMapView(DataSet ds) {
-        // setup a reasonable screen size
-        MainApplication.getMap().mapView.setBounds(new Rectangle(1920, 1080));
+        // setup a reasonable size for the edit window
+        MainApplication.getMap().mapView.setBounds(new Rectangle(1345, 939));
         if (ds.getDataSourceBoundingBox() != null) {
             MainApplication.getMap().mapView.zoomTo(ds.getDataSourceBoundingBox());
@@ -59 +59 @@
      */
     @Test
-    @Ignore
     public void testTicket18189() throws Exception {
         DataSet dataSet = new DataSet();
@@ -105 +104 @@
      */
     @Test
-    @Ignore
     public void testTicket11508() throws Exception {
         DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(11508, "11508_example.osm"), null);
@@ -134 +132 @@
      */
     @Test
-    @Ignore
     public void testTicket18189Crossing() throws Exception {
         DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(18189, "moveontocrossing.osm"), null);
@@ -160 +157 @@
      */
     @Test
-    @Ignore
     public void testTicket18189ThreeWays() throws Exception {
         DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(18189, "data.osm"), null);
@@ -184 +180 @@
     }
 
+    /**
+     * Non-regression test for <a href="https://josm.openstreetmap.de/ticket/18420">Bug #18420</a>.
+     * @throws Exception if an error occurs
+     */
+    @Test
+    public void testTicket18420() throws Exception {
+        DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(18420, "user-sample.osm"), null);
+        Layer layer = new OsmDataLayer(ds, OsmDataLayer.createNewName(), null);
+        MainApplication.getLayerManager().addLayer(layer);
+        try {
+            List<Node> nodesToMove = ds.getNodes().stream().filter(n -> n.hasTag("name")).collect(Collectors.toList());
+            assertTrue(nodesToMove.size() == 2);
+            Node n = nodesToMove.iterator().next();
+            if (!n.hasTag("name", "select me 1st"))
+                Collections.reverse(nodesToMove);
+            Node toMove1 = nodesToMove.get(0);
+            Node toMove2 = nodesToMove.get(1);
+            Node expected1 = new Node(new LatLon(49.8546658263727, 6.206059532463773));
+            Node expected2 = new Node(new LatLon(49.854738602108085, 6.206213646054511));
+            ds.setSelected(nodesToMove);
+            setupMapView(ds);
+            JoinNodeWayAction action = JoinNodeWayAction.createMoveNodeOntoWayAction();
+            action.setEnabled(true);
+            action.actionPerformed(null);
+            assertTrue("Node was moved to an unexpected position", toMove1.getEastNorth().equalsEpsilon(expected1.getEastNorth(), 1e-7));
+            assertTrue("Node was moved to an unexpected position", toMove2.getEastNorth().equalsEpsilon(expected2.getEastNorth(), 1e-7));
+            assertTrue("Node was not added to expected number of ways", toMove1.getParentWays().size() == 2);
+            assertTrue("Node was not added to expected number of ways", toMove2.getParentWays().size() == 2);
+        } finally {
+            MainApplication.getLayerManager().removeLayer(layer);
+        }
+    }
+
 }