Index: trunk/src/org/openstreetmap/josm/data/osm/Node.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/osm/Node.java (revision 15333) +++ trunk/src/org/openstreetmap/josm/data/osm/Node.java (revision 15335) @@ -373,5 +373,4 @@ for (final Node n : w.getNodes()) { final boolean containsN = visited.contains(n); - visited.add(n); if (!containsN && (predicate == null || predicate.test(n)) && n.isConnectedTo(otherNodes, hops - 1, predicate, visited)) { Index: trunk/src/org/openstreetmap/josm/data/validation/tests/UnconnectedWays.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/validation/tests/UnconnectedWays.java (revision 15333) +++ trunk/src/org/openstreetmap/josm/data/validation/tests/UnconnectedWays.java (revision 15335) @@ -8,5 +8,4 @@ import java.awt.geom.Area; import java.awt.geom.Line2D; -import java.awt.geom.Point2D; import java.util.ArrayList; import java.util.Collection; @@ -47,4 +46,12 @@ 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,10 +65,10 @@ */ 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,10 +83,10 @@ */ 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,10 +101,10 @@ */ 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,10 +119,10 @@ */ 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,10 +137,10 @@ */ 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,13 +149,16 @@ 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 List waySegments; + private Set endnodes; // 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; /** @@ -158,5 +168,5 @@ */ public UnconnectedWays(String title) { - this(title, UNCONNECTED_WAYS); + this(title, UNCONNECTED_WAYS, false); } @@ -166,9 +176,11 @@ * @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,50 +188,19 @@ 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 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); - } - } - } - } - return map; - } - - protected Map getWayEndNodesNearOtherWay() { - Map map = new HashMap<>(); - for (MyWaySegment s : ways) { + for (MyWaySegment s : waySegments) { if (isCanceled()) { map.clear(); @@ -227,8 +208,13 @@ } 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,19 +224,17 @@ } - protected Map getWayNodesNearOtherWay() { + protected Map getWayEndNodesNearOtherWay() { Map 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,7 +242,7 @@ } - protected Map getConnectedWayEndNodesNearOtherWay() { + protected Map getWayNodesNearOtherWay() { Map map = new HashMap<>(); - for (MyWaySegment s : ways) { + for (MyWaySegment s : waySegments) { if (isCanceled()) { map.clear(); @@ -266,11 +250,7 @@ } 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,8 +260,12 @@ 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()); } @@ -290,44 +274,56 @@ @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 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,13 +340,10 @@ 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 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,47 +362,17 @@ 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) { - // 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 trimmed = new HashSet<>(nearbyNodeCache); - Set 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 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 result = null; + List foundNodes = searchNodes.search(bounds); for (Node n : foundNodes) { if (!nearby(n, dist) || !n.getCoor().isIn(dsArea)) { @@ -419,14 +382,10 @@ // 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,12 +393,9 @@ List getWaySegments(Way w) { List 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,8 +405,5 @@ 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,28 +414,21 @@ @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); - } - } - - private void addNode(Node n, QuadBuckets s) { + if (partialSelection) { + waysToTest.add(w); + } + } + + @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); boolean o = othernodes.contains(n); - if (!m && !e && !o && !eh) { + if (!m && !e && !o) { s.add(n); } else if (!o) { @@ -490,6 +436,4 @@ if (e) { endnodes.remove(n); - } else if (eh) { - endnodesHighway.remove(n); } else { middlenodes.remove(n);