Changeset 35440 in osm for applications/editors/josm/plugins

Timestamp:

2020-05-13T11:34:47+02:00 (5 years ago)

Author:

gerdp

Message:

see #josm19188: circle arc: Issues with three selected nodes and a selected closed way

• fix calculation of clockwise flag
• improve positioning of added nodes
• ignore fixed nodes when they would produce loops
• add more plausibility checks
• correct handling of other ways connected to the modified part so that new nodes are also added to those ways which share segements with the selected nodes

Location:

applications/editors/josm/plugins/utilsplugin2

Files:

• build.xml (modified) (1 diff)
• src/org/openstreetmap/josm/plugins/utilsplugin2/curves/CircleArcMaker.java (modified) (7 diffs)
• src/org/openstreetmap/josm/plugins/utilsplugin2/curves/CurveAction.java (modified) (2 diffs)

applications/editors/josm/plugins/utilsplugin2/build.xml

@@ -5 +5 @@
     <property name="commit.message" value="[josm_utilsplugin2]: select boundary by double-click; multitagger table highlights"/>
     <!-- enter the *lowest* JOSM version this plugin is currently compatible with -->
-    <property name="plugin.main.version" value="14518"/>
+    <property name="plugin.main.version" value="14654"/>
 
     <property name="plugin.author" value="Kalle Lampila, Upliner, Zverik, akks, joshdoe and others"/>

applications/editors/josm/plugins/utilsplugin2/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;
@@ -11 +13 @@
 import java.util.List;
 import java.util.Set;
+import java.util.stream.Collectors;
 
 import org.openstreetmap.josm.command.AddCommand;
@@ -23 +26 @@
 import org.openstreetmap.josm.data.osm.DataSet;
 import org.openstreetmap.josm.data.osm.Node;
+import org.openstreetmap.josm.data.osm.Relation;
 import org.openstreetmap.josm.data.osm.Way;
 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;
 
 /**
@@ -107 +112 @@
 
         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));
@@ -126 +133 @@
 
         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);
-        }
-
-        Set<Node> fixNodes = new HashSet<>(anchorNodes);
-        if (!selectedWays.isEmpty()) {
-            nodes.stream().filter(n -> n.getParentWays().size() > 1 || n.isTagged()).forEach(fixNodes::add);
-        }
+            anchorNodes = Arrays.asList(n1, n2, n3);
+        }
+
+
+        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()) {
@@ -145 +162 @@
         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));
-        }
-        if (needsUndo) {
-            // make sure we don't add the new nodes twice
-            UndoRedoHandler.getInstance().undo(1);
+        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.isReferredByWays(2) || n.referrers(Relation.class).count() > 0) {
+                    fixNodes.add(n);
+                }
+            }
+        }
+
+        List<Node> orig = nodes.subList(pos1, pos3 + 1);
+        List<Node> arcNodes = new ArrayList<>(orig);
+
+        Set<Way> targetWays = new HashSet<>();
+        if (!selectedWays.isEmpty()) {
+            orig.forEach(n -> targetWays.addAll(n.getParentWays()));
+        } else {
+            targetWays.add(w);
+        }
+        try {
+            List<Command> modCmds = worker(arcNodes, fixNodes, center, radius, maxAngle, clockwise);
+            cmds.addAll(modCmds);
+            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("Could not detect which part of the 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> origNodes, 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();
-
-        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);
+        // Nodes that are not "fix" will be adjusted making regular arcs.
+        List<Node> nodes = new ArrayList<>(origNodes);
+        Set<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()) {
+            Node first = nodes.get(i);
+            PolarCoor pcFirst = new PolarCoor(radius, PolarCoor.computeAngle(first.getEastNorth(), center), center);
+            for (j = i + 1; j < nodes.size(); j++) {
+                Node test = nodes.get(j);
+                if (fixNodes.contains(test)) {
                     break;
-            }
-            Node first = nodes.get(i);
-
-            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 (!clockWise && delta < 0) {
+                }
+            }
+
+            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;
+                    }
+                    if (!first.isSelected() && fixNodes.remove(first)) {
+                        // try again with fewer fixed nodes
+                        return worker(origNodes, fixNodes, center, radius, maxAngle, clockwise);
+                    }
+                }
+                if (!clockwise && delta < 0) {
                     delta += 2 * Math.PI;
-                } else if (clockWise && delta > 0) {
+                } else if (clockwise && delta > 0) {
                     delta -= 2 * Math.PI;
                 }
+                sumAbsDelta += Math.abs(delta);
+                if (sumAbsDelta > 2 * Math.PI) {
+                    // something went really wrong, we would add more than a full circle
+                    throw new JosmRuntimeException("Would create 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);
+                int numToAdd = Math.max((int) Math.ceil(Math.abs(delta / maxStep)), j - i) - (j - i);
+                int added = 0;
                 double step = delta / (numToAdd + j - i);
-                for (int k = i + 1; k < j; k++) {
-                    PolarCoor p = new PolarCoor(radius, pcFirst.angle + (k - i) * step, center);
-                    addMoveCommandIfNeeded(nodes.get(k), p, cmds);
-                }
-                // add needed nodes
-                for (int k = j; k < j + numToAdd; k++) {
-                    PolarCoor p = new PolarCoor(radius, pcFirst.angle + (k - i) * step, center);
-                    Node nNew = new Node(p.toEastNorth());
-                    nodes.add(k, nNew);
-                    cmds.add(new AddCommand(nodes.get(0).getDataSet(), nNew));
-                }
-                j += numToAdd;
-                nodeCount += numToAdd;
+
+                // move existing nodes or add new nodes
+                List<Node> oldNodes = new ArrayList<>(nodes.subList(i, j));
+                PolarCoor ref = pcFirst;
+                for (int k = i; k < j + numToAdd; k++) {
+                    PolarCoor pc1 = new PolarCoor(radius, pcFirst.angle + (k - i) * step, center);
+                    if (!oldNodes.isEmpty()) {
+                        PolarCoor pc2 = new PolarCoor(oldNodes.get(0).getEastNorth(), center);
+                        if (pc2.angle < ref.angle && ref.angle < 0 && step > 0
+                                || pc2.angle > ref.angle && ref.angle > 0 && step < 0) {
+                            // projected node would produce a loop
+                            pc2 = ref; //
+                        }
+
+                        double delta2 = ref.angle - pc2.angle;
+                        if (ref.angle < 0 && pc2.angle > 0) {
+                            delta2 += 2 * Math.PI;
+                        } else if (ref.angle > 0 && pc2.angle < 0) {
+                            delta2 -= 2 * Math.PI;
+                        }
+                        if (added >= numToAdd || Math.abs(delta2) < Math.abs(step * 1.5)) {
+                            // existing node is close enough
+                            addMoveCommandIfNeeded(oldNodes.remove(0), pc1, cmds);
+                            ref = pc1;
+                        }
+                    }
+                    if (ref != pc1) {
+                        ref = pc1;
+                        Node nNew = new Node(pc1.toEastNorth());
+                        nodes.add(k, nNew);
+                        cmds.add(new AddCommand(nodes.get(0).getDataSet(), nNew));
+                        ++added;
+                    }
+                }
+
+                j += added;
             }
             i = j; // Update start point for next iteration
         }
+        origNodes.clear();
+        origNodes.addAll(nodes);
         return cmds;
     }
@@ -235 +345 @@
     }
 
+    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;
+    }
 }

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

@@ -21 +21 @@
 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;
 
@@ -47 +48 @@
         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();
         }
     }