[Improvement] MalformedRoundaboutCheck: Minimum OSM Nodes/Angles (osm…

…lab#476)

* Flag roundabouts with fewer than 9 OSM nodes

* Check for angles greater or equal to threshold

* missed import

* Catch exception where polyline cannot be built

* tests

* integration

* spotless

* some more spotless

* Use CommonMethods.buildOriginalOsmWayGeometry

* Some tests noticed they have bad angles

* integration

* Check if roundabout is made of mulitple ways

* forgot to rebuild image before running check

* fixed tests

* spotlessJava fixes that didn't show up on local check

Co-authored-by: Brian Jorgenson (Insight Global Inc) <[email protected]>

config/configuration.json

@@ -631,6 +631,8 @@
     }
   },
   "MalformedRoundaboutCheck" : {
+    "angle.threshold.maximum_degree": 60.0,
+    "min.nodes": 9.0,
     "traffic.countries.left":["AIA", "ATG", "AUS", "BGD", "BHS", "BMU", "BRB", "BRN", "BTN", "BWA",
       "CCK", "COK", "CXR", "CYM", "CYP", "DMA", "FJI", "FLK", "GBR", "GGY", "GRD",
       "GUY", "HKG", "IDN", "IMN", "IND", "IRL", "JAM", "JEY", "JPN", "KEN", "KIR",

src/main/java/org/openstreetmap/atlas/checks/validation/linear/edges/MalformedRoundaboutCheck.java

@@ -12,6 +12,8 @@
 import org.openstreetmap.atlas.checks.base.BaseCheck;
 import org.openstreetmap.atlas.checks.flag.CheckFlag;
 import org.openstreetmap.atlas.checks.utility.CommonMethods;
+import org.openstreetmap.atlas.exception.CoreException;
+import org.openstreetmap.atlas.geography.Location;
 import org.openstreetmap.atlas.geography.PolyLine;
 import org.openstreetmap.atlas.geography.Polygon;
 import org.openstreetmap.atlas.geography.atlas.change.FeatureChange;
@@ -35,6 +37,8 @@
 import org.openstreetmap.atlas.tags.TunnelTag;
 import org.openstreetmap.atlas.tags.annotations.validation.Validators;
 import org.openstreetmap.atlas.utilities.configuration.Configuration;
+import org.openstreetmap.atlas.utilities.scalars.Angle;
+import org.openstreetmap.atlas.utilities.tuples.Tuple;
 
 /**
  * This check flags roundabouts where the directionality is opposite to what it should be, the
@@ -51,6 +55,8 @@ public class MalformedRoundaboutCheck extends BaseCheck<Long>
     private static final String BASIC_INSTRUCTION = "This roundabout is malformed.";
     private static final String ENCLOSED_ROADS_INSTRUCTIONS = "This roundabout has car navigable ways inside it.";
     private static final String WRONG_WAY_INSTRUCTIONS = "This roundabout is going the wrong direction, or has been improperly tagged as a roundabout.";
+    private static final String MINIMUM_NODES_INSTRUCTION = "This roundabout has less than {0,number,#} nodes.";
+    private static final String SHARP_ANGLE_INSTRUCTION = "This roundabout has too sharp of an angle at {0}, consider adding more nodes or rearranging the roundabout to fix this.";
     private static final List<String> LEFT_DRIVING_COUNTRIES_DEFAULT = Arrays.asList("AIA", "ATG",
             "AUS", "BGD", "BHS", "BMU", "BRB", "BRN", "BTN", "BWA", "CCK", "COK", "CXR", "CYM",
             "CYP", "DMA", "FJI", "FLK", "GBR", "GGY", "GRD", "GUY", "HKG", "IDN", "IMN", "IND",
@@ -59,15 +65,25 @@ public class MalformedRoundaboutCheck extends BaseCheck<Long>
             "PAK", "PCN", "PNG", "SGP", "SGS", "SHN", "SLB", "SUR", "SWZ", "SYC", "TCA", "THA",
             "TKL", "TLS", "TON", "TTO", "TUV", "TZA", "UGA", "VCT", "VGB", "VIR", "WSM", "ZAF",
             "ZMB", "ZWE");
-    private static final List<String> FALLBACK_INSTRUCTIONS = Arrays
-            .asList(ENCLOSED_ROADS_INSTRUCTIONS, WRONG_WAY_INSTRUCTIONS, BASIC_INSTRUCTION);
+    private static final double MIN_NODES_DEFAULT = 9.0;
+    private static final double MAX_THRESHOLD_DEGREES_DEFAULT = 60.0;
+    private static final int MIN_NODES_INSTRUCTION_INDEX = 3;
+    private static final int SHARP_ANGLE_INSTRUCTION_INDEX = 4;
+    private static final List<String> FALLBACK_INSTRUCTIONS = Arrays.asList(
+            ENCLOSED_ROADS_INSTRUCTIONS, WRONG_WAY_INSTRUCTIONS, BASIC_INSTRUCTION,
+            MINIMUM_NODES_INSTRUCTION, SHARP_ANGLE_INSTRUCTION);
     private final List<String> leftDrivingCountries;
+    private final double minNodes;
+    private final Angle maxAngleThreshold;
 
     public MalformedRoundaboutCheck(final Configuration configuration)
     {
         super(configuration);
         this.leftDrivingCountries = configurationValue(configuration, "traffic.countries.left",
                 LEFT_DRIVING_COUNTRIES_DEFAULT);
+        this.minNodes = configurationValue(configuration, "min.nodes", MIN_NODES_DEFAULT);
+        this.maxAngleThreshold = this.configurationValue(configuration,
+                "angle.threshold.maximum_degree", MAX_THRESHOLD_DEGREES_DEFAULT, Angle::degrees);
     }
 
     @Override
@@ -159,6 +175,39 @@ protected Optional<CheckFlag> flag(final AtlasObject object)
             instructions.add(this.getLocalizedInstruction(0));
         }
 
+        try
+        {
+            final PolyLine originalGeometry = CommonMethods
+                    .buildOriginalOsmWayGeometry((Edge) object);
+            // There should be a minimum amount of OSM nodes in a roundabout to have good
+            // visuals.
+            // Only count nodes when we have the full roundabout, some are split into multiple
+            // ways.
+            if (originalGeometry.size() < this.minNodes
+                    && !this.isMultiWayRoundabout(roundaboutEdgeSet))
+            {
+                instructions.add(
+                        this.getLocalizedInstruction(MIN_NODES_INSTRUCTION_INDEX, this.minNodes));
+            }
+
+            // Check for sharp angles, roundabouts should be smooth curves
+            final List<Tuple<Angle, Location>> maxAngles = originalGeometry
+                    .anglesGreaterThanOrEqualTo(this.maxAngleThreshold);
+            if (!maxAngles.isEmpty())
+            {
+                final String angleLocations = "(" + maxAngles.stream()
+                        .map(t -> "(" + t.getSecond().getLatitude() + ", "
+                                + t.getSecond().getLongitude() + ")")
+                        .collect(Collectors.joining(", ")) + ")";
+                instructions.add(this.getLocalizedInstruction(SHARP_ANGLE_INSTRUCTION_INDEX,
+                        angleLocations));
+            }
+        }
+        catch (final CoreException ignored)
+        {
+            /* Do Nothing */
+        }
+
         if (!instructions.isEmpty())
         {
             final CheckFlag flag = this.createFlag(roundaboutEdgeSet,

src/test/java/org/openstreetmap/atlas/checks/validation/linear/edges/MalformedRoundaboutCheckTest.java

@@ -31,7 +31,9 @@ private static void verifyFixSuggestions(final CheckFlag flag, final int count)
     public void clockwiseRoundaboutLeftDrivingMissingTagTest()
     {
         this.verifier.actual(this.setup.clockwiseRoundaboutLeftDrivingMissingTagAtlas(),
-                new MalformedRoundaboutCheck(ConfigurationResolver.emptyConfiguration()));
+                new MalformedRoundaboutCheck(ConfigurationResolver.inlineConfiguration(
+                        "{\"MalformedRoundaboutCheck\":{\"angle.threshold.maximum_degree\":" + 179.0
+                                + ",\"min.nodes\":" + 0.0 + "}}")));
         this.verifier.globallyVerify(flags -> Assert.assertEquals(
                 "1. This roundabout is malformed.\n"
                         + "2. This roundabout does not form a single, one-way, complete, car navigable route.",
@@ -43,7 +45,9 @@ public void counterClockwiseConnectedDoubleRoundaboutRightDrivingTest()
     {
         this.verifier.actual(
                 this.setup.counterClockwiseConnectedDoubleRoundaboutRightDrivingAtlas(),
-                new MalformedRoundaboutCheck(ConfigurationResolver.emptyConfiguration()));
+                new MalformedRoundaboutCheck(ConfigurationResolver.inlineConfiguration(
+                        "{\"MalformedRoundaboutCheck\":{\"angle.threshold.maximum_degree\":" + 179.0
+                                + ",\"min.nodes\":" + 0.0 + "}}")));
         this.verifier.globallyVerify(flags -> Assert.assertEquals(
                 "1. This roundabout is malformed.\n"
                         + "2. This roundabout does not form a single, one-way, complete, car navigable route.",
@@ -54,7 +58,9 @@ public void counterClockwiseConnectedDoubleRoundaboutRightDrivingTest()
     public void counterClockwiseRoundaboutBridgeRightDrivingEnclosedTest()
     {
         this.verifier.actual(this.setup.counterClockwiseRoundaboutBridgeRightDrivingEnclosedAtlas(),
-                new MalformedRoundaboutCheck(ConfigurationResolver.emptyConfiguration()));
+                new MalformedRoundaboutCheck(ConfigurationResolver.inlineConfiguration(
+                        "{\"MalformedRoundaboutCheck\":{\"angle.threshold.maximum_degree\":" + 179.0
+                                + ",\"min.nodes\":" + 0.0 + "}}")));
         this.verifier.globallyVerify(flags -> Assert.assertEquals(0, flags.size()));
     }
 
@@ -70,15 +76,19 @@ public void counterClockwiseRoundaboutRightDrivingCyclewayTest()
     public void counterClockwiseRoundaboutRightDrivingEnclosedBridgeTest()
     {
         this.verifier.actual(this.setup.counterClockwiseRoundaboutRightDrivingEnclosedBridgeAtlas(),
-                new MalformedRoundaboutCheck(ConfigurationResolver.emptyConfiguration()));
+                new MalformedRoundaboutCheck(ConfigurationResolver.inlineConfiguration(
+                        "{\"MalformedRoundaboutCheck\":{\"angle.threshold.maximum_degree\":" + 179.0
+                                + ",\"min.nodes\":" + 0.0 + "}}")));
         this.verifier.globallyVerify(flags -> Assert.assertEquals(0, flags.size()));
     }
 
     @Test
     public void counterClockwiseRoundaboutRightDrivingEnclosedPathTest()
     {
         this.verifier.actual(this.setup.counterClockwiseRoundaboutRightDrivingEnclosedPathAtlas(),
-                new MalformedRoundaboutCheck(ConfigurationResolver.emptyConfiguration()));
+                new MalformedRoundaboutCheck(ConfigurationResolver.inlineConfiguration(
+                        "{\"MalformedRoundaboutCheck\":{\"angle.threshold.maximum_degree\":" + 179.0
+                                + ",\"min.nodes\":" + 0.0 + "}}")));
         this.verifier.globallyVerify(flags -> Assert.assertEquals(0, flags.size()));
     }
 
@@ -87,7 +97,9 @@ public void counterClockwiseRoundaboutRightDrivingNonCarNavigableAtlas()
     {
         this.verifier.actual(
                 this.setup.counterClockwiseRoundaboutRightDrivingNonCarNavigableAtlas(),
-                new MalformedRoundaboutCheck(ConfigurationResolver.emptyConfiguration()));
+                new MalformedRoundaboutCheck(ConfigurationResolver.inlineConfiguration(
+                        "{\"MalformedRoundaboutCheck\":{\"angle.threshold.maximum_degree\":" + 179.0
+                                + ",\"min.nodes\":" + 0.0 + "}}")));
         this.verifier.globallyVerify(flags -> Assert.assertEquals(
                 "1. This roundabout is malformed.\n"
                         + "2. This roundabout does not form a single, one-way, complete, car navigable route.",
@@ -98,7 +110,9 @@ public void counterClockwiseRoundaboutRightDrivingNonCarNavigableAtlas()
     public void counterClockwiseRoundaboutRightDrivingOneWayNoTest()
     {
         this.verifier.actual(this.setup.counterClockwiseRoundaboutRightDrivingOneWayNoAtlas(),
-                new MalformedRoundaboutCheck(ConfigurationResolver.emptyConfiguration()));
+                new MalformedRoundaboutCheck(ConfigurationResolver.inlineConfiguration(
+                        "{\"MalformedRoundaboutCheck\":{\"angle.threshold.maximum_degree\":" + 179.0
+                                + ",\"min.nodes\":" + 0.0 + "}}")));
         this.verifier.globallyVerify(flags -> Assert.assertEquals(
                 "1. This roundabout is malformed.\n"
                         + "2. This roundabout does not form a single, one-way, complete, car navigable route.",
@@ -118,18 +132,34 @@ public void counterClockwiseRoundaboutRightDrivingOutsideConnectionTest()
     public void counterClockwiseRoundaboutRightDrivingWrongEdgesTagTest()
     {
         this.verifier.actual(this.setup.counterClockwiseRoundaboutRightDrivingWrongEdgesTagAtlas(),
-                new MalformedRoundaboutCheck(ConfigurationResolver.emptyConfiguration()));
+                new MalformedRoundaboutCheck(ConfigurationResolver.inlineConfiguration(
+                        "{\"MalformedRoundaboutCheck\":{\"angle.threshold.maximum_degree\":" + 179.0
+                                + ",\"min.nodes\":" + 0.0 + "}}")));
         this.verifier.globallyVerify(flags -> Assert.assertEquals(
                 "1. This roundabout is malformed.\n"
                         + "2. This roundabout has car navigable ways inside it.",
                 flags.get(0).getInstructions()));
     }
 
+    @Test
+    public void testAngleGreaterThanThreshold()
+    {
+        this.verifier.actual(this.setup.angleGreaterThanThreshold(),
+                new MalformedRoundaboutCheck(ConfigurationResolver.inlineConfiguration(
+                        "{\"MalformedRoundaboutCheck\":{\"angle.threshold.maximum_degree\":" + 60.0
+                                + ", \"min.nodes\":" + 1.0 + "}}")));
+        this.verifier.globallyVerify(flags -> Assert.assertEquals(
+                "1. This roundabout is malformed.\n2. This roundabout has too sharp of an angle at ((-27.4512972, 153.0120204), (-27.4512764, 153.0121637), (-27.4513581, 153.0121916), (-27.4512564, 153.0120857)), consider adding more nodes or rearranging the roundabout to fix this.",
+                flags.get(0).getInstructions()));
+    }
+
     @Test
     public void testClockwiseRoundaboutLeftDrivingAtlas()
     {
         this.verifier.actual(this.setup.clockwiseRoundaboutLeftDrivingAtlas(),
-                new MalformedRoundaboutCheck(ConfigurationResolver.emptyConfiguration()));
+                new MalformedRoundaboutCheck(ConfigurationResolver.inlineConfiguration(
+                        "{\"MalformedRoundaboutCheck\":{\"angle.threshold.maximum_degree\":" + 179.0
+                                + ",\"min.nodes\":" + 0.0 + "}}")));
         this.verifier.verifyEmpty();
     }
 
@@ -169,7 +199,9 @@ public void testCounterClockwiseRoundaboutLeftDrivingAtlas()
     public void testCounterClockwiseRoundaboutRightDrivingAtlas()
     {
         this.verifier.actual(this.setup.counterClockwiseRoundaboutRightDrivingAtlas(),
-                new MalformedRoundaboutCheck(ConfigurationResolver.emptyConfiguration()));
+                new MalformedRoundaboutCheck(ConfigurationResolver.inlineConfiguration(
+                        "{\"MalformedRoundaboutCheck\":{\"angle.threshold.maximum_degree\":" + 179.0
+                                + ",\"min.nodes\":" + 0.0 + "}}")));
         this.verifier.verifyEmpty();
     }
 
@@ -184,7 +216,18 @@ public void testCounterClockwiseRoundaboutRightDrivingMadeLeftAtlas()
                 "1. This roundabout is going the wrong direction, or has been improperly tagged as a roundabout.",
                 flags.get(0).getInstructions()));
         this.verifier.verify(flag -> verifyFixSuggestions(flag, 1));
+    }
 
+    @Test
+    public void testFewerThanMinOSMNodesRoundaboutAtlas()
+    {
+        this.verifier.actual(this.setup.fewerThanMinOSMNodesRoundaboutAtlas(),
+                new MalformedRoundaboutCheck(ConfigurationResolver
+                        .inlineConfiguration("{\"MalformedRoundaboutCheck\":{\"min.nodes\":" + 9.0
+                                + ", \"angle.threshold.maximum_degree\":" + 179.0 + "}}")));
+        this.verifier.globallyVerify(flags -> Assert.assertEquals(
+                "1. This roundabout is malformed.\n2. This roundabout has less than 9 nodes.",
+                flags.get(0).getInstructions()));
     }
 
     @Test
@@ -196,7 +239,6 @@ public void testMultiDirectionalRoundaboutAtlas()
                 "1. This roundabout is malformed.\n"
                         + "2. This roundabout does not form a single, one-way, complete, car navigable route.",
                 flags.get(0).getInstructions()));
