Ticket #16803: 16803-v2.patch

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

@@ -8 +8 @@
 import java.util.Arrays;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
-import java.util.stream.Collectors;
 
 import org.openstreetmap.josm.command.ChangePropertyCommand;
 import org.openstreetmap.josm.data.osm.Node;
@@ -22 +22 @@
 import org.openstreetmap.josm.data.validation.Severity;
 import org.openstreetmap.josm.data.validation.Test;
 import org.openstreetmap.josm.data.validation.TestError;
+import org.openstreetmap.josm.tools.Geometry;
 import org.openstreetmap.josm.tools.Logging;
 import org.openstreetmap.josm.tools.Utils;
 
@@ -41 +42 @@
 
     protected static final String SOURCE_MAXSPEED = "source:maxspeed";
 
+    // CHECKSTYLE.OFF: SingleSpaceSeparator
+    private static final Set<String> LINK_TO_HIGHWAYS = new HashSet<>(Arrays.asList(
+            "motorway",  "motorway_link",
+            "trunk",     "trunk_link",
+            "primary",   "primary_link",
+            "secondary", "secondary_link",
+            "tertiary",  "tertiary_link"
+            ));
+
     /**
      * Classified highways in order of importance
      */
-    // CHECKSTYLE.OFF: SingleSpaceSeparator
     private static final List<String> CLASSIFIED_HIGHWAYS = Arrays.asList(
             "motorway",  "motorway_link",
             "trunk",     "trunk_link",
@@ -56 +65 @@
             "living_street");
     // CHECKSTYLE.ON: SingleSpaceSeparator
 
+
     private static final Set<String> KNOWN_SOURCE_MAXSPEED_CONTEXTS = new HashSet<>(Arrays.asList(
             "urban", "rural", "zone", "zone20", "zone:20", "zone30", "zone:30", "zone40",
             "nsl_single", "nsl_dual", "motorway", "trunk", "living_street", "bicycle_road"));
