Ticket #19188: 19188.patch

src/org/openstreetmap/josm/plugins/utilsplugin2/curves/CircleArcMaker.java

@@ -1 +1 @@
 // License: GPL. For details, see LICENSE file.
 package org.openstreetmap.josm.plugins.utilsplugin2.curves;
 
+import static org.openstreetmap.josm.tools.I18n.tr;
+
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -10 +12 @@
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 import org.openstreetmap.josm.command.AddCommand;
 import org.openstreetmap.josm.command.ChangeCommand;
@@ -27 +30 @@
 import org.openstreetmap.josm.data.projection.ProjectionRegistry;
 import org.openstreetmap.josm.gui.MainApplication;
 import org.openstreetmap.josm.tools.Geometry;
+import org.openstreetmap.josm.tools.JosmRuntimeException;
 
 /**
  * Create a circle arc
@@ -107 +111 @@
         LatLon ll2 = ProjectionRegistry.getProjection().eastNorth2latlon(center);
 
         double radiusInMeters = ll1.greatCircleDistance(ll2);
+        if (radiusInMeters < 0.01)
+            throw new JosmRuntimeException(tr("Radius too small"));
 
         int numberOfNodesInCircle = (int) Math.ceil(6.0 * Math.pow(radiusInMeters, 0.5));
         // an odd number of nodes makes the distribution uneven
@@ -126 +132 @@
         final List<Node> nodes = new ArrayList<>(w.getNodes());
 
         if (!selectedWays.isEmpty()) {
+            if (w.isClosed()) {
+                // see #19188
+                nodes.clear();
+                nodes.addAll(findShortestPart(w, anchorNodes));
+            }
             // Fix #7341. sort nodes in ways nodes order
             List<Node> consideredNodes = Arrays.asList(n1, n2, n3);
-            Collections.sort(consideredNodes, (o1, o2) -> nodes.indexOf(o1) - nodes.indexOf(o2));
+            consideredNodes.sort((o1, o2) -> nodes.indexOf(o1) - nodes.indexOf(o2));
             n1 = consideredNodes.get(0);
+            n2 = consideredNodes.get(1);
             n3 = consideredNodes.get(2);
+            anchorNodes = Arrays.asList(n1, n2, n3);
         }
 
-        Set<Node> fixNodes = new HashSet<>(anchorNodes);
-        if (!selectedWays.isEmpty()) {
-            nodes.stream().filter(
-                    n -> n.isTagged() || n.getParentWays().size() > 1 || n.referrers(Relation.class).count() > 0)
-                    .forEach(fixNodes::add);
+
+        List<Node> cwTest = new ArrayList<>(Arrays.asList(n1, n2, n3));
+        if (cwTest.get(0) != cwTest.get(cwTest.size() - 1)) {
+            cwTest.add(cwTest.get(0));
         }
+        boolean clockWise = Geometry.isClockwise(cwTest);
+
         boolean needsUndo = false;
         if (!cmds.isEmpty()) {
             UndoRedoHandler.getInstance().add(new SequenceCommand("add nodes", cmds));
             needsUndo = true;
         }
+        Set<Way> targetWays = new HashSet<>();
+        anchorNodes.forEach(n -> targetWays.addAll(n.getParentWays()));
 
         int pos1 = nodes.indexOf(n1);
         int pos3 = nodes.indexOf(n3);
-        List<Node> toModify = new ArrayList<>(nodes.subList(pos1, pos3 + 1));
-        cmds.addAll(worker(toModify, fixNodes, center, radius, maxAngle));
-        if (toModify.size() > pos3 + 1 - pos1) {
-            List<Node> changed = new ArrayList<>();
-            changed.addAll(nodes.subList(0, pos1));
-            changed.addAll(toModify);
-            changed.addAll(nodes.subList(pos3 + 1, nodes.size()));
-            Way wNew = new Way(w);
-            wNew.setNodes(changed);
-            cmds.add(new ChangeCommand(w, wNew));
+        Set<Node> fixNodes = new HashSet<>(anchorNodes);
+        if (!selectedWays.isEmpty()) {
+            for (int i = pos1 + 1; i < pos3; i++) {
+                Node n = nodes.get(i);
+                if (n.isTagged() || n.getParentWays().size() > 1 || n.referrers(Relation.class).count() > 0)
+                    fixNodes.add(n);
+            }
         }
-        if (needsUndo) {
-            // make sure we don't add the new nodes twice
-            UndoRedoHandler.getInstance().undo(1);
+
+        List<Node> orig = nodes.subList(pos1, pos3 + 1);
+        List<Node> arcNodes = new ArrayList<>(orig);
+        try {
+            cmds.addAll(worker(arcNodes, fixNodes, center, radius, maxAngle, clockWise));
+            if (!arcNodes.equals(orig)) {
+                fuseArc(ds, arcNodes, targetWays, cmds);
+            }
+        } finally {
+            if (needsUndo) {
+                // make sure we don't add the new nodes twice
+                UndoRedoHandler.getInstance().undo(1);
+            }
         }
+        if (cmds.isEmpty()) {
+            throw new JosmRuntimeException(tr("Nothing to do"));
+        }
         return cmds;
     }
 
+    /**
+     * Try to find out which nodes should be moved when a closed way is modified. The positions of the anchor
+     * nodes might not give the right order. Rotate the nodes so that each of the selected nodes is first
+     * and check which variant produces the shortest part of the way.
+     * @param w the closed way to modify
+     * @param anchorNodes the (selected) anchor nodes
+     * @return the way nodes, possibly rotated
+     * @throws JosmRuntimeException if no usable rotation was found
+     */
+    private static List<Node> findShortestPart(Way w, List<Node> anchorNodes) {
+        int bestRotate = 0;
+        double shortest = Double.MAX_VALUE;
+        final double wayLength = w.getLength();
+        for (int i = 0; i < w.getNodesCount() - 1; i++) {
+            List<Node> nodes = rotate(w, i);
+            List<Integer> positions = anchorNodes.stream().map(nodes::indexOf).sorted().collect(Collectors.toList());
+            double lenghth = getLength(nodes, positions.get(0), positions.get(2));
+            if (lenghth < shortest) {
+                bestRotate = i;
+                shortest = lenghth;
+            }
+        }
+        if (shortest >= wayLength / 2)
+            throw new JosmRuntimeException(tr("Don't know which part of closed way should be changed"));
+        return rotate(w, bestRotate);
+    }
+
+    private static List<Node> rotate(Way w, int distance) {
+        List<Node> nodes = new ArrayList<>(w.getNodes());
+        nodes.remove(nodes.size() - 1); // remove closing node
+        Collections.rotate(nodes, distance);
+        nodes.add(nodes.get(0));
+        return nodes;
+    }
+
+    private static double getLength(List<Node> nodes, int pos1, int pos3) {
+        Way tmp = new Way();
+        tmp.setNodes(nodes.subList(pos1, pos3+1));
+        return tmp.getLength();
+    }
+
     // code partly taken from AlignInCircleAction
