Changeset 17400 in josm for trunk

Timestamp:

2020-12-10T17:38:18+01:00 (4 years ago)

Author:

GerdP

Message:

fix #20121: Confusing handling of water objects in CrossingWays

• report all self crossing ways with the same message
• ignore corssing water areas because they are reported by mapcss geometry tests
• don't ignore crossing between water areas and highway, railway, barrier, or building
• remove redundant and missleading checks in CrossingWays.Ways.ignoreWaySegmentCombination()
• add unit test for coverage and test of these changes
• fix @since

Location:

trunk

Files:

• src/org/openstreetmap/josm/data/validation/tests/CrossingWays.java (modified) (7 diffs)
• test/data/crossingWays.osm (added)
• test/unit/org/openstreetmap/josm/data/validation/tests/CrossingWaysTest.java (modified) (6 diffs)

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

@@ -114 +114 @@
         boolean ignoreWaySegmentCombination(Way w1, Way w2) {
             if (w1 == w2)
-                return false;
-            if (areLayerOrLevelDifferent(w1, w2)) {
                 return true;
-            }
+            if (areLayerOrLevelDifferent(w1, w2))
+                return true;
             if (isBuilding(w1) && isBuilding(w2))
-                return true;
-            if (w1.hasKey(HIGHWAY) && w2.hasKey(HIGHWAY) && !Objects.equals(w1.get("level"), w2.get("level"))) {
-                return true;
-            }
+                return true; // handled by mapcss tests
             if (((isResidentialArea(w1) || w1.hasKey(BARRIER, HIGHWAY, RAILWAY, WATERWAY)) && isResidentialArea(w2))
              || ((isResidentialArea(w2) || w2.hasKey(BARRIER, HIGHWAY, RAILWAY, WATERWAY)) && isResidentialArea(w1)))
                 return true;
-            if (isSubwayOrTramOrRazed(w2)) {
-                return true;
-            }
+            if (isWaterArea(w1) && isWaterArea(w2))
+                return true; // handled by mapcss tests
             if (w1.hasKey(RAILWAY) && w2.hasKey(RAILWAY) && w1.hasTag(RAILWAY, "yard") != w2.hasTag(RAILWAY, "yard")) {
                 return true; // see #20089
             }
-            if (isCoastline(w1) != isCoastline(w2)) {
-                return true;
-            }
-            if ((w1.hasTag(WATERWAY, "river", "stream", "canal", "drain", "ditch") && w2.hasTag(WATERWAY, "riverbank"))
-             || (w2.hasTag(WATERWAY, "river", "stream", "canal", "drain", "ditch") && w1.hasTag(WATERWAY, "riverbank"))) {
-                return true;
-            }
-            return isProposedOrAbandoned(w2);
+            return (w1.hasTag(WATERWAY, "river", "stream", "canal", "drain", "ditch") && w2.hasTag(WATERWAY, "riverbank"))
+                    || (w2.hasTag(WATERWAY, "river", "stream", "canal", "drain", "ditch") && w1.hasTag(WATERWAY, "riverbank"));
         }
 
