Changeset 15335 in josm for trunk/src/org/openstreetmap/josm/data/validation/tests/UnconnectedWays.java

Timestamp:

2019-09-04T11:43:24+02:00 (5 years ago)

Author:

GerdP

Message:

fix #18051 and #18106
This is more or less a rewrite of the code, I hope I didn't introduce too many new problems.

• always consider all ways with matching tag when calculating unconnected nodes
• for partial tests filter the calculated errors (this means a small additional delay for this test when uploading data)
• fix recursion problem in isConnectedTo so that it doesn't stop too early
• remove code which excluded eg. ways with highway=cycleway + railway=abandoned or waterway=river + boundary=administrative
• improve performance, esp. use HashSet when calculating types of nodes in method addNode() and QuadBuckets only when searching is done. Together with other changes this reduces runtime by ~50%, so I hope the delay for the upload is not too high.

File:

• trunk/src/org/openstreetmap/josm/data/validation/tests/UnconnectedWays.java (modified) (24 diffs)

trunk/src/org/openstreetmap/josm/data/validation/tests/UnconnectedWays.java

@@ -8 +8 @@
 import java.awt.geom.Area;
 import java.awt.geom.Line2D;
-import java.awt.geom.Point2D;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -47 +46 @@
 public abstract class UnconnectedWays extends Test {
     private final int code;
+    private final boolean isHighwayTest;
+
+    protected abstract boolean isCandidate(OsmPrimitive p);
+
+    @Override
+    public boolean isPrimitiveUsable(OsmPrimitive p) {
+        return super.isPrimitiveUsable(p) && ((partialSelection && p instanceof Node) || isCandidate(p));
+    }
 
     /**
@@ -58 +65 @@
          */
         public UnconnectedHighways() {
-            super(tr("Unconnected highways"), UNCONNECTED_HIGHWAYS);
+            super(tr("Unconnected highways"), UNCONNECTED_HIGHWAYS, true);
         }
 
         @Override
-        public boolean isPrimitiveUsable(OsmPrimitive p) {
-            return super.isPrimitiveUsable(p) && p.hasKey(HIGHWAY);
+        protected boolean isCandidate(OsmPrimitive p) {
+            return p.hasKey(HIGHWAY);
         }
     }
@@ -76 +83 @@
          */
         public UnconnectedRailways() {
-            super(tr("Unconnected railways"), UNCONNECTED_RAILWAYS);
+            super(tr("Unconnected railways"), UNCONNECTED_RAILWAYS, false);
         }
 
         @Override
-        public boolean isPrimitiveUsable(OsmPrimitive p) {
-            return super.isPrimitiveUsable(p) && p.hasKey("railway");
+        protected boolean isCandidate(OsmPrimitive p) {
+            return p.hasKey(RAILWAY) && !p.hasTag(RAILWAY, "abandoned");
         }
     }
@@ -94 +101 @@
          */
         public UnconnectedWaterways() {
-            super(tr("Unconnected waterways"), UNCONNECTED_WATERWAYS);
+            super(tr("Unconnected waterways"), UNCONNECTED_WATERWAYS, false);
         }
 
         @Override
-        public boolean isPrimitiveUsable(OsmPrimitive p) {
-            return super.isPrimitiveUsable(p) && p.hasKey("waterway");
+        protected boolean isCandidate(OsmPrimitive p) {
+            return p.hasKey("waterway");
         }
     }
@@ -112 +119 @@
          */
         public UnconnectedNaturalOrLanduse() {
-            super(tr("Unconnected natural lands and landuses"), UNCONNECTED_NATURAL_OR_LANDUSE);
+            super(tr("Unconnected natural lands and landuses"), UNCONNECTED_NATURAL_OR_LANDUSE, false);
         }
 
         @Override
-        public boolean isPrimitiveUsable(OsmPrimitive p) {
-            return super.isPrimitiveUsable(p) && p.hasKey("natural", "landuse");
+        protected boolean isCandidate(OsmPrimitive p) {
+            return p.hasKey("natural", "landuse") && !p.hasTag("natural", "tree_row", "cliff");
         }
     }
@@ -130 +137 @@
          */
         public UnconnectedPower() {
-            super(tr("Unconnected power ways"), UNCONNECTED_POWER);
+            super(tr("Unconnected power ways"), UNCONNECTED_POWER, false);
         }
 
         @Override
-        public boolean isPrimitiveUsable(OsmPrimitive p) {
-            return super.isPrimitiveUsable(p) && p.hasTag("power", "line", "minor_line", "cable");
+        protected boolean isCandidate(OsmPrimitive p) {
+            return p.hasTag("power", "line", "minor_line", "cable");
         }
     }
@@ -142 +149 @@
     protected static final String PREFIX = ValidatorPrefHelper.PREFIX + "." + UnconnectedWays.class.getSimpleName();
 
-    private Set<MyWaySegment> ways;
-    private QuadBuckets<Node> endnodes; // nodes at end of way
-    private QuadBuckets<Node> endnodesHighway; // nodes at end of way
-    private QuadBuckets<Node> middlenodes; // nodes in middle of way
+    private List<MyWaySegment> waySegments;
+    private Set<Node> endnodes; // nodes at end of way
+    private Set<Node> middlenodes; // nodes in middle of way
     private Set<Node> othernodes; // nodes appearing at least twice
+    private QuadBuckets<Node> searchNodes;
+    private Set<Way> waysToTest;
+    private Set<Node> nodesToTest;
     private Area dsArea;
 
     private double mindist;
     private double minmiddledist;
+    private DataSet ds;
 
     /**
@@ -158 +168 @@
      */
     public UnconnectedWays(String title) {
-        this(title, UNCONNECTED_WAYS);
+        this(title, UNCONNECTED_WAYS, false);
 
     }
@@ -166 +176 @@
      * @param title The test title
      * @param code The test code
+     * @param isHighwayTest use {@code true} if test concerns highways or railways
      * @since 14468
      */
-    public UnconnectedWays(String title, int code) {
+    public UnconnectedWays(String title, int code, boolean isHighwayTest) {
         super(title, tr("This test checks if a way has an endpoint very near to another way."));
         this.code = code;
+        this.isHighwayTest = isHighwayTest;
     }
 
@@ -176 +188 @@
     public void startTest(ProgressMonitor monitor) {
         super.startTest(monitor);
-        ways = new HashSet<>();
-        endnodes = new QuadBuckets<>();
-        endnodesHighway = new QuadBuckets<>();
-        middlenodes = new QuadBuckets<>();
+        waySegments = new ArrayList<>();
+        waysToTest = new HashSet<>();
+        nodesToTest = new HashSet<>();
+        endnodes = new HashSet<>();
+        middlenodes = new HashSet<>();
         othernodes = new HashSet<>();
         mindist = Config.getPref().getDouble(PREFIX + ".node_way_distance", 10.0);
         minmiddledist = Config.getPref().getDouble(PREFIX + ".way_way_distance", 0.0);
-        DataSet dataSet = OsmDataManager.getInstance().getEditDataSet();
-        dsArea = dataSet == null ? null : dataSet.getDataSourceArea();
+        ds = OsmDataManager.getInstance().getEditDataSet();
+        dsArea = ds == null ? null : ds.getDataSourceArea();
     }
 
     protected Map<Node, Way> getWayEndNodesNearOtherHighway() {
         Map<Node, Way> map = new HashMap<>();
-        for (int iter = 0; iter < 1; iter++) {
-            for (MyWaySegment s : ways) {
-                if (isCanceled()) {
-                    map.clear();
-                    return map;
-                }
-                if (s.highway) {
-                    for (Node en : s.nearbyNodes(mindist)) {
-                        if (en == null || !endnodesHighway.contains(en)) {
-                            continue;
-                        }
-                        if (en.hasTag(HIGHWAY, "turning_circle", "bus_stop")
-                                || en.hasTag("amenity", "parking_entrance")
-                                || en.hasTag(RAILWAY, "buffer_stop")
-                                || en.isKeyTrue("noexit")
-                                || en.hasKey("entrance", "barrier")) {
-                            continue;
-                        }
-                        // to handle intersections of 't' shapes and similar
-                        if (en.isConnectedTo(s.w.getNodes(), 3 /* hops */, null)) {
-                            continue;
-                        }
-                        map.put(en, s.w);
-                    }
-                }
-            }
-        }
-        return map;
-    }
-
-    protected Map<Node, Way> getWayEndNodesNearOtherWay() {
-        Map<Node, Way> map = new HashMap<>();
-        for (MyWaySegment s : ways) {
+        for (MyWaySegment s : waySegments) {
             if (isCanceled()) {
                 map.clear();
@@ -227 +208 @@
             }
             for (Node en : s.nearbyNodes(mindist)) {
-                if (en.isConnectedTo(s.w.getNodes(), 3 /* hops */, null)) {
+                if (en.hasTag(HIGHWAY, "turning_circle", "bus_stop")
+                        || en.hasTag("amenity", "parking_entrance")
+                        || en.hasTag(RAILWAY, "buffer_stop")
+                        || en.isKeyTrue("noexit")
+                        || en.hasKey("entrance", "barrier")) {
                     continue;
                 }
-                if (((!s.highway && endnodesHighway.contains(en)) || endnodes.contains(en)) && !s.w.concernsArea()) {
+                // to handle intersections of 't' shapes and similar
+                if (!en.isConnectedTo(s.w.getNodes(), 3 /* hops */, null)) {
                     map.put(en, s.w);
                 }
@@ -238 +224 @@
     }
 
-    protected Map<Node, Way> getWayNodesNearOtherWay() {
+    protected Map<Node, Way> getWayEndNodesNearOtherWay() {
         Map<Node, Way> map = new HashMap<>();
-        for (MyWaySegment s : ways) {
+        for (MyWaySegment s : waySegments) {
             if (isCanceled()) {
                 map.clear();
                 return map;
             }
-            for (Node en : s.nearbyNodes(minmiddledist)) {
-                if (en.isConnectedTo(s.w.getNodes(), 3 /* hops */, null)) {
-                    continue;
-                }
-                if (!middlenodes.contains(en)) {
-                    continue;
-                }
-                map.put(en, s.w);
+            if (!s.concernsArea) {
+                for (Node en : s.nearbyNodes(mindist)) {
+                    if (!en.isConnectedTo(s.w.getNodes(), 3 /* hops */, null)) {
+                        map.put(en, s.w);
+                    }
+                }
             }
         }
@@ -258 +242 @@
     }
 
-    protected Map<Node, Way> getConnectedWayEndNodesNearOtherWay() {
+    protected Map<Node, Way> getWayNodesNearOtherWay() {
         Map<Node, Way> map = new HashMap<>();
-        for (MyWaySegment s : ways) {
+        for (MyWaySegment s : waySegments) {
             if (isCanceled()) {
                 map.clear();
@@ -266 +250 @@
             }
             for (Node en : s.nearbyNodes(minmiddledist)) {
-                if (en.isConnectedTo(s.w.getNodes(), 3 /* hops */, null)) {
-                    continue;
-                }
-                if (!othernodes.contains(en)) {
-                    continue;
-                }
-                map.put(en, s.w);
+                if (!en.isConnectedTo(s.w.getNodes(), 3 /* hops */, null)) {
+                    map.put(en, s.w);
+                }
             }
         }
@@ -280 +260 @@
     protected final void addErrors(Severity severity, Map<Node, Way> errorMap, String message) {
         for (Map.Entry<Node, Way> error : errorMap.entrySet()) {
+            Node node = error.getKey();
+            Way way = error.getValue();
+            if (partialSelection && !nodesToTest.contains(node) && !waysToTest.contains(way))
+                continue;
             errors.add(TestError.builder(this, severity, code)
                     .message(message)
-                    .primitives(error.getKey(), error.getValue())
-                    .highlight(error.getKey())
+                    .primitives(node, way)
+                    .highlight(node)
                     .build());
         }
@@ -290 +274 @@
     @Override
     public void endTest() {
-        addErrors(Severity.WARNING, getWayEndNodesNearOtherHighway(), tr("Way end node near other highway"));
-        addErrors(Severity.WARNING, getWayEndNodesNearOtherWay(), tr("Way end node near other way"));
+        if (ds == null)
+            return;
+
+        for (Way w : ds.getWays()) {
+            if (w.isUsable() && isCandidate(w) && w.getRealNodesCount() > 1
+                    // don't complain about highways ending near platforms
+                    && !w.hasTag(HIGHWAY, "platform") && !w.hasTag(RAILWAY, "platform", "platform_edge")
+                    ) {
+                waySegments.addAll(getWaySegments(w));
+                addNode(w.firstNode(), endnodes);
+                addNode(w.lastNode(), endnodes);
+            }
+        }
+        searchNodes = new QuadBuckets<>();
+        searchNodes.addAll(endnodes);
+        if (isHighwayTest) {
+            addErrors(Severity.WARNING, getWayEndNodesNearOtherHighway(), tr("Way end node near other highway"));
+        } else {
+            addErrors(Severity.WARNING, getWayEndNodesNearOtherWay(), tr("Way end node near other way"));
+        }
+
         /* the following two use a shorter distance */
-        if (minmiddledist > 0.0) {
+        boolean includeOther = isBeforeUpload ? ValidatorPrefHelper.PREF_OTHER_UPLOAD.get() : ValidatorPrefHelper.PREF_OTHER.get();
+        if (minmiddledist > 0.0 && includeOther) {
+            searchNodes.clear();
+            searchNodes.addAll(middlenodes);
             addErrors(Severity.OTHER, getWayNodesNearOtherWay(), tr("Way node near other way"));
-            addErrors(Severity.OTHER, getConnectedWayEndNodesNearOtherWay(), tr("Connected way end node near other way"));
-        }
-        ways = null;
+            searchNodes.clear();
+            searchNodes.addAll(othernodes);
+            addErrors(Severity.OTHER, getWayNodesNearOtherWay(), tr("Connected way end node near other way"));
+        }
+        waySegments = null;
         endnodes = null;
-        endnodesHighway = null;
         middlenodes = null;
         othernodes = null;
+        searchNodes = null;
         dsArea = null;
+        ds = null;
         super.endTest();
     }
 
     private class MyWaySegment {
-        private final Line2D line;
         public final Way w;
-        public final boolean isAbandoned;
-        public final boolean isBoundary;
-        public final boolean highway;
-        private final double len;
-        private Set<Node> nearbyNodeCache;
-        private double nearbyNodeCacheDist = -1.0;
         private final Node n1;
         private final Node n2;
-
-        MyWaySegment(Way w, Node n1, Node n2) {
+        private final boolean concernsArea;
+
+        MyWaySegment(Way w, Node n1, Node n2, boolean concersArea) {
             this.w = w;
-            String railway = w.get(RAILWAY);
-            String highway = w.get(HIGHWAY);
-            this.isAbandoned = "abandoned".equals(railway) || w.isKeyTrue("disused");
-            this.highway = (highway != null || railway != null) && !isAbandoned;
-            this.isBoundary = !this.highway && w.hasTag("boundary", "administrative");
-            line = new Line2D.Double(n1.getEastNorth().east(), n1.getEastNorth().north(),
-                    n2.getEastNorth().east(), n2.getEastNorth().north());
-            len = line.getP1().distance(line.getP2());
             this.n1 = n1;
             this.n2 = n2;
+            this.concernsArea = concersArea;
         }
 
@@ -344 +340 @@
             if (coord == null)
                 return false;
-            Point2D p = new Point2D.Double(coord.east(), coord.north());
-            if (line.getP1().distance(p) > len+dist)
-                return false;
-            if (line.getP2().distance(p) > len+dist)
-                return false;
-            return line.ptSegDist(p) < dist;
-        }
-
-        public List<LatLon> getBounds(double fudge) {
+            EastNorth en1 = n1.getEastNorth();
+            EastNorth en2 = n2.getEastNorth();
+            return Line2D.ptSegDist(en1.getX(), en1.getY(), en2.getX(), en2.getY(), coord.getX(), coord.getY()) < dist;
+        }
+
+        public BBox getBounds(double fudge) {
             double x1 = n1.getCoor().lon();
             double x2 = n2.getCoor().lon();
@@ -369 +362 @@
             LatLon topLeft = new LatLon(y2+fudge, x1-fudge);
             LatLon botRight = new LatLon(y1-fudge, x2+fudge);
-            List<LatLon> ret = new ArrayList<>(2);
-            ret.add(topLeft);
-            ret.add(botRight);
-            return ret;
+            return new BBox(topLeft, botRight);
         }
 
         public Collection<Node> nearbyNodes(double dist) {
-            // If you're looking for nodes that are farther away that we looked for last time,
-            // the cached result is no good
-            if (dist > nearbyNodeCacheDist) {
-                nearbyNodeCache = null;
-            }
-            if (nearbyNodeCache != null) {
-                // If we've cached an area greater than the
-                // one now being asked for...
-                if (nearbyNodeCacheDist > dist) {
-                    // Used the cached result and trim out
-                    // the nodes that are not in the smaller
-                    // area, but keep the old larger cache.
-                    Set<Node> trimmed = new HashSet<>(nearbyNodeCache);
-                    Set<Node> initial = new HashSet<>(nearbyNodeCache);
-                    for (Node n : initial) {
-                        if (!nearby(n, dist)) {
-                            trimmed.remove(n);
-                        }
-                    }
-                    return trimmed;
-                }
-                return nearbyNodeCache;
-            }
             /*
-             * We know that any point near the line must be at
+             * We know that any point near the line segment must be at
              * least as close as the other end of the line, plus
              * a little fudge for the distance away ('dist').
              */
 
-            // This needs to be a hash set because the searches
-            // overlap a bit and can return duplicate nodes.
-            nearbyNodeCache = null;
-            List<LatLon> bounds = this.getBounds(dist * (360.0d / (Ellipsoid.WGS84.a * 2 * Math.PI)));
-            List<Node> foundNodes = endnodesHighway.search(new BBox(bounds.get(0), bounds.get(1)));
-            foundNodes.addAll(endnodes.search(new BBox(bounds.get(0), bounds.get(1))));
-
+            BBox bounds = this.getBounds(dist * (360.0d / (Ellipsoid.WGS84.a * 2 * Math.PI)));
+            List<Node> result = null;
+            List<Node> foundNodes = searchNodes.search(bounds);
             for (Node n : foundNodes) {
                 if (!nearby(n, dist) || !n.getCoor().isIn(dsArea)) {
@@ -419 +382 @@
                 // so defer as much of the work as possible, like
                 // allocating the hash set
-                if (nearbyNodeCache == null) {
-                    nearbyNodeCache = new HashSet<>();
-                }
-                nearbyNodeCache.add(n);
-            }
-            nearbyNodeCacheDist = dist;
-            if (nearbyNodeCache == null) {
-                nearbyNodeCache = Collections.emptySet();
-            }
-            return nearbyNodeCache;
+                if (result == null) {
+                    result = new ArrayList<>();
+                }
+                result.add(n);
+            }
+            return result == null ? Collections.emptyList() : result;
         }
     }
@@ -434 +393 @@
     List<MyWaySegment> getWaySegments(Way w) {
         List<MyWaySegment> ret = new ArrayList<>();
-        if (!w.isUsable()
-                || w.hasKey("barrier")
-                || w.hasTag("natural", "cliff"))
+        if (!w.isUsable() || w.isKeyTrue("disused"))
             return ret;
 
         int size = w.getNodesCount();
-        if (size < 2)
-            return ret;
+        boolean concersArea = w.concernsArea();
         for (int i = 1; i < size; ++i) {
             if (i < size-1) {
@@ -449 +405 @@
             Node b = w.getNode(i);
             if (a.isDrawable() && b.isDrawable()) {
-                MyWaySegment ws = new MyWaySegment(w, a, b);
-                if (ws.isBoundary || ws.isAbandoned) {
-                    continue;
-                }
+                MyWaySegment ws = new MyWaySegment(w, a, b, concersArea);
                 ret.add(ws);
             }
@@ -461 +414 @@
     @Override
     public void visit(Way w) {
-        // do not consider empty ways
-        if (w.getNodesCount() > 0
-                // ignore addr:interpolation ways as they are not physical features and most of
-                // the time very near the associated highway, which is perfectly normal, see #9332
-                && !w.hasKey("addr:interpolation")
-                // similarly for public transport platforms, tree rows
-                && !w.hasTag(HIGHWAY, "platform") && !w.hasTag(RAILWAY, "platform", "platform_edge") && !w.hasTag("natural", "tree_row")
-                ) {
-            ways.addAll(getWaySegments(w));
-            QuadBuckets<Node> set = endnodes;
-            if (w.hasKey(HIGHWAY, RAILWAY)) {
-                set = endnodesHighway;
-            }
-            addNode(w.firstNode(), set);
-            addNode(w.lastNode(), set);
-        }
-    }
-
-    private void addNode(Node n, QuadBuckets<Node> s) {
+        if (partialSelection) {
+            waysToTest.add(w);
+        }
+    }
+
+    @Override
+    public void visit(Node n) {
+        if (partialSelection) {
+            nodesToTest.add(n);
+        }
+    }
+
+    private void addNode(Node n, Set<Node> s) {
         boolean m = middlenodes.contains(n);
         boolean e = endnodes.contains(n);
-        boolean eh = endnodesHighway.contains(n);
         boolean o = othernodes.contains(n);
-        if (!m && !e && !o && !eh) {
+        if (!m && !e && !o) {
             s.add(n);
         } else if (!o) {
@@ -490 +436 @@
             if (e) {
                 endnodes.remove(n);
-            } else if (eh) {
-                endnodesHighway.remove(n);
             } else {
                 middlenodes.remove(n);