Ticket #18051: 18051.patch

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

@@ -7 +7 @@
 
 import java.awt.geom.Area;
 import java.awt.geom.Line2D;
-import java.awt.geom.Point2D;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -62 +61 @@
 
         @Override
         public boolean isPrimitiveUsable(OsmPrimitive p) {
-            return super.isPrimitiveUsable(p) && p.hasKey(HIGHWAY);
+            return super.isPrimitiveUsable(p) && ((partialSelection && p instanceof Node) || p.hasKey(HIGHWAY));
         }
     }
 
@@ -80 +79 @@
 
         @Override
         public boolean isPrimitiveUsable(OsmPrimitive p) {
-            return super.isPrimitiveUsable(p) && p.hasKey("railway");
+            return super.isPrimitiveUsable(p) && ((partialSelection && p instanceof Node) || p.hasKey("railway"));
         }
     }
 
@@ -98 +97 @@
 
         @Override
         public boolean isPrimitiveUsable(OsmPrimitive p) {
-            return super.isPrimitiveUsable(p) && p.hasKey("waterway");
+            return super.isPrimitiveUsable(p) && ((partialSelection && p instanceof Node) || p.hasKey("waterway"));
         }
     }
 
@@ -116 +115 @@
 
         @Override
         public boolean isPrimitiveUsable(OsmPrimitive p) {
-            return super.isPrimitiveUsable(p) && p.hasKey("natural", "landuse");
+            return super.isPrimitiveUsable(p) && ((partialSelection && p instanceof Node) || p.hasKey("natural", "landuse"));
         }
     }
 
@@ -142 +141 @@
     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 Set<Node> endnodes; // nodes at end of way
+    private Set<Node> endnodesHighway; // 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;
 
     /**
      * Constructs a new {@code UnconnectedWays} test.
@@ -176 +179 @@
     public void startTest(ProgressMonitor monitor) {
         super.startTest(monitor);
         ways = new HashSet<>();
-        endnodes = new QuadBuckets<>();
-        endnodesHighway = new QuadBuckets<>();
-        middlenodes = new QuadBuckets<>();
+        waysToTest = new HashSet<>();
+        nodesToTest = new HashSet<>();
+        endnodes = new HashSet<>();
+        endnodesHighway = 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);
+        for (MyWaySegment s : ways) {
+            if (isCanceled()) {
+                map.clear();
+                return map;
+            }
+            if (s.highway) {
+                for (Node en : s.nearbyNodes(mindist)) {
+                    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);
                 }
             }
         }
@@ -244 +244 @@
                 map.clear();
                 return map;
             }
+            s.nearbyNodeCache = null;
             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);
             }
         }
@@ -257 +255 @@
         return map;
     }
 
-    protected Map<Node, Way> getConnectedWayEndNodesNearOtherWay() {
-        Map<Node, Way> map = new HashMap<>();
-        for (MyWaySegment s : ways) {
-            if (isCanceled()) {
-                map.clear();
-                return map;
-            }
-            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);
-            }
-        }
-        return map;
-    }
-
     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());
         }
     }
@@ -289 +271 @@
 
     @Override
     public void endTest() {
+        for (Way w : ds.getWays()) {
+            if (isPrimitiveUsable(w)
+                    // 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));
+                Set<Node> set = endnodes;
+                if (w.hasKey(HIGHWAY, RAILWAY)) {
+                    set = endnodesHighway;
+                }
+                addNode(w.firstNode(), set);
+                addNode(w.lastNode(), set);
+            }
+        }
+        searchNodes = new QuadBuckets<>();
+        searchNodes.addAll(endnodes);
+        searchNodes.addAll(endnodesHighway);
         addErrors(Severity.WARNING, getWayEndNodesNearOtherHighway(), tr("Way end node near other highway"));
         addErrors(Severity.WARNING, getWayEndNodesNearOtherWay(), tr("Way end node near other way"));
         /* the following two use a shorter distance */
         if (minmiddledist > 0.0) {
+            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"));
+            searchNodes.clear();
+            searchNodes.addAll(othernodes);
+            addErrors(Severity.OTHER, getWayNodesNearOtherWay(), tr("Connected way end node near other way"));
         }
         ways = null;
         endnodes = null;
@@ -301 +307 @@
         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;
@@ -320 +326 @@
         MyWaySegment(Way w, Node n1, Node n2) {
             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.highway = (w.hasKey(HIGHWAY) || 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;
         }
@@ -343 +345 @@
             EastNorth coord = n.getEastNorth();
             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;
+            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 List<LatLon> getBounds(double fudge) {
+        public BBox getBounds(double fudge) {
             double x1 = n1.getCoor().lon();
             double x2 = n2.getCoor().lon();
             if (x1 > x2) {
@@ -368 +367 @@
             }
             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) {
@@ -407 +403 @@
             // 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> foundNodes = searchNodes.search(bounds);
             for (Node n : foundNodes) {
-                if (!nearby(n, dist) || !n.getCoor().isIn(dsArea)) {
+                if (!nearby(n, dist) || (!n.isModified() && !n.getCoor().isIn(dsArea))) {
                     continue;
                 }
                 // It is actually very rare for us to find a node
@@ -460 +454 @@
 
     @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);
+        if (partialSelection) {
+            waysToTest.add(w);
         }
     }
 
-    private void addNode(Node n, QuadBuckets<Node> s) {
+    @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);

test/unit/org/openstreetmap/josm/data/validation/tests/UnconnectedWaysTest.java

@@ -14 +14 @@
 import org.junit.Test;
 import org.openstreetmap.josm.JOSMFixture;
 import org.openstreetmap.josm.data.osm.DataSet;
+import org.openstreetmap.josm.gui.MainApplication;
+import org.openstreetmap.josm.gui.layer.OsmDataLayer;
 import org.openstreetmap.josm.gui.progress.NullProgressMonitor;
 import org.openstreetmap.josm.io.IllegalDataException;
 import org.openstreetmap.josm.io.OsmReader;
+import org.openstreetmap.josm.testutils.JOSMTestRules;
 
 /**
  * Unit tests of {@code UnconnectedWays} class.
@@ -24 +27 @@
 public class UnconnectedWaysTest {
 
     private UnconnectedWays bib;
-
+    public JOSMTestRules test = new JOSMTestRules().main();
     /**
      * Setup test.
      * @throws Exception if the test cannot be initialized
@@ -34 +37 @@
         bib = new UnconnectedWays.UnconnectedHighways();
         JOSMFixture.createUnitTestFixture().init();
         bib.initialize();
-        bib.startTest(null);
     }
 
     /**
@@ -47 +49 @@
     public void testTicket6313() throws IOException, IllegalDataException, FileNotFoundException {
         try (InputStream fis = Files.newInputStream(Paths.get("data_nodist/UnconnectedWaysTest.osm"))) {
             final DataSet ds = OsmReader.parseDataSet(fis, NullProgressMonitor.INSTANCE);
+            MainApplication.getLayerManager().addLayer(new OsmDataLayer(ds, null, null));
+
+            bib.startTest(null);
             bib.visit(ds.allPrimitives());
             bib.endTest();
             assertThat(bib.getErrors(), isEmpty());