Index: src/org/openstreetmap/josm/data/validation/tests/UnconnectedWays.java =================================================================== --- src/org/openstreetmap/josm/data/validation/tests/UnconnectedWays.java (revision 15328) +++ src/org/openstreetmap/josm/data/validation/tests/UnconnectedWays.java (working copy) @@ -7,7 +7,6 @@ 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,7 +61,7 @@ @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,7 +79,7 @@ @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,7 +97,7 @@ @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,7 +115,7 @@ @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,14 +141,18 @@ protected static final String PREFIX = ValidatorPrefHelper.PREFIX + "." + UnconnectedWays.class.getSimpleName(); private Set ways; - private QuadBuckets endnodes; // nodes at end of way - private QuadBuckets endnodesHighway; // nodes at end of way - private QuadBuckets middlenodes; // nodes in middle of way + private Set endnodes; // nodes at end of way + private Set endnodesHighway; // nodes at end of way + private Set middlenodes; // nodes in middle of way private Set othernodes; // nodes appearing at least twice + private QuadBuckets searchNodes; + private Set waysToTest; + private Set nodesToTest; private Area dsArea; private double mindist; private double minmiddledist; + private DataSet ds; /** * Constructs a new {@code UnconnectedWays} test. @@ -176,42 +179,39 @@ 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 getWayEndNodesNearOtherHighway() { Map 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,13 +244,11 @@ 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,32 +255,16 @@ return map; } - protected Map getConnectedWayEndNodesNearOtherWay() { - Map 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 errorMap, String message) { for (Map.Entry 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,12 +271,36 @@ @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 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,17 +307,17 @@ 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 nearbyNodeCache; private double nearbyNodeCacheDist = -1.0; private final Node n1; @@ -320,13 +326,9 @@ 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,15 +345,12 @@ 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 getBounds(double fudge) { + public BBox getBounds(double fudge) { double x1 = n1.getCoor().lon(); double x2 = n2.getCoor().lon(); if (x1 > x2) { @@ -368,10 +367,7 @@ } LatLon topLeft = new LatLon(y2+fudge, x1-fudge); LatLon botRight = new LatLon(y1-fudge, x2+fudge); - List ret = new ArrayList<>(2); - ret.add(topLeft); - ret.add(botRight); - return ret; + return new BBox(topLeft, botRight); } public Collection nearbyNodes(double dist) { @@ -407,12 +403,10 @@ // This needs to be a hash set because the searches // overlap a bit and can return duplicate nodes. nearbyNodeCache = null; - List bounds = this.getBounds(dist * (360.0d / (Ellipsoid.WGS84.a * 2 * Math.PI))); - List 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 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,25 +454,19 @@ @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 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 s) { + @Override + public void visit(Node n) { + if (partialSelection) { + nodesToTest.add(n); + } + } + + private void addNode(Node n, Set s) { boolean m = middlenodes.contains(n); boolean e = endnodes.contains(n); boolean eh = endnodesHighway.contains(n); Index: test/unit/org/openstreetmap/josm/data/validation/tests/UnconnectedWaysTest.java =================================================================== --- test/unit/org/openstreetmap/josm/data/validation/tests/UnconnectedWaysTest.java (revision 15328) +++ test/unit/org/openstreetmap/josm/data/validation/tests/UnconnectedWaysTest.java (working copy) @@ -14,9 +14,12 @@ 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,7 +27,7 @@ public class UnconnectedWaysTest { private UnconnectedWays bib; - + public JOSMTestRules test = new JOSMTestRules().main(); /** * Setup test. * @throws Exception if the test cannot be initialized @@ -34,7 +37,6 @@ bib = new UnconnectedWays.UnconnectedHighways(); JOSMFixture.createUnitTestFixture().init(); bib.initialize(); - bib.startTest(null); } /** @@ -47,6 +49,9 @@ 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());