@@ -98 +108 @@
         if (w.isUsable()) {
             if (w.isClosed() && w.hasTag(HIGHWAY, CLASSIFIED_HIGHWAYS) && w.hasTag("junction", "roundabout")
                     && IN_DOWNLOADED_AREA_STRICT.test(w)) {
-                // TODO: find out how to handle splitted roundabouts (see #12841)
+                // TODO: find out how to handle split roundabouts (see #12841)
                 testWrongRoundabout(w);
             }
             if (w.hasKey(SOURCE_MAXSPEED)) {
@@ -164 +174 @@
      */
     public static boolean isHighwayLinkOkay(final Way way) {
         final String highway = way.get(HIGHWAY);
-        if (highway == null || !highway.endsWith("_link")
-                || !IN_DOWNLOADED_AREA.test(way.getNode(0)) || !IN_DOWNLOADED_AREA.test(way.getNode(way.getNodesCount()-1))) {
+        if (highway == null || !highway.endsWith("_link")) {
             return true;
         }
 
-        final Set<OsmPrimitive> referrers = new HashSet<>();
+        // check if connected to a high class road where the link must match the higher class
+        String highClass = null;
+        for (int i = 0; i < way.getNodesCount(); i++) {
+            Node n = way.getNode(i);
+            if (!IN_DOWNLOADED_AREA.test(n))
+                return true;
+            Set<Way> otherWays = new HashSet<>();
+            otherWays.addAll(Utils.filteredCollection(n.getReferrers(), Way.class));
+            if (otherWays.size() == 1)
+                continue;
+            Iterator<Way> iter = otherWays.iterator();
+            while (iter.hasNext()) {
+                Way w = iter.next();
+                final String hw2 = w.get(HIGHWAY);
+                if (way == w || w.getNodesCount() < 2 || !w.isUsable() || hw2 == null)
+                    iter.remove();
+                else {
+                    if ("motorway".equals(hw2)) {
+                        highClass = "motorway";
+                        break;
+                    } else if ("trunk".equals(hw2))
+                        highClass = "trunk";
+                }
+            }
+        }
 
-        if (way.isClosed()) {
-            // for closed way we need to check all adjacent ways
-            for (Node n: way.getNodes()) {
-                referrers.addAll(n.getReferrers());
+        if (highClass != null && !highway.equals(highClass + "_link")) {
+            return false;
+        }
+
+        for (int i = 0; i < way.getNodesCount(); i++) {
+            Node n = way.getNode(i);
+            Set<Way> otherWays = new HashSet<>();
+            otherWays.addAll(Utils.filteredCollection(n.getReferrers(), Way.class));
+            if (otherWays.size() == 1)
+                continue;
+            otherWays.removeIf(w -> w == way || !highway.startsWith(w.get(HIGHWAY)) || !LINK_TO_HIGHWAYS.contains(w.get(HIGHWAY)));
+            if (otherWays.isEmpty())
+                continue;
+
+            //TODO: ignore ways which are not allowed because of turn restrictions, oneway attributes or access rules?
+            HashSet<Way> sameTag = new HashSet<>();
+            for (Way ow : otherWays) {
+                if (highway.equals(ow.get(HIGHWAY)))
+                    sameTag.add(ow);
+                else
+                    return true;
             }
+            // we have way(s) with the same _link tag, ignore those with a sharp angle
+            final int pos = i;
+            sameTag.removeIf(w -> isSharpAngle(way, pos, w));
+            if (!sameTag.isEmpty())
+                return true;
+        }
+        return false;
+
+    }
+
+    /**
+     * Check if the two given connected ways form a sharp angle.
+     * @param way 1st way
+     * @param nodePos node position of connecting node in 1st way
+     * @param otherWay the 2nd way
+     * @return true if angle is sharp or way cannot be travelled because of oneway attributes
+     */
+    private static boolean isSharpAngle(Way way, int nodePos, Way otherWay) {
+        Node n = way.getNode(nodePos);
+        int oneway = way.isOneway();
+        if (oneway == 0) {
+            if ("roundabout".equals(way.get("junction"))) {
+                oneway = 1;
+            }
+        }
+
+        if (oneway != 1) {
+            Node prev = getPrevNode(way, nodePos);
+            if (prev != null && !onlySharpAngle(n, prev, otherWay))
+                return false;
+        }
+        if (oneway != -1) {
+            Node next = getNextNode(way, nodePos);
+            if (next != null && !onlySharpAngle(n, next, otherWay))
+                return false;
+        }
+        return true;
+    }
+
+    private static Node getNextNode(Way way, int nodePos) {
+        if (nodePos + 1 > way.getNodesCount()) {
+            if (way.isClosed())
+                return way.getNode(1);
+            return null;
         } else {
-            referrers.addAll(way.firstNode().getReferrers());
-            referrers.addAll(way.lastNode().getReferrers());
+            return way.getNode(nodePos + 1);
         }
+    }
 
-        // Find ways of same class (exact class of class_link)
-        List<Way> sameClass = Utils.filteredCollection(referrers, Way.class).stream().filter(
-                otherWay -> !way.equals(otherWay) && otherWay.hasTag(HIGHWAY, highway, highway.replaceAll("_link$", "")))
-                .collect(Collectors.toList());
-        if (sameClass.size() > 1) {
-            // It is possible to have a class_link between 2 segments of same class
-            // in roundabout designs that physically separate a specific turn from the main roundabout
-            // But if we have more than a single adjacent class, and one of them is a roundabout, that's an error
-            for (Way w : sameClass) {
-                if (w.hasTag("junction", "roundabout")) {
-                    return false;
+    private static Node getPrevNode(Way way, int nodePos) {
+        if (nodePos == 0) {
+            if (way.isClosed())
+                return way.getNode(way.getNodesCount()-2);
+            return null;
+        } else {
+            return way.getNode(nodePos - 1);
+        }
+    }
+
+    private static boolean onlySharpAngle(Node common, Node from, Way toWay) {
+        int oneway = toWay.isOneway();
+        if (oneway == 0) {
+            if ("roundabout".equals(toWay.get("junction"))) {
+                oneway = 1;
+            }
+        }
+
+        for (int i = 0; i < toWay.getNodesCount(); i++) {
+            if (common == toWay.getNode(i)) {
+
+                if (oneway != 1) {
+                    Node to = getNextNode(toWay, i);
+                    if (to != null && !isSharpAngle(from, common, to))
+                        return false;
                 }
+                if (oneway != -1) {
+                    Node to = getPrevNode(toWay, i);
+                    if (to != null && !isSharpAngle(from, common, to))
+                        return false;
+                }
+                break;
             }
         }
-        // Link roads should always at least one adjacent segment of same class
-        return !sameClass.isEmpty();
+        return true;
     }
 
+    /**
+     * Returns true if angle of a corner defined with 3 point coordinates is < 60.
+     *
+     * @param n1 first node
+     * @param n2 Common node
+     * @param n3 third node
+     * @return true if angle is below 60.
+     */
+
+    private static boolean isSharpAngle(Node n1, Node n2, Node n3) {
+        double angle = Geometry.getNormalizedAngleInDegrees(
+                Geometry.getCornerAngle(n1.getEastNorth(), n2.getEastNorth(), n3.getEastNorth()));
+        return angle < 60;
+    }
+
     private void testHighwayLink(final Way way) {
         if (!isHighwayLinkOkay(way)) {
             errors.add(TestError.builder(this, Severity.WARNING, SOURCE_WRONG_LINK)

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

@@ -116 +116 @@
             if (!test.getErrors().isEmpty()) {
                 fail(test.getErrors().get(0).getMessage());
             }
-            Way w1 = ways.stream().filter(w -> 28508494 == w.getId()).findFirst().get();
-            Way w2 = ways.stream().filter(w -> 28508493 == w.getId()).findFirst().get();
-            test.visit(w1);
-            test.visit(w2);
-            assertEquals(2, test.getErrors().size());
         }
     }
 }