InvalidGeometryCheck (osmlab#304)

* InvalidGeometryCheck

* docs

* better refactor

config/configuration.json

@@ -355,6 +355,15 @@
       "tags":"access,highway"
     }
   },
+  "InvalidGeometryCheck": {
+    "challenge": {
+      "description": "Tasks containing Ways with invalid geometries",
+      "blurb": "Invalid Geometries",
+      "instruction": "Correct the geometries of the displayed ways",
+      "difficulty": "NORMAL",
+      "tags":"geometry"
+    }
+  },
   "InvalidLanesTagCheck": {
     "lanes.filter": "lanes->1,1.5,2,3,4,5,6,7,8,9,10",
     "challenge":{

dependencies.gradle

@@ -1,7 +1,7 @@
 project.ext.versions = [
     checkstyle: '8.18',
     jacoco: '0.8.3',
-    atlas: '6.1.6',
+    atlas: '6.1.7',
     commons:'2.6',
     atlas_generator: '5.0.3',
     atlas_checkstyle: '5.6.9',

docs/available_checks.md

@@ -89,6 +89,7 @@ This document is a list of tables with a description and link to documentation f
 | [~~DuplicateWaysCheck~~ (Deprecated)](checks/duplicateWaysCheck.md) | The purpose of this check is to identify Ways that have either had their entire length or a segment of their length duplicated or drawn multiple times. **This check has been deprecated and is no longer active.** |
 | [GeneralizedCoastlineCheck](checks/generalizedCoastlineCheck.md) | The purpose of this check is to identify coastlines whose nodes are too far apart, have angles that are too sharp, and/or have _source=PGS_ Tag values. |
 | [IntersectingBuildingsCheck](checks/intersectingBuildingsCheck.md) | The purpose of this check is to identify buildings that intersect other buildings. |
+| [InvalidGeometryCheck](checks/invalidGeometryCheck.md) | This check flags invalid polyline and polygon geometries. |
 | LineCrossingBuildingCheck | The purpose of this check is to identify line items (edges or lines) that are crossing buildings invalidly. |
 | LineCrossingWaterBodyCheck | The purpose of this check is to identify line items (edges or lines) and optionally buildings, that cross water bodies invalidly. |
 | MalformedPolyLineCheck | This check identifies lines that have only one point, or none, and the ones that are too long. |

docs/checks/invalidGeometryCheck.md

@@ -0,0 +1,21 @@
+# Invalid Geometry Check
+
+#### Description
+
+This check flags invalid polyline and polygon geometries. 
+
+#### Live Example
+
+The Way [id:803496316](https://www.openstreetmap.org/way/803496316) is an invalid self intersecting polygon. 
+
+#### Code Review
+
+This check looks at three types of Atlas Items: [Areas](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/geography/atlas/items/Area.java), [Edges](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/geography/atlas/items/Edge.java), and [Lines](https://github.com/osmlab/atlas/blob/dev/src/main/java/org/openstreetmap/atlas/geography/atlas/items/Line.java).
+A feature is considered valid for the check if it is one of those types and has not been country sliced.
+
+Geometries are validated using the [Java Topology Suite](https://github.com/locationtech/jts) (JTS).
+The Atlas geometries are converted to JTS geometries. 
+If a features fails to pass the JTS geometry `.isSimple()` or `.isValid()` methods then it is flagged.
+
+To learn more about the code, please look at the comments in the source code for the check.
+[InvalidGeometryCheck.java](../../src/main/java/org/openstreetmap/atlas/checks/validation/geometry/InvalidGeometryCheck.java)

src/main/java/org/openstreetmap/atlas/checks/validation/GeometryValidator.java

@@ -1,45 +1,76 @@
 package org.openstreetmap.atlas.checks.validation;
 
-import org.locationtech.jts.geom.LineString;
+import java.util.Optional;
+
+import org.openstreetmap.atlas.geography.Location;
 import org.openstreetmap.atlas.geography.PolyLine;
 import org.openstreetmap.atlas.geography.Polygon;
+import org.openstreetmap.atlas.geography.converters.jts.JtsPointConverter;
 import org.openstreetmap.atlas.geography.converters.jts.JtsPolyLineConverter;
 import org.openstreetmap.atlas.geography.converters.jts.JtsPolygonConverter;
 
 /**
  * Basic JTS verification for a {@link PolyLine} and {@link Polygon}
  *
  * @author mgostintsev
+ * @author bbreithaupt
  */
 public final class GeometryValidator
 {
+    private static final Optional<String> NOT_SIMPLE_LINEAR = Optional.of(
+            "Linear geometry intersecting itself at interior points (points other than the boundary)");
+    private static final Optional<String> NOT_SIMPLE_POINT = Optional
+            .of("Point geometry has repeated point");
+    private static final Optional<String> NOT_SIMPLE_POLYGON = Optional
+            .of("Polygon geometry is malformed");
+    private static final Optional<String> NOT_VALID_LINEAR = Optional
+            .of("Linear geometry has exactly two identical points");
+    private static final Optional<String> NOT_VALID_POINT = Optional
+            .of("Point geometry has invalid dimension value (NaN)");
+    private static final Optional<String> NOT_VALID_POLYGON = Optional.of(
+            "Polygon geometry has one or more of the following: Invalid coordinates, Invalid linear rings in "
+                    + "construction, Holes in the polygon that touch other holes or the outer ring more than at one"
+                    + " point, Interior that is not connected (split into two by holes)");
+    private static final JtsPointConverter POINT_CONVERTER = new JtsPointConverter();
     private static final JtsPolyLineConverter POLYLINE_CONVERTER = new JtsPolyLineConverter();
     private static final JtsPolygonConverter POLYGON_CONVERTER = new JtsPolygonConverter();
 
-    /**
-     * Tests that the {@link PolyLine}'s geometry is valid
-     *
-     * @param polyline
-     *            the {@link PolyLine} to test
-     * @return {@code true} if the {@link PolyLine} has valid geometry, otherwise {@code false}
-     */
-    public static boolean isValidPolyLine(final PolyLine polyline)
+    public static Optional<String> testSimplicity(final Iterable<Location> geometry)
     {
-        final LineString lineString = POLYLINE_CONVERTER.convert(polyline);
-        return lineString.isSimple();
+        if (geometry instanceof Location
+                && !POINT_CONVERTER.convert((Location) geometry).isSimple())
+        {
+            return NOT_SIMPLE_POINT;
+        }
+        if (geometry instanceof PolyLine
+                && !POLYLINE_CONVERTER.convert((PolyLine) geometry).isSimple())
+        {
+            return NOT_SIMPLE_LINEAR;
+        }
+        if (geometry instanceof Polygon
+                && !POLYGON_CONVERTER.convert((Polygon) geometry).isSimple())
+        {
+            return NOT_SIMPLE_POLYGON;
+        }
+        return Optional.empty();
     }
 
-    /**
-     * Tests that the {@link Polygon}'s geometry is valid
-     *
-     * @param polygon
-     *            the {@link Polygon} to test
-     * @return {@code true} if the {@link Polygon} has valid geometry, otherwise {@code false}
-     */
-    public static boolean isValidPolygon(final Polygon polygon)
+    public static Optional<String> testValidity(final Iterable<Location> geometry)
     {
-        final org.locationtech.jts.geom.Polygon jtsPolygon = POLYGON_CONVERTER.convert(polygon);
-        return jtsPolygon.isSimple();
+        if (geometry instanceof Location && !POINT_CONVERTER.convert((Location) geometry).isValid())
+        {
+            return NOT_VALID_POINT;
+        }
+        if (geometry instanceof PolyLine
+                && !POLYLINE_CONVERTER.convert((PolyLine) geometry).isValid())
+        {
+            return NOT_VALID_LINEAR;
+        }
+        if (geometry instanceof Polygon && !POLYGON_CONVERTER.convert((Polygon) geometry).isValid())
+        {
+            return NOT_VALID_POLYGON;
+        }
+        return Optional.empty();
     }
 
     private GeometryValidator()

src/main/java/org/openstreetmap/atlas/checks/validation/geometry/InvalidGeometryCheck.java

@@ -0,0 +1,77 @@
+package org.openstreetmap.atlas.checks.validation.geometry;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.Optional;
+
+import org.openstreetmap.atlas.checks.base.BaseCheck;
+import org.openstreetmap.atlas.checks.flag.CheckFlag;
+import org.openstreetmap.atlas.checks.validation.GeometryValidator;
+import org.openstreetmap.atlas.geography.atlas.items.Area;
+import org.openstreetmap.atlas.geography.atlas.items.AtlasItem;
+import org.openstreetmap.atlas.geography.atlas.items.AtlasObject;
+import org.openstreetmap.atlas.geography.atlas.items.Edge;
+import org.openstreetmap.atlas.geography.atlas.items.LineItem;
+import org.openstreetmap.atlas.tags.SyntheticBoundaryNodeTag;
+import org.openstreetmap.atlas.tags.SyntheticGeometrySlicedTag;
+import org.openstreetmap.atlas.utilities.configuration.Configuration;
+
+/**
+ * Checks Atlas items using {@link org.locationtech.jts.geom.Geometry} isValid and isSimple methods.
+ * Generates flag based on JTS provided invalidity and non simplicity causes.
+ *
+ * @author jklamer
+ * @author bbreithaupt
+ */
+public class InvalidGeometryCheck extends BaseCheck<Long>
+{
+    private static final String NOT_SIMPLE_TEMPLATE = "Geometry is Not Simple: {0}. ";
+    private static final String NOT_VALID_TEMPLATE = "Geometry is Not Valid: {0}. ";
+    private static final List<String> FALLBACK_INSTRUCTIONS = Arrays.asList(NOT_SIMPLE_TEMPLATE,
+            NOT_VALID_TEMPLATE);
+    private static final long serialVersionUID = 4212714363153085279L;
+
+    public InvalidGeometryCheck(final Configuration configuration)
+    {
+        super(configuration);
+    }
+
+    @Override
+    public boolean validCheckForObject(final AtlasObject object)
+    {
+        return (object instanceof Area || object instanceof LineItem)
+                && !SyntheticGeometrySlicedTag.isGeometrySliced(object)
+                && !(object instanceof Edge && ((Edge) object).connectedNodes().stream()
+                        .anyMatch(SyntheticBoundaryNodeTag::isBoundaryNode));
+    }
+
+    @Override
+    protected Optional<CheckFlag> flag(final AtlasObject object)
+    {
+        final List<String> instructions = new ArrayList<>();
+
+        // check if simple if not a LineItem (too many correct in OSM but not simple linear
+        // geometries)
+        final Optional<String> simpleTest = object instanceof LineItem ? Optional.empty()
+                : GeometryValidator.testSimplicity(((AtlasItem) object).getRawGeometry());
+        // check if valid
+        final Optional<String> validTest = GeometryValidator
+                .testValidity(((AtlasItem) object).getRawGeometry());
+
+        simpleTest.ifPresent(reason -> instructions.add(this.getLocalizedInstruction(0, reason)));
+        validTest.ifPresent(reason -> instructions.add(this.getLocalizedInstruction(1, reason)));
+
+        return instructions.isEmpty() ? Optional.empty()
+                : Optional.of(new CheckFlag(this.getTaskIdentifier(object),
+                        Collections.singleton(object), instructions));
+    }
+
+    @Override
+    protected List<String> getFallbackInstructions()
+    {
+        return FALLBACK_INSTRUCTIONS;
+    }
+
+}

src/main/java/org/openstreetmap/atlas/checks/validation/intersections/SelfIntersectingPolylineCheck.java

@@ -12,7 +12,6 @@
 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.Segment;
 import org.openstreetmap.atlas.geography.atlas.items.Area;
 import org.openstreetmap.atlas.geography.atlas.items.AtlasObject;
@@ -141,14 +140,7 @@ else if (object instanceof Area)
             boolean isJtsValid = true;
             try
             {
-                if (object instanceof Area)
-                {
-                    isJtsValid = GeometryValidator.isValidPolygon((Polygon) polyline);
-                }
-                else
-                {
-                    isJtsValid = GeometryValidator.isValidPolyLine(polyline);
-                }
+                isJtsValid = GeometryValidator.testSimplicity(polyline).isEmpty();
             }
             catch (final IllegalArgumentException e)
             {

src/test/java/org/openstreetmap/atlas/checks/validation/geometry/InvalidGeometryCheckTest.java

@@ -0,0 +1,96 @@
+package org.openstreetmap.atlas.checks.validation.geometry;
+
+import org.junit.Assert;
+import org.junit.Rule;
+import org.junit.Test;
+import org.openstreetmap.atlas.checks.configuration.ConfigurationResolver;
+import org.openstreetmap.atlas.checks.validation.verifier.ConsumerBasedExpectedCheckVerifier;
+import org.openstreetmap.atlas.utilities.configuration.Configuration;
+
+/**
+ * Test class for {@link InvalidGeometryCheck}
+ *
+ * @author jklamer
+ * @author bbreithaupt
+ */
+public class InvalidGeometryCheckTest
+{
+
+    @Rule
+    public InvalidGeometryCheckTestRule setup = new InvalidGeometryCheckTestRule();
+    @Rule
+    public ConsumerBasedExpectedCheckVerifier verifier = new ConsumerBasedExpectedCheckVerifier();
+    private final Configuration configuration = ConfigurationResolver.emptyConfiguration();
+
+    @Test
+    public void borderSlicedPolygonTest()
+    {
+        this.verifier.actual(this.setup.borderSlicedPolygonAtlas(),
+                new InvalidGeometryCheck(this.configuration));
+        this.verifier.verifyEmpty();
+    }
+
+    @Test
+    public void boundaryNodeTest()
+    {
+        this.verifier.actual(this.setup.boundaryNodeAtlas(),
+                new InvalidGeometryCheck(this.configuration));
+        this.verifier.verifyEmpty();
+    }
+
+    @Test
+    public void bowtiePolygonInvalidTest()
+    {
+        this.verifier.actual(this.setup.getBowtiePolygonAtlas(),
+                new InvalidGeometryCheck(this.configuration));
+        this.verifier.verifyNotEmpty();
+        this.verifier.verify(flag -> Assert.assertEquals(InvalidGeometryCheckTestRule.TEST_ID_5,
+                flag.getIdentifier()));
+    }
+
+    @Test
+    public void disconnectedCenterPolygonInvalidTest()
+    {
+        this.verifier.actual(this.setup.getDisconnectedCenterPolygonAtlas(),
+                new InvalidGeometryCheck(this.configuration));
+        this.verifier.verifyNotEmpty();
+        this.verifier.verify(flag -> Assert.assertEquals(InvalidGeometryCheckTestRule.TEST_ID_7,
+                flag.getIdentifier()));
+    }
+
+    @Test
+    public void hangNailPolygonInvalidTest()
+    {
+        this.verifier.actual(this.setup.getHangNailPolygonAtlas(),
+                new InvalidGeometryCheck(this.configuration));
+        this.verifier.verifyNotEmpty();
+        this.verifier.verify(flag -> Assert.assertEquals(InvalidGeometryCheckTestRule.TEST_ID_6,
+                flag.getIdentifier()));
+    }
+
+    @Test
+    public void testFineLinearTest()
+    {
+        this.verifier.actual(this.setup.getFineLinearAtlas(),
+                new InvalidGeometryCheck(this.configuration));
+        this.verifier.verifyEmpty();
+    }
+
+    @Test
+    public void testFinePolygonTest()
+    {
+        this.verifier.actual(this.setup.getFinePolygonAtlas(),
+                new InvalidGeometryCheck(this.configuration));
+        this.verifier.verifyEmpty();
+    }
+
+    @Test
+    public void testNotValidLinearTest()
+    {
+        this.verifier.actual(this.setup.getNotValidLinearAtlas(),
+                new InvalidGeometryCheck(this.configuration));
+        this.verifier.verifyNotEmpty();
+        this.verifier.verify(flag -> Assert.assertEquals(InvalidGeometryCheckTestRule.TEST_ID_2,
+                flag.getIdentifier()));
+    }
+}