diff --git a/config/configuration.json b/config/configuration.json index 2ea7d3cb2..210491085 100644 --- a/config/configuration.json +++ b/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":{ diff --git a/dependencies.gradle b/dependencies.gradle index f1a8b9cf7..7f112a7bd 100644 --- a/dependencies.gradle +++ b/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', diff --git a/docs/available_checks.md b/docs/available_checks.md index 2a6d946d4..c9f8cc679 100644 --- a/docs/available_checks.md +++ b/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. | diff --git a/docs/checks/invalidGeometryCheck.md b/docs/checks/invalidGeometryCheck.md new file mode 100644 index 000000000..eb02a6a5e --- /dev/null +++ b/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) diff --git a/src/main/java/org/openstreetmap/atlas/checks/validation/GeometryValidator.java b/src/main/java/org/openstreetmap/atlas/checks/validation/GeometryValidator.java index 029327fe1..7670c8146 100644 --- a/src/main/java/org/openstreetmap/atlas/checks/validation/GeometryValidator.java +++ b/src/main/java/org/openstreetmap/atlas/checks/validation/GeometryValidator.java @@ -1,8 +1,11 @@ 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; @@ -10,36 +13,64 @@ * Basic JTS verification for a {@link PolyLine} and {@link Polygon} * * @author mgostintsev + * @author bbreithaupt */ public final class GeometryValidator { + private static final Optional NOT_SIMPLE_LINEAR = Optional.of( + "Linear geometry intersecting itself at interior points (points other than the boundary)"); + private static final Optional NOT_SIMPLE_POINT = Optional + .of("Point geometry has repeated point"); + private static final Optional NOT_SIMPLE_POLYGON = Optional + .of("Polygon geometry is malformed"); + private static final Optional NOT_VALID_LINEAR = Optional + .of("Linear geometry has exactly two identical points"); + private static final Optional NOT_VALID_POINT = Optional + .of("Point geometry has invalid dimension value (NaN)"); + private static final Optional 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 testSimplicity(final Iterable 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 testValidity(final Iterable 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() diff --git a/src/main/java/org/openstreetmap/atlas/checks/validation/geometry/InvalidGeometryCheck.java b/src/main/java/org/openstreetmap/atlas/checks/validation/geometry/InvalidGeometryCheck.java new file mode 100644 index 000000000..3cfe4d78d --- /dev/null +++ b/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 +{ + 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 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 flag(final AtlasObject object) + { + final List instructions = new ArrayList<>(); + + // check if simple if not a LineItem (too many correct in OSM but not simple linear + // geometries) + final Optional simpleTest = object instanceof LineItem ? Optional.empty() + : GeometryValidator.testSimplicity(((AtlasItem) object).getRawGeometry()); + // check if valid + final Optional 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 getFallbackInstructions() + { + return FALLBACK_INSTRUCTIONS; + } + +} diff --git a/src/main/java/org/openstreetmap/atlas/checks/validation/intersections/SelfIntersectingPolylineCheck.java b/src/main/java/org/openstreetmap/atlas/checks/validation/intersections/SelfIntersectingPolylineCheck.java index 2434ff34d..39e4266e8 100644 --- a/src/main/java/org/openstreetmap/atlas/checks/validation/intersections/SelfIntersectingPolylineCheck.java +++ b/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) { diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/geometry/InvalidGeometryCheckTest.java b/src/test/java/org/openstreetmap/atlas/checks/validation/geometry/InvalidGeometryCheckTest.java new file mode 100644 index 000000000..7b7e0b323 --- /dev/null +++ b/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())); + } +} diff --git a/src/test/java/org/openstreetmap/atlas/checks/validation/geometry/InvalidGeometryCheckTestRule.java b/src/test/java/org/openstreetmap/atlas/checks/validation/geometry/InvalidGeometryCheckTestRule.java new file mode 100644 index 000000000..f06eadaa9 --- /dev/null +++ b/src/test/java/org/openstreetmap/atlas/checks/validation/geometry/InvalidGeometryCheckTestRule.java @@ -0,0 +1,134 @@ +package org.openstreetmap.atlas.checks.validation.geometry; + +import org.openstreetmap.atlas.geography.atlas.Atlas; +import org.openstreetmap.atlas.utilities.testing.CoreTestRule; +import org.openstreetmap.atlas.utilities.testing.TestAtlas; +import org.openstreetmap.atlas.utilities.testing.TestAtlas.Area; +import org.openstreetmap.atlas.utilities.testing.TestAtlas.Edge; +import org.openstreetmap.atlas.utilities.testing.TestAtlas.Line; +import org.openstreetmap.atlas.utilities.testing.TestAtlas.Loc; +import org.openstreetmap.atlas.utilities.testing.TestAtlas.Node; + +/** + * {@link InvalidGeometryCheckTest} data generator + * + * @author jklamer + * @author bbreithaupt + */ +public class InvalidGeometryCheckTestRule extends CoreTestRule +{ + public static final String TEST_ID_1 = "123456789000000"; + public static final String TEST_ID_2 = "223456789000000"; + public static final String TEST_ID_3 = "323456789000000"; + public static final String TEST_ID_4 = "423456789000000"; + public static final String TEST_ID_5 = "523456789000000"; + public static final String TEST_ID_6 = "623456789000000"; + public static final String TEST_ID_7 = "723456789000000"; + private static final String LOCATION_1 = "10.13076961357, -80.73619975709"; + private static final String LOCATION_2 = "10.12871245371, -80.69336019785"; + private static final String LOCATION_3 = "10.09288147237, -80.73759291349"; + private static final String LOCATION_4 = "10.09168132254, -80.69405677605"; + private static final String LOCATION_5 = "10.10625426762, -80.7243579277"; + private static final String LOCATION_6 = "10.11654065468, -80.71007807462"; + private static final String LOCATION_7 = "10.11654065468, NaN"; + private static final String LOCATION_8 = "10.11448340362, -80.7306271315"; + + @TestAtlas( + // areas + areas = { @Area(id = TEST_ID_5, coordinates = { @Loc(value = LOCATION_1), + @Loc(value = LOCATION_4), @Loc(value = LOCATION_2), @Loc(value = LOCATION_3), + @Loc(value = LOCATION_1) }) }) + private Atlas bowtiePolygonAtlas; + + @TestAtlas( + // areas + areas = { @Area(id = TEST_ID_7, coordinates = { @Loc(value = LOCATION_1), + @Loc(value = LOCATION_2), @Loc(value = LOCATION_4), @Loc(value = LOCATION_6), + @Loc(value = LOCATION_5), @Loc(value = LOCATION_4), @Loc(value = LOCATION_3), + @Loc(value = LOCATION_5), @Loc(value = LOCATION_8), @Loc(value = LOCATION_3), + @Loc(value = LOCATION_1) }) }) + private Atlas disconnectedCenterPolygonAtlas; + + @TestAtlas( + // lines + lines = { @Line(coordinates = { @Loc(value = LOCATION_3), @Loc(value = LOCATION_4), + @Loc(value = LOCATION_2), @Loc(value = LOCATION_6), @Loc(value = LOCATION_5), + @Loc(value = LOCATION_1) }) }) + private Atlas fineLinearAtlas; + + @TestAtlas( + // areas + areas = { @Area(coordinates = { @Loc(value = LOCATION_1), @Loc(value = LOCATION_2), + @Loc(value = LOCATION_4), @Loc(value = LOCATION_3), @Loc(LOCATION_1) }) }) + private Atlas finePolygonAtlas; + + @TestAtlas( + // areas + areas = { @Area(id = TEST_ID_6, coordinates = { @Loc(value = LOCATION_1), + @Loc(value = LOCATION_2), @Loc(value = LOCATION_4), @Loc(value = LOCATION_5), + @Loc(value = LOCATION_2) }) }) + private Atlas hangNailPolygonAtlas; + + @TestAtlas( + // nodes + nodes = { + @Node(coordinates = @Loc(value = LOCATION_5), tags = "synthetic_boundary_node=yes") }, + // edges + edges = { @Edge(id = TEST_ID_3, coordinates = { @Loc(value = LOCATION_5), + @Loc(value = LOCATION_5) }) }) + private Atlas boundaryNodeAtlas; + + @TestAtlas( + // lines + lines = { @Line(id = TEST_ID_2, coordinates = { @Loc(value = LOCATION_8), + @Loc(value = LOCATION_8) }) }) + private Atlas notValidLinearAtlas; + + @TestAtlas( + // areas + areas = { @Area(id = TEST_ID_5, coordinates = { @Loc(value = LOCATION_1), + @Loc(value = LOCATION_4), @Loc(value = LOCATION_2), @Loc(value = LOCATION_3), + @Loc(value = LOCATION_1) }, tags = "synthetic_geometry_sliced=yes") }) + private Atlas borderSlicedPolygonAtlas; + + public Atlas borderSlicedPolygonAtlas() + { + return this.borderSlicedPolygonAtlas; + } + + public Atlas boundaryNodeAtlas() + { + return this.boundaryNodeAtlas; + } + + public Atlas getBowtiePolygonAtlas() + { + return this.bowtiePolygonAtlas; + } + + public Atlas getDisconnectedCenterPolygonAtlas() + { + return this.disconnectedCenterPolygonAtlas; + } + + public Atlas getFineLinearAtlas() + { + return this.fineLinearAtlas; + } + + public Atlas getFinePolygonAtlas() + { + return this.finePolygonAtlas; + } + + public Atlas getHangNailPolygonAtlas() + { + return this.hangNailPolygonAtlas; + } + + public Atlas getNotValidLinearAtlas() + { + return this.notValidLinearAtlas; + } + +}