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

Timestamp:

2024-02-21T08:25:24+01:00 (11 months ago)

Author:

GerdP

Message:

fix #23505: incorrect number of modified ways in command, improve performance, fix memory leak

• collect modifed ways to avoid two change commands per way
• use ChangeNodesCommand instead of ChangeCommand to avoid memory leak
• use core code to check if nodes are outside downloaded area
• store selected ways in HashSet to avoid multiple sequential searches in averageNearbyNodes()

File:

• applications/editors/josm/plugins/simplifyarea/src/org/openstreetmap/josm/plugins/simplifyarea/SimplifyAreaAction.java (modified) (15 diffs)

applications/editors/josm/plugins/simplifyarea/src/org/openstreetmap/josm/plugins/simplifyarea/SimplifyAreaAction.java

@@ -14 +14 @@
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -22 +23 @@
 
 import org.openstreetmap.josm.actions.JosmAction;
-import org.openstreetmap.josm.command.ChangeCommand;
+import org.openstreetmap.josm.command.ChangeNodesCommand;
 import org.openstreetmap.josm.command.Command;
 import org.openstreetmap.josm.command.DeleteCommand;
 import org.openstreetmap.josm.command.MoveCommand;
 import org.openstreetmap.josm.command.SequenceCommand;
-import org.openstreetmap.josm.data.Bounds;
 import org.openstreetmap.josm.data.UndoRedoHandler;
 import org.openstreetmap.josm.data.coor.ILatLon;
@@ -41 +41 @@
 import org.openstreetmap.josm.tools.ImageProvider;
 import org.openstreetmap.josm.tools.Shortcut;
-import org.openstreetmap.josm.tools.Utils;
 
 public final class SimplifyAreaAction extends JosmAction {
@@ -49 +48 @@
                 Shortcut.registerShortcut("tools:simplifyArea", tr("More tools: {0}", tr("Simplify Area")), KeyEvent.VK_Y, Shortcut.CTRL_SHIFT),
                 true, "simplifyarea", true);
-    }
-
-    private List<Bounds> getCurrentEditBounds() {
-        return MainApplication.getLayerManager().getEditLayer().data.getDataSourceBounds();
-    }
-
-    private static boolean isInBounds(final Node node, final List<Bounds> bounds) {
-        for (final Bounds b : bounds) {
-            if (b.contains(node)) {
-                return true;
-            }
-        }
-        return false;
     }
 
@@ -91 +77 @@
     @Override
     public void actionPerformed(final ActionEvent e) {
-        final Collection<OsmPrimitive> selection = getLayerManager().getEditDataSet().getSelected();
-
-        final List<Bounds> bounds = getCurrentEditBounds();
-        for (final OsmPrimitive prim : selection) {
-            if (prim instanceof Way && bounds.size() > 0) {
-                final Way way = (Way) prim;
-                // We check if each node of each way is at least in one download
-                // bounding box. Otherwise nodes may get deleted that are necessary by
-                // unloaded ways (see Ticket #1594)
-                for (final Node node : way.getNodes()) {
-                    if (!isInBounds(node, bounds)) {
-                        if (!confirmWayWithNodesOutsideBoundingBox()) {
-                            return;
-                        }
-                        break;
-                    }
-                }
-            }
-        }
-        final Collection<Way> ways = Utils.filteredCollection(selection, Way.class);
+        final Collection<Way> ways = new LinkedHashSet<>(getLayerManager().getEditDataSet().getSelectedWays());
+        ways.removeIf(w -> !w.isUsable());
+        int checkRes = Command.checkOutlyingOrIncompleteOperation(ways, null);
+        if ((checkRes & Command.IS_OUTSIDE) != 0) {
+            if (!confirmWayWithNodesOutsideBoundingBox())
+                return;
+        }
         if (ways.isEmpty()) {
             alertSelectAtLeastOneWay();
@@ -147 +120 @@
 
         final Collection<Command> allCommands = new ArrayList<>();
+        Map<Way, List<Node>> modifiedWays = new HashMap<>();
 
         if (!nodesReallyToRemove.isEmpty()) {
@@ -160 +134 @@
                         nodes.add(nodes.get(0));
                     }
-
-                    final Way newWay = new Way(way);
-                    newWay.setNodes(nodes);
-                    allCommands.add(new ChangeCommand(way, newWay));
+                    allCommands.add(new ChangeNodesCommand(way, nodes));
+                    modifiedWays.put(way, nodes);
                 }
             }