@@ -274 +263 @@
 
     /**
-     * Self crossing ways test (for all the rest)
+     * Self crossing ways test (for all ways)
      */
     public static class SelfCrossing extends CrossingWays {
 
         protected static final int CROSSING_SELF = 604;
-
-        CrossingWays.Ways normalTest = new Ways();
-        CrossingWays.Boundaries boundariesTest = new Boundaries();
 
         /**
@@ -291 +277 @@
 
         @Override
-        public boolean isPrimitiveUsable(OsmPrimitive p) {
-            return super.isPrimitiveUsable(p) && !(normalTest.isPrimitiveUsable(p)
-                    || boundariesTest.isPrimitiveUsable(p));
-        }
-
-        @Override
         boolean ignoreWaySegmentCombination(Way w1, Way w2) {
-            return w1 != w2; // should not happen
+            return false; // we should not get here
         }
     }
@@ -332 +312 @@
     }
 
+    static boolean isWaterArea(OsmPrimitive w) {
+        return w.hasTag("natural", "water") || w.hasTag(LANDUSE, "reservoir");
+    }
+
     static boolean isHighway(OsmPrimitive w) {
         return w.hasTagDifferent(HIGHWAY, "rest_area", "services", "bus_stop", "platform");
@@ -358 +342 @@
     @Override
     public void visit(Way w) {
-        if (this instanceof SelfCrossing) {
+        boolean findSelfCrossingOnly = this instanceof SelfCrossing;
+        if (findSelfCrossingOnly) {
             // free memory, we are not interested in previous ways
             cellSegments.clear();
@@ -378 +363 @@
                     List<WaySegment> highlight;
 
-                    if (!es1.intersects(es2) || ignoreWaySegmentCombination(es1.way, es2.way)) {
+                    if (!es1.intersects(es2)
+                            || (!findSelfCrossingOnly && ignoreWaySegmentCombination(es1.way, es2.way))) {
                         continue;
                     }
@@ -475 +461 @@
      * @return {@code true} if one or more segments of the way are crossing
      * @see SelfIntersectingWay
-     * @since xxx
+     * @since 17393
      */
     public static boolean isSelfCrossing(Way way) {

trunk/test/unit/org/openstreetmap/josm/data/validation/tests/CrossingWaysTest.java

@@ -6 +6 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import java.nio.file.Files;
+import java.nio.file.Paths;
 import java.util.HashMap;
 import java.util.List;
 
+import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
-import org.junit.jupiter.api.Test;
 import org.openstreetmap.josm.TestUtils;
 import org.openstreetmap.josm.data.coor.EastNorth;
 import org.openstreetmap.josm.data.coor.LatLon;
+import org.openstreetmap.josm.data.osm.DataSet;
 import org.openstreetmap.josm.data.osm.Node;
 import org.openstreetmap.josm.data.osm.Way;
 import org.openstreetmap.josm.data.osm.WaySegment;
+import org.openstreetmap.josm.data.validation.TestError;
 import org.openstreetmap.josm.data.validation.tests.CrossingWays.Boundaries;
 import org.openstreetmap.josm.data.validation.tests.CrossingWays.SelfCrossing;
 import org.openstreetmap.josm.data.validation.tests.CrossingWays.Ways;
+import org.openstreetmap.josm.io.OsmReader;
 import org.openstreetmap.josm.testutils.JOSMTestRules;
 
@@ -34 +39 @@
     @RegisterExtension
     @SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD")
-    public JOSMTestRules rule = new JOSMTestRules().preferences();
+    public JOSMTestRules rule = new JOSMTestRules().preferences().projection();
 
     private static Way newUsableWay(String tags) {
@@ -127 +132 @@
         testMessage(601, test, "amenity=restaurant", "amenity=restaurant");
         testMessage(611, test, "building=yes", "amenity=restaurant");
+        testMessage(611, test, "building=yes", "natural=water");
         testMessage(612, test, "building=yes", "highway=road");
         testMessage(613, test, "building=yes", "railway=rail");
@@ -133 +139 @@
         testMessage(620, test, "highway=road", "highway=road");
         testMessage(621, test, "highway=road", "amenity=restaurant");
+        testMessage(621, test, "highway=road", "natural=water");
         testMessage(622, test, "highway=road", "railway=rail");
         testMessage(623, test, "highway=road", "waterway=river");
         testMessage(630, test, "railway=rail", "railway=rail");
         testMessage(631, test, "railway=rail", "amenity=restaurant");
+        testMessage(631, test, "railway=rail", "natural=water");
         testMessage(632, test, "railway=rail", "waterway=river");
         testMessage(641, test, "landuse=residential", "amenity=restaurant");
@@ -146 +154 @@
         testMessage(663, test, "barrier=hedge", "railway=rail");
         testMessage(664, test, "barrier=hedge", "waterway=river");
+        testMessage(665, test, "barrier=hedge", "natural=water");
 
         assertFalse(test.isPrimitiveUsable(newUsableWay("amenity=restaurant")));
@@ -174 +183 @@
         SelfCrossing test = new CrossingWays.SelfCrossing();
         // isPrimitiveUsable
-        assertFalse(test.isPrimitiveUsable(newUsableWay("highway=motorway")));
-        assertFalse(test.isPrimitiveUsable(newUsableWay("barrier=yes")));
-        assertFalse(test.isPrimitiveUsable(newUsableWay("boundary=administrative")));
         assertFalse(test.isPrimitiveUsable(TestUtils.newWay("amenity=restaurant"))); // Unusable (0 node)
-        assertTrue(test.isPrimitiveUsable(newUsableWay("amenity=restaurant"))); // Usable (2 nodes)
-    }
+    }
+
+    /**
+     * Various cases of crossing linear ways and areas, mainly for coverage
+     * @throws Exception if an error occurs
+     */
+    @Test
+    void testCoverage() throws Exception {
+        DataSet ds = OsmReader.parseDataSet(
+                Files.newInputStream(Paths.get(TestUtils.getTestDataRoot(), "crossingWays.osm")), null);
+        CrossingWays crossingWays = new CrossingWays.Ways();
+        crossingWays.startTest(null);
+        crossingWays.visit(ds.allPrimitives());
+        crossingWays.endTest();
+
+        for (TestError e : crossingWays.getErrors()) {
+            // we don't report self crossing ways in this test
+            assertEquals(2, e.getPrimitives().size(), e.getPrimitives().toString());
+            // see #20121: crossing water areas should not be reported
+            assertFalse(e.getPrimitives().stream().filter(Way.class::isInstance).allMatch(CrossingWays::isWaterArea));
+        }
+
+        CrossingWays selfCrossing = new CrossingWays.SelfCrossing();
+        selfCrossing.startTest(null);
+        selfCrossing.visit(ds.allPrimitives());
+        selfCrossing.endTest();
+        assertEquals(1, selfCrossing.getErrors().size());
+
+        CrossingWays crossingBoundaries = new CrossingWays.Boundaries();
+        crossingBoundaries.startTest(null);
+        crossingBoundaries.visit(ds.allPrimitives());
+        crossingBoundaries.endTest();
+        assertEquals(2, crossingBoundaries.getErrors().size());
+    }
+
 }