From 16a028e0555dfb8bc28735885ffd5396a380707b Mon Sep 17 00:00:00 2001 From: Nicholas Knize Date: Fri, 27 Mar 2015 14:13:30 -0500 Subject: [PATCH] [GEO] Fix hole intersection at tangential coordinate OGC SFA 2.1.10 assertion 3 allows interior boundaries to touch exterior boundaries provided they intersect at a single point. Issue #9511 provides an example where a valid shape is incorrectly interpreted as invalid (a false violation of assertion 3). When the intersecting point appears as the first and last coordinate of the interior boundary in a polygon, the ShapeBuilder incorrectly counted this as multiple intersecting vertices. The fix required a little more than just a logic check. Passing the duplicate vertices resulted in a connected component in the edge graph causing an invalid self crossing polygon. This required additional logic to the edge assignment in order to correctly segment the connected components. Finally, an additional hole validation has been added along with proper unit tests for testing valid and invalid conditions (including dateline crossing polys). closes #9511 --- .../geo/builders/BasePolygonBuilder.java | 82 ++++++++-- .../common/geo/builders/ShapeBuilder.java | 23 ++- .../common/geo/GeoJSONShapeParserTests.java | 2 +- .../common/geo/ShapeBuilderTests.java | 148 +++++++++++++----- 4 files changed, 196 insertions(+), 59 deletions(-) diff --git a/src/main/java/org/elasticsearch/common/geo/builders/BasePolygonBuilder.java b/src/main/java/org/elasticsearch/common/geo/builders/BasePolygonBuilder.java index 2981145b90ab5..c1cb9bb32acf1 100644 --- a/src/main/java/org/elasticsearch/common/geo/builders/BasePolygonBuilder.java +++ b/src/main/java/org/elasticsearch/common/geo/builders/BasePolygonBuilder.java @@ -19,14 +19,18 @@ package org.elasticsearch.common.geo.builders; +import com.google.common.collect.Sets; +import com.spatial4j.core.exception.InvalidShapeException; import com.spatial4j.core.shape.Shape; import com.vividsolutions.jts.geom.*; -import org.elasticsearch.ElasticsearchParseException; +import org.apache.commons.lang3.tuple.Pair; import org.elasticsearch.common.xcontent.XContentBuilder; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; /** @@ -111,6 +115,18 @@ public ShapeBuilder close() { return shell.close(); } + /** + * Validates only 1 vertex is tangential (shared) between the interior and exterior of a polygon + */ + protected void validateHole(BaseLineStringBuilder shell, BaseLineStringBuilder hole) { + HashSet exterior = Sets.newHashSet(shell.points); + HashSet interior = Sets.newHashSet(hole.points); + exterior.retainAll(interior); + if (exterior.size() >= 2) { + throw new InvalidShapeException("Invalid polygon, interior cannot share more than one point with the exterior"); + } + } + /** * The coordinates setup by the builder will be assembled to a polygon. The result will consist of * a set of polygons. Each of these components holds a list of linestrings defining the polygon: the @@ -125,6 +141,7 @@ public Coordinate[][][] coordinates() { int numEdges = shell.points.size()-1; // Last point is repeated for (int i = 0; i < holes.size(); i++) { numEdges += holes.get(i).points.size()-1; + validateHole(shell, this.holes.get(i)); } Edge[] edges = new Edge[numEdges]; @@ -253,28 +270,62 @@ private static int component(final Edge edge, final int id, final ArrayList DATELINE ? DATELINE : (any.coordinate.x < -DATELINE ? -DATELINE : 0); + double shiftOffset = any.coordinate.x > DATELINE ? DATELINE : (any.coordinate.x < -DATELINE ? -DATELINE : 0); if (debugEnabled()) { - LOGGER.debug("shift: {[]}", shift); + LOGGER.debug("shift: {[]}", shiftOffset); } // run along the border of the component, collect the // edges, shift them according to the dateline and // update the component id - int length = 0; + int length = 0, connectedComponents = 0; + // if there are two connected components, splitIndex keeps track of where to split the edge array + // start at 1 since the source coordinate is shared + int splitIndex = 1; Edge current = edge; + Edge prev = edge; + // bookkeep the source and sink of each visited coordinate + HashMap> visitedEdge = new HashMap<>(); do { - - current.coordinate = shift(current.coordinate, shift); + current.coordinate = shift(current.coordinate, shiftOffset); current.component = id; - if(edges != null) { + + if (edges != null) { + // found a closed loop - we have two connected components so we need to slice into two distinct components + if (visitedEdge.containsKey(current.coordinate)) { + if (connectedComponents > 0 && current.next != edge) { + throw new InvalidShapeException("Shape contains more than one shared point"); + } + + // a negative id flags the edge as visited for the edges(...) method. + // since we're splitting connected components, we want the edges method to visit + // the newly separated component + final int visitID = -id; + Edge firstAppearance = visitedEdge.get(current.coordinate).getRight(); + // correct the graph pointers by correcting the 'next' pointer for both the + // first appearance and this appearance of the edge + Edge temp = firstAppearance.next; + firstAppearance.next = current.next; + current.next = temp; + current.component = visitID; + // backtrack until we get back to this coordinate, setting the visit id to + // a non-visited value (anything positive) + do { + prev.component = visitID; + prev = visitedEdge.get(prev.coordinate).getLeft(); + ++splitIndex; + } while (!current.coordinate.equals(prev.coordinate)); + ++connectedComponents; + } else { + visitedEdge.put(current.coordinate, Pair.of(prev, current)); + } edges.add(current); + prev = current; } - length++; - } while((current = current.next) != edge); + } while(connectedComponents == 0 && (current = current.next) != edge); - return length; + return (splitIndex != 1) ? length-splitIndex: length; } /** @@ -364,11 +415,12 @@ private static void assign(Edge[] holes, Coordinate[][] points, int numHoles, Ed // if no intersection is found then the hole is not within the polygon, so // don't waste time calling a binary search final int pos; - if (intersections == 0 || - (pos = Arrays.binarySearch(edges, 0, intersections, current, INTERSECTION_ORDER)) >= 0) { - throw new ElasticsearchParseException("Invalid shape: Hole is not within polygon"); + boolean sharedVertex = false; + if (intersections == 0 || ((pos = Arrays.binarySearch(edges, 0, intersections, current, INTERSECTION_ORDER)) >= 0) + && !(sharedVertex = (edges[pos].intersect.compareTo(current.coordinate) == 0)) ) { + throw new InvalidShapeException("Invalid shape: Hole is not within polygon"); } - final int index = -(pos+2); + final int index = -((sharedVertex) ? 0 : pos+2); final int component = -edges[index].component - numHoles - 1; if(debugEnabled()) { @@ -465,7 +517,7 @@ private static int createEdges(int component, Orientation orientation, BaseLineS Edge[] edges, int offset) { // inner rings (holes) have an opposite direction than the outer rings // XOR will invert the orientation for outer ring cases (Truth Table:, T/T = F, T/F = T, F/T = T, F/F = F) - boolean direction = (component != 0 ^ orientation == Orientation.RIGHT); + boolean direction = (component == 0 ^ orientation == Orientation.RIGHT); // set the points array accordingly (shell or hole) Coordinate[] points = (hole != null) ? hole.coordinates(false) : shell.coordinates(false); Edge.ring(component, direction, orientation == Orientation.LEFT, shell, points, 0, edges, offset, points.length-1); diff --git a/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java b/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java index 1154585507706..98779c5f1a86e 100644 --- a/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java +++ b/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java @@ -20,6 +20,7 @@ package org.elasticsearch.common.geo.builders; import com.spatial4j.core.context.jts.JtsSpatialContext; +import com.spatial4j.core.exception.InvalidShapeException; import com.spatial4j.core.shape.Shape; import com.spatial4j.core.shape.jts.JtsGeometry; import com.vividsolutions.jts.geom.Coordinate; @@ -446,7 +447,8 @@ protected static final class Edge { protected Edge(Coordinate coordinate, Edge next, Coordinate intersection) { this.coordinate = coordinate; - this.next = next; + // use setter to catch duplicate point cases + this.setNext(next); this.intersect = intersection; if (next != null) { this.component = next.component; @@ -457,6 +459,17 @@ protected Edge(Coordinate coordinate, Edge next) { this(coordinate, next, Edge.MAX_COORDINATE); } + protected void setNext(Edge next) { + // don't bother setting next if its null + if (next != null) { + // self-loop throws an invalid shape + if (this.coordinate.equals(next.coordinate)) { + throw new InvalidShapeException("Provided shape has duplicate consecutive coordinates at: " + this.coordinate); + } + this.next = next; + } + } + private static final int top(Coordinate[] points, int offset, int length) { int top = 0; // we start at 1 here since top points to 0 for (int i = 1; i < length; i++) { @@ -522,17 +535,19 @@ private static Edge[] concat(int component, boolean direction, Coordinate[] poin if (direction) { edges[edgeOffset + i] = new Edge(points[pointOffset + i], edges[edgeOffset + i - 1]); edges[edgeOffset + i].component = component; - } else { + } else if(!edges[edgeOffset + i - 1].coordinate.equals(points[pointOffset + i])) { edges[edgeOffset + i - 1].next = edges[edgeOffset + i] = new Edge(points[pointOffset + i], null); edges[edgeOffset + i - 1].component = component; + } else { + throw new InvalidShapeException("Provided shape has duplicate consecutive coordinates at: " + points[pointOffset + i]); } } if (direction) { - edges[edgeOffset].next = edges[edgeOffset + length - 1]; + edges[edgeOffset].setNext(edges[edgeOffset + length - 1]); edges[edgeOffset].component = component; } else { - edges[edgeOffset + length - 1].next = edges[edgeOffset]; + edges[edgeOffset + length - 1].setNext(edges[edgeOffset]); edges[edgeOffset + length - 1].component = component; } diff --git a/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java b/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java index 0b71570403f5f..28cc9f41cd1d3 100644 --- a/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java +++ b/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java @@ -267,7 +267,7 @@ public void testParse_invalidMultiPolygon() throws IOException { XContentParser parser = JsonXContent.jsonXContent.createParser(multiPolygonGeoJson); parser.nextToken(); - ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + ElasticsearchGeoAssertions.assertValidException(parser, InvalidShapeException.class); } public void testParse_OGCPolygonWithoutHoles() throws IOException { diff --git a/src/test/java/org/elasticsearch/common/geo/ShapeBuilderTests.java b/src/test/java/org/elasticsearch/common/geo/ShapeBuilderTests.java index cb9c53846f2bd..c706288c04a64 100644 --- a/src/test/java/org/elasticsearch/common/geo/ShapeBuilderTests.java +++ b/src/test/java/org/elasticsearch/common/geo/ShapeBuilderTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.common.geo; +import com.spatial4j.core.exception.InvalidShapeException; import com.spatial4j.core.shape.Circle; import com.spatial4j.core.shape.Point; import com.spatial4j.core.shape.Rectangle; @@ -27,6 +28,7 @@ import com.vividsolutions.jts.geom.Coordinate; import com.vividsolutions.jts.geom.LineString; import com.vividsolutions.jts.geom.Polygon; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.geo.builders.PolygonBuilder; import org.elasticsearch.common.geo.builders.ShapeBuilder; import org.elasticsearch.test.ElasticsearchTestCase; @@ -38,14 +40,12 @@ */ public class ShapeBuilderTests extends ElasticsearchTestCase { - @Test public void testNewPoint() { Point point = ShapeBuilder.newPoint(-100, 45).build(); assertEquals(-100D, point.getX(), 0.0d); assertEquals(45D, point.getY(), 0.0d); } - @Test public void testNewRectangle() { Rectangle rectangle = ShapeBuilder.newEnvelope().topLeft(-45, 30).bottomRight(45, -30).build(); assertEquals(-45D, rectangle.getMinX(), 0.0d); @@ -54,7 +54,6 @@ public void testNewRectangle() { assertEquals(30D, rectangle.getMaxY(), 0.0d); } - @Test public void testNewPolygon() { Polygon polygon = ShapeBuilder.newPolygon() .point(-45, 30) @@ -70,7 +69,6 @@ public void testNewPolygon() { assertEquals(exterior.getCoordinateN(3), new Coordinate(-45, -30)); } - @Test public void testNewPolygon_coordinate() { Polygon polygon = ShapeBuilder.newPolygon() .point(new Coordinate(-45, 30)) @@ -86,7 +84,6 @@ public void testNewPolygon_coordinate() { assertEquals(exterior.getCoordinateN(3), new Coordinate(-45, -30)); } - @Test public void testNewPolygon_coordinates() { Polygon polygon = ShapeBuilder.newPolygon() .points(new Coordinate(-45, 30), new Coordinate(45, 30), new Coordinate(45, -30), new Coordinate(-45, -30), new Coordinate(-45, 30)).toPolygon(); @@ -97,8 +94,7 @@ public void testNewPolygon_coordinates() { assertEquals(exterior.getCoordinateN(2), new Coordinate(45, -30)); assertEquals(exterior.getCoordinateN(3), new Coordinate(-45, -30)); } - - @Test + public void testLineStringBuilder() { // Building a simple LineString ShapeBuilder.newLineString() @@ -140,7 +136,6 @@ public void testLineStringBuilder() { .build(); } - @Test public void testMultiLineString() { ShapeBuilder.newMultiLinestring() .linestring() @@ -174,22 +169,17 @@ public void testMultiLineString() { .end() .build(); } - - @Test + + @Test(expected = InvalidShapeException.class) public void testPolygonSelfIntersection() { - try { - ShapeBuilder.newPolygon() + ShapeBuilder.newPolygon() .point(-40.0, 50.0) .point(40.0, 50.0) .point(-40.0, -50.0) .point(40.0, -50.0) - .close().build(); - fail("Polygon self-intersection"); - } catch (Throwable e) {} - + .close().build(); } - @Test public void testGeoCircle() { double earthCircumference = 40075016.69; Circle circle = ShapeBuilder.newCircleBuilder().center(0, 0).radius("100m").build(); @@ -214,8 +204,7 @@ public void testGeoCircle() { assertEquals((360 * randomRadius) / earthCircumference, circle.getRadius(), 0.00000001); assertEquals((Point) new PointImpl(randomLon, randomLat, ShapeBuilder.SPATIAL_CONTEXT), circle.getCenter()); } - - @Test + public void testPolygonWrapping() { Shape shape = ShapeBuilder.newPolygon() .point(-150.0, 65.0) @@ -227,7 +216,6 @@ public void testPolygonWrapping() { assertMultiPolygon(shape); } - @Test public void testLineStringWrapping() { Shape shape = ShapeBuilder.newLineString() .point(-150.0, 65.0) @@ -235,11 +223,9 @@ public void testLineStringWrapping() { .point(-250.0, -65.0) .point(-150.0, -65.0) .build(); - assertMultiLineString(shape); } - @Test public void testDatelineOGC() { // tests that the following shape (defined in counterclockwise OGC order) // https://gist.github.com/anonymous/7f1bb6d7e9cd72f5977c crosses the dateline @@ -278,11 +264,9 @@ public void testDatelineOGC() { .point(-179,1); Shape shape = builder.close().build(); - assertMultiPolygon(shape); } - @Test public void testDateline() { // tests that the following shape (defined in clockwise non-OGC order) // https://gist.github.com/anonymous/7f1bb6d7e9cd72f5977c crosses the dateline @@ -321,11 +305,9 @@ public void testDateline() { .point(-179,1); Shape shape = builder.close().build(); - assertMultiPolygon(shape); } - - @Test + public void testComplexShapeWithHole() { PolygonBuilder builder = ShapeBuilder.newPolygon() .point(-85.0018514,37.1311314) @@ -396,11 +378,9 @@ public void testComplexShapeWithHole() { .point(-85.0000002,37.1317672); Shape shape = builder.close().build(); - - assertPolygon(shape); + assertPolygon(shape); } - @Test public void testShapeWithHoleAtEdgeEndPoints() { PolygonBuilder builder = ShapeBuilder.newPolygon() .point(-4, 2) @@ -419,11 +399,9 @@ public void testShapeWithHoleAtEdgeEndPoints() { .point(4, 1); Shape shape = builder.close().build(); - - assertPolygon(shape); + assertPolygon(shape); } - @Test public void testShapeWithPointOnDateline() { PolygonBuilder builder = ShapeBuilder.newPolygon() .point(180, 0) @@ -432,11 +410,9 @@ public void testShapeWithPointOnDateline() { .point(180, 0); Shape shape = builder.close().build(); - - assertPolygon(shape); + assertPolygon(shape); } - @Test public void testShapeWithEdgeAlongDateline() { // test case 1: test the positive side of the dateline PolygonBuilder builder = ShapeBuilder.newPolygon() @@ -459,7 +435,6 @@ public void testShapeWithEdgeAlongDateline() { assertPolygon(shape); } - @Test public void testShapeWithBoundaryHoles() { // test case 1: test the positive side of the dateline PolygonBuilder builder = ShapeBuilder.newPolygon() @@ -484,7 +459,7 @@ public void testShapeWithBoundaryHoles() { .point(179, 10) .point(179, -10) .point(-176, -15) - .point(-172,0); + .point(-172, 0); builder.hole() .point(-176, 10) .point(-176, -10) @@ -495,6 +470,89 @@ public void testShapeWithBoundaryHoles() { assertMultiPolygon(shape); } + public void testShapeWithTangentialHole() { + // test a shape with one tangential (shared) vertex (should pass) + PolygonBuilder builder = ShapeBuilder.newPolygon() + .point(179, 10) + .point(168, 15) + .point(164, 0) + .point(166, -15) + .point(179, -10) + .point(179, 10); + builder.hole() + .point(-177, 10) + .point(-178, -10) + .point(-180, -5) + .point(-180, 5) + .point(-177, 10); + Shape shape = builder.close().build(); + assertMultiPolygon(shape); + } + + @Test(expected = InvalidShapeException.class) + public void testShapeWithInvalidTangentialHole() { + // test a shape with one invalid tangential (shared) vertex (should throw exception) + PolygonBuilder builder = ShapeBuilder.newPolygon() + .point(179, 10) + .point(168, 15) + .point(164, 0) + .point(166, -15) + .point(179, -10) + .point(179, 10); + builder.hole() + .point(164, 0) + .point(175, 10) + .point(175, 5) + .point(179, -10) + .point(164, 0); + Shape shape = builder.close().build(); + assertMultiPolygon(shape); + } + + public void testBoundaryShapeWithTangentialHole() { + // test a shape with one tangential (shared) vertex for each hole (should pass) + PolygonBuilder builder = ShapeBuilder.newPolygon() + .point(-177, 10) + .point(176, 15) + .point(172, 0) + .point(176, -15) + .point(-177, -10) + .point(-177, 10); + builder.hole() + .point(-177, 10) + .point(-178, -10) + .point(-180, -5) + .point(-180, 5) + .point(-177, 10); + builder.hole() + .point(172, 0) + .point(176, 10) + .point(176, -5) + .point(172, 0); + Shape shape = builder.close().build(); + assertMultiPolygon(shape); + } + + @Test(expected = InvalidShapeException.class) + public void testBoundaryShapeWithInvalidTangentialHole() { + // test shape with two tangential (shared) vertices (should throw exception) + PolygonBuilder builder = ShapeBuilder.newPolygon() + .point(-177, 10) + .point(176, 15) + .point(172, 0) + .point(176, -15) + .point(-177, -10) + .point(-177, 10); + builder.hole() + .point(-177, 10) + .point(172, 0) + .point(180, -5) + .point(176, -10) + .point(-177, 10); + Shape shape = builder.close().build(); + assertMultiPolygon(shape); + } + /** * Test an enveloping polygon around the max mercator bounds */ @@ -513,7 +571,7 @@ public void testBoundaryShape() { @Test public void testShapeWithAlternateOrientation() { - // ccw: should produce a single polygon spanning hemispheres + // cw: should produce a multi polygon spanning hemispheres PolygonBuilder builder = ShapeBuilder.newPolygon() .point(180, 0) .point(176, 4) @@ -534,4 +592,16 @@ public void testShapeWithAlternateOrientation() { assertMultiPolygon(shape); } + + @Test(expected = InvalidShapeException.class) + public void testInvalidShapeWithConsecutiveDuplicatePoints() { + PolygonBuilder builder = ShapeBuilder.newPolygon() + .point(180, 0) + .point(176, 4) + .point(176, 4) + .point(-176, 4) + .point(180, 0); + Shape shape = builder.close().build(); + assertPolygon(shape); + } }