@@ -170 +142 @@
         }
 
-        final Collection<Command> avgCommands = averageNearbyNodes(ways, nodesReallyToRemove);
+        final Collection<Command> avgCommands = averageNearbyNodes(ways, nodesReallyToRemove, modifiedWays);
         if (avgCommands != null && !avgCommands.isEmpty()) {
             allCommands.add(new SequenceCommand(tr("average nearby nodes"), avgCommands));
@@ -176 +148 @@
 
         if (!allCommands.isEmpty()) {
-            final SequenceCommand rootCommand = new SequenceCommand(trn("Simplify {0} way", "Simplify {0} ways", allCommands.size(), allCommands.size()), allCommands);
+            final SequenceCommand rootCommand = new SequenceCommand(trn("Simplify {0} way", "Simplify {0} ways", modifiedWays.size(), modifiedWays.size()), allCommands);
             UndoRedoHandler.getInstance().add(rootCommand);
             MainApplication.getMap().repaint();
@@ -200 +172 @@
 
     // average nearby nodes
-    private static Collection<Command> averageNearbyNodes(final Collection<Way> ways, final Collection<Node> nodesAlreadyDeleted) {
+    private static Collection<Command> averageNearbyNodes(final Collection<Way> ways, final Collection<Node> nodesAlreadyDeleted, Map<Way, List<Node>> modifiedWays) {
         final double mergeThreshold = Config.getPref().getDouble(SimplifyAreaPreferenceSetting.MERGE_THRESHOLD, 0.2);
 
@@ -254 +226 @@
 
                     // test if both nodes have same parents
-                    if (!referrers.containsAll(referrers2) || !referrers2.containsAll(referrers)) {
+                    if (referrers.size() != referrers2.size() || !referrers2.containsAll(referrers)) {
                         continue;
                     }
@@ -290 +262 @@
             if (nodesToDelete.removeAll(coordMap.keySet())) {
                 nodesToDelete2.addAll(nodesToDelete);
-                final Way newWay = new Way(way);
                 final List<Node> nodes = way.getNodes();
                 final boolean closed = nodes.get(0).equals(nodes.get(nodes.size() - 1));
@@ -301 +272 @@
                 }
 
-                newWay.setNodes(nodes);
                 if (!way.getNodes().equals(nodes)) {
-                    commands.add(new ChangeCommand(way, newWay));
+                    List<Node> nodes1 = modifiedWays.get(way);
+                    if (nodes1 == null || !nodes.equals(nodes1)) {
+                        commands.add(new ChangeNodesCommand(way, nodes));
+                        modifiedWays.put(way, nodes);
+                    }
                 }
             }
@@ -421 +395 @@
 
     public static double computeConvectAngle(final ILatLon coord1, final ILatLon coord2, final ILatLon coord3) {
-        final double angle =  Math.abs(heading(coord2, coord3) - heading(coord1, coord2));
+        final double angle = Math.abs(heading(coord2, coord3) - heading(coord1, coord2));
         return Math.toDegrees(angle < Math.PI ? angle : 2 * Math.PI - angle);
     }
@@ -436 +410 @@
     }
 
-    public static double R = 6378135;
+    public static final double R = 6378135;
 
     public static double crossTrackError(final ILatLon l1, final ILatLon l2, final ILatLon l3) {