-    private static List<Command> worker(List<Node> nodes, Set<Node> fixNodes, EastNorth center, double radius, double maxAngle) {
+    private static List<Command> worker(List<Node> nodes, Set<Node> origFixNodes, EastNorth center, double radius,
+            double maxAngle, boolean clockWise) {
         List<Command> cmds = new LinkedList<>();
 
         // Move each node to that distance from the center.
-        // Nodes that are not "fix" will be adjust making regular arcs.
-        int nodeCount = nodes.size();
+        // Nodes that are not "fix" will be adjusted making regular arcs.
 
-        List<Node> cwTest = new ArrayList<>(nodes);
-        if (cwTest.get(0) != cwTest.get(cwTest.size() - 1)) {
-            cwTest.add(cwTest.get(0));
-        }
-        boolean clockWise = Geometry.isClockwise(cwTest);
+        HashSet<Node> fixNodes = new HashSet<>(origFixNodes);
         double maxStep = Math.PI * 2 / (360.0 / maxAngle);
 
-        // Search first fixed node
-        int startPosition;
-        for (startPosition = 0; startPosition < nodeCount; startPosition++) {
-            if (fixNodes.contains(nodes.get(startPosition)))
-                break;
-        }
-        int i = startPosition; // Start position for current arc
+        double sumAbsDelta = 0;
+        int i = 0; // Start position for current arc
         int j; // End position for current arc
-        while (i < nodeCount) {
-            for (j = i + 1; j < nodeCount; j++) {
-                if (fixNodes.contains(nodes.get(j)))
+        while (i < nodes.size()) {
+            for (j = i + 1; j < nodes.size(); j++) {
+                if (fixNodes.contains(nodes.get(j))) {
                     break;
+                }
             }
             Node first = nodes.get(i);
+            PolarCoor pcFirst = new PolarCoor(radius, PolarCoor.computeAngle(first.getEastNorth(), center), center);
 
-            PolarCoor pcFirst = new PolarCoor(radius, PolarCoor.computeAngle(first.getEastNorth(), center), center);
-            addMoveCommandIfNeeded(first, pcFirst, cmds);
-            if (j < nodeCount) {
-                double delta;
-                PolarCoor pcLast = new PolarCoor(nodes.get(j).getEastNorth(), center);
-                delta = pcLast.angle - pcFirst.angle;
+            if (j < nodes.size()) {
+                Node last = nodes.get(j);
+                PolarCoor pcLast = new PolarCoor(last.getEastNorth(), center);
+                double delta = pcLast.angle - pcFirst.angle;
+                if ((!clockWise && delta < 0 || clockWise && delta > 0)
+                        && Math.signum(pcFirst.angle) == Math.signum(pcLast.angle)) {
+                    // cannot project node onto circle arc, ignore that it is fixed
+                    if (!last.isSelected() && fixNodes.remove(last)) {
+                        continue;
+                    } else {
+                        throw new JosmRuntimeException(tr("Too many fixed nodes"));
+                    }
+                }
                 if (!clockWise && delta < 0) {
                     delta += 2 * Math.PI;
                 } else if (clockWise && delta > 0) {
                     delta -= 2 * Math.PI;
                 }
+                sumAbsDelta += Math.abs(delta);
+                if (sumAbsDelta > 2 * Math.PI) {
+                    // something went wrong, we would add more than a full circle
+                    throw new JosmRuntimeException(tr("Would produce a loop"));
+                }
+
                 // do we have enough nodes to produce a nice circle?
                 int numToAdd = Math.max((int) Math.ceil(Math.abs(delta / maxStep)), j - i) - (j-i);
                 double step = delta / (numToAdd + j - i);
-                for (int k = i + 1; k < j; k++) {
+
+                // TODO: better mix new nodes and old nodes to reduce move distance
+                for (int k = i; k < j; k++) {
                     PolarCoor p = new PolarCoor(radius, pcFirst.angle + (k - i) * step, center);
                     addMoveCommandIfNeeded(nodes.get(k), p, cmds);
                 }
@@ -221 +296 @@
                     cmds.add(new AddCommand(nodes.get(0).getDataSet(), nNew));
                 }
                 j += numToAdd;
-                nodeCount += numToAdd;
             }
             i = j; // Update start point for next iteration
         }
@@ -237 +311 @@
         }
     }
 
+    private static void fuseArc(DataSet ds, List<Node> arcNodes, Set<Way> targetWays, Collection<Command> cmds) {
+        // replace each segment of the target ways with the corresponding nodes of the new arc
+        for (Way originalTw : targetWays) {
+            Way tw = new Way(originalTw);
+            boolean twWasChanged = false;
+            for (int i = 0; i < arcNodes.size(); i++) {
+                Node arcNode1 = arcNodes.get(i);
+                // we don't want to match nodes which where added by worker
+                if (arcNode1.getDataSet() != ds || !arcNode1.getParentWays().contains(originalTw))
+                    continue;
+
+                boolean changed = false;
+                for (int j = i + 1; j < arcNodes.size() && !changed; j++) {
+                    Node arcNode2 = arcNodes.get(j);
+                    if (arcNode2.getDataSet() != ds || !arcNode2.getParentWays().contains(originalTw))
+                        continue;
+                    changed = tryAddArc(tw, i, j, arcNodes);
+                    twWasChanged |= changed;
+                }
+            }
+            if (twWasChanged) {
+                cmds.add(new ChangeCommand(ds, originalTw, tw));
+            }
+        }
+    }
+
+    private static boolean tryAddArc(Way tw, int i, int j, List<Node> arcNodes) {
+        int pos1 = tw.getNodes().indexOf(arcNodes.get(i));
+        int pos2 = tw.getNodes().indexOf(arcNodes.get(j));
+        if (tw.isClosed()) {
+            if (pos1 - pos2 > 1 && pos2 == 0) {
+                pos2 = tw.getNodesCount() - 1;
+            } else if (pos2 - pos1 > 1 && pos1 == 0) {
+                pos1 = tw.getNodesCount() - 1;
+            }
+        }
+        if (pos2 + 1 == pos1) {
+            for (int k = i + 1; k < j; k++) {
+                tw.addNode(pos1, arcNodes.get(k));
+            }
+            return true;
+        } else if (pos2 - 1 == pos1) {
+            for (int k = j - 1; k > i; k--) {
+                tw.addNode(pos2, arcNodes.get(k));
+            }
+            return true;
+        }
+        return false;
+    }
+
 }

src/org/openstreetmap/josm/plugins/utilsplugin2/curves/CurveAction.java

@@ -20 +20 @@
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
 import org.openstreetmap.josm.data.osm.Way;
 import org.openstreetmap.josm.gui.Notification;
+import org.openstreetmap.josm.tools.JosmRuntimeException;
 import org.openstreetmap.josm.tools.Shortcut;
 
 // TODO: investigate splines
@@ -46 +47 @@
         List<Node> selectedNodes = new ArrayList<>(getLayerManager().getEditDataSet().getSelectedNodes());
         List<Way> selectedWays = new ArrayList<>(getLayerManager().getEditDataSet().getSelectedWays());
 
-        Collection<Command> cmds = CircleArcMaker.doCircleArc(selectedNodes, selectedWays);
-        if (cmds == null || cmds.isEmpty()) {
-            new Notification(tr("Could not use selection to create a curve")).setIcon(JOptionPane.WARNING_MESSAGE).show();
-        } else {
-            UndoRedoHandler.getInstance().add(new SequenceCommand("Create a curve", cmds));
+        String msg = null;
+        try {
+            Collection<Command> cmds = CircleArcMaker.doCircleArc(selectedNodes, selectedWays);
+            if (cmds == null || cmds.isEmpty()) {
+                msg = tr("Could not use selection to create a curve");
+            } else {
+                UndoRedoHandler.getInstance().add(new SequenceCommand("Create a curve", cmds));
+            }
+        } catch (JosmRuntimeException ex) {
+            msg = tr("Could not use selection to create a curve: {0}", ex.getMessage());
         }
+        if (msg != null) {
+            new Notification(msg).setIcon(JOptionPane.WARNING_MESSAGE).show();
+        }
     }
 
     @Override