-
     }
 
     @Test
@@ -211,7 +253,9 @@ public void testRoundaboutWithEnclosedMultiLayerNavigableRoad()
     public void testRoundaboutWithEnclosedNavigableRoad()
     {
         this.verifier.actual(this.setup.enclosedNavigableRoad(),
-                new MalformedRoundaboutCheck(ConfigurationResolver.emptyConfiguration()));
+                new MalformedRoundaboutCheck(ConfigurationResolver.inlineConfiguration(
+                        "{\"MalformedRoundaboutCheck\":{\"angle.threshold.maximum_degree\":" + 179.0
+                                + ",\"min.nodes\":" + 0.0 + "}}")));
         this.verifier.globallyVerify(flags -> Assert.assertEquals(
                 "1. This roundabout is malformed.\n"
                         + "2. This roundabout has car navigable ways inside it.",
@@ -222,7 +266,9 @@ public void testRoundaboutWithEnclosedNavigableRoad()
     public void testRoundaboutWithEnclosedNavigableRoadArea()
     {
         this.verifier.actual(this.setup.enclosedNavigableRoadArea(),
-                new MalformedRoundaboutCheck(ConfigurationResolver.emptyConfiguration()));
+                new MalformedRoundaboutCheck(ConfigurationResolver.inlineConfiguration(
+                        "{\"MalformedRoundaboutCheck\":{\"angle.threshold.maximum_degree\":" + 179.0
+                                + ",\"min.nodes\":" + 0.0 + "}}")));
         this.verifier.verifyEmpty();
     }