Index: /trunk/src/org/openstreetmap/josm/actions/JoinNodeWayAction.java =================================================================== --- /trunk/src/org/openstreetmap/josm/actions/JoinNodeWayAction.java (revision 15609) +++ /trunk/src/org/openstreetmap/josm/actions/JoinNodeWayAction.java (revision 15610) @@ -13,8 +13,11 @@ 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,5 +94,5 @@ Collection selectedNodes = ds.getSelectedNodes(); Collection cmds = new LinkedList<>(); - Map> data = new HashMap<>(); + Map> data = new LinkedHashMap<>(); // If the user has selected some ways, only join the node to these. @@ -100,5 +103,7 @@ for (Node node : selectedNodes) { List wss = mapView.getNearestWaySegments(mapView.getPoint(node), OsmPrimitive::isSelectable); - Set seenWays = new HashSet<>(); + // we cannot trust the order of elements in wss because it was calculated based on rounded position value of node + TreeMap> 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,13 +111,38 @@ 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 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 wslist = nearestMap.computeIfAbsent(distSq, k -> new LinkedList<>()); + wslist.add(ws); + } + Set seenWays = new HashSet<>(); + Double usedDist = null; + while (!nearestMap.isEmpty()) { + Entry> 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 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,4 +172,5 @@ 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) Index: /trunk/test/data/regress/18189/moveontocrossing.osm =================================================================== --- /trunk/test/data/regress/18189/moveontocrossing.osm (revision 15609) +++ /trunk/test/data/regress/18189/moveontocrossing.osm (revision 15610) @@ -1,22 +1,22 @@ - - - - - + + + + + - - - - - + + + + + - - - - + + + + Index: /trunk/test/data/regress/18420/user-sample.osm =================================================================== --- /trunk/test/data/regress/18420/user-sample.osm (revision 15610) +++ /trunk/test/data/regress/18420/user-sample.osm (revision 15610) @@ -0,0 +1,119 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + Index: /trunk/test/unit/org/openstreetmap/josm/actions/JoinNodeWayActionTest.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/actions/JoinNodeWayActionTest.java (revision 15609) +++ /trunk/test/unit/org/openstreetmap/josm/actions/JoinNodeWayActionTest.java (revision 15610) @@ -6,8 +6,8 @@ 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,9 +37,9 @@ @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,5 +59,4 @@ */ @Test - @Ignore public void testTicket18189() throws Exception { DataSet dataSet = new DataSet(); @@ -105,5 +104,4 @@ */ @Test - @Ignore public void testTicket11508() throws Exception { DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(11508, "11508_example.osm"), null); @@ -134,5 +132,4 @@ */ @Test - @Ignore public void testTicket18189Crossing() throws Exception { DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(18189, "moveontocrossing.osm"), null); @@ -160,5 +157,4 @@ */ @Test - @Ignore public void testTicket18189ThreeWays() throws Exception { DataSet ds = OsmReader.parseDataSet(TestUtils.getRegressionDataStream(18189, "data.osm"), null); @@ -184,3 +180,36 @@ } + /** + * Non-regression test for Bug #18420. + * @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 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); + } + } + }