[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
This commit is contained in:
Nicholas Knize 2015-03-27 14:13:30 -05:00
parent 97a9b4ec2f
commit a8a35d7c29
4 changed files with 192 additions and 52 deletions

View File

@ -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 abstract class BasePolygonBuilder<E extends BasePolygonBuilder<E>> extend
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 abstract class BasePolygonBuilder<E extends BasePolygonBuilder<E>> extend
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 @@ public abstract class BasePolygonBuilder<E extends BasePolygonBuilder<E>> extend
}
}
double shift = any.coordinate.x > 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<Coordinate, Pair<Edge, Edge>> 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 @@ public abstract class BasePolygonBuilder<E extends BasePolygonBuilder<E>> extend
// 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 @@ public abstract class BasePolygonBuilder<E extends BasePolygonBuilder<E>> extend
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);

View File

@ -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 @@ public abstract class ShapeBuilder implements ToXContent {
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 @@ public abstract class ShapeBuilder implements ToXContent {
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 @@ public abstract class ShapeBuilder implements ToXContent {
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;
}

View File

@ -267,7 +267,7 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase {
XContentParser parser = JsonXContent.jsonXContent.createParser(multiPolygonGeoJson);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
ElasticsearchGeoAssertions.assertValidException(parser, InvalidShapeException.class);
}
public void testParse_OGCPolygonWithoutHoles() throws IOException {

View File

@ -28,6 +28,7 @@ import com.spatial4j.core.shape.impl.PointImpl;
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;
@ -39,14 +40,12 @@ import static org.elasticsearch.test.hamcrest.ElasticsearchGeoAssertions.*;
*/
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);
@ -55,7 +54,6 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
assertEquals(30D, rectangle.getMaxY(), 0.0d);
}
@Test
public void testNewPolygon() {
Polygon polygon = ShapeBuilder.newPolygon()
.point(-45, 30)
@ -71,7 +69,6 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
assertEquals(exterior.getCoordinateN(3), new Coordinate(-45, -30));
}
@Test
public void testNewPolygon_coordinate() {
Polygon polygon = ShapeBuilder.newPolygon()
.point(new Coordinate(-45, 30))
@ -87,7 +84,6 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
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();
@ -98,8 +94,7 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
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()
@ -141,7 +136,6 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
.build();
}
@Test
public void testMultiLineString() {
ShapeBuilder.newMultiLinestring()
.linestring()
@ -175,7 +169,7 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
.end()
.build();
}
@Test(expected = InvalidShapeException.class)
public void testPolygonSelfIntersection() {
ShapeBuilder.newPolygon()
@ -186,7 +180,6 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
.close().build();
}
@Test
public void testGeoCircle() {
double earthCircumference = 40075016.69;
Circle circle = ShapeBuilder.newCircleBuilder().center(0, 0).radius("100m").build();
@ -211,8 +204,7 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
assertEquals((360 * randomRadius) / earthCircumference, circle.getRadius(), 0.00000001);
assertEquals(new PointImpl(randomLon, randomLat, ShapeBuilder.SPATIAL_CONTEXT), circle.getCenter());
}
@Test
public void testPolygonWrapping() {
Shape shape = ShapeBuilder.newPolygon()
.point(-150.0, 65.0)
@ -224,7 +216,6 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
assertMultiPolygon(shape);
}
@Test
public void testLineStringWrapping() {
Shape shape = ShapeBuilder.newLineString()
.point(-150.0, 65.0)
@ -232,11 +223,9 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
.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
@ -275,11 +264,9 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
.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
@ -318,11 +305,9 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
.point(-179,1);
Shape shape = builder.close().build();
assertMultiPolygon(shape);
}
@Test
public void testComplexShapeWithHole() {
PolygonBuilder builder = ShapeBuilder.newPolygon()
.point(-85.0018514,37.1311314)
@ -393,11 +378,9 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
.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)
@ -416,11 +399,9 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
.point(4, 1);
Shape shape = builder.close().build();
assertPolygon(shape);
assertPolygon(shape);
}
@Test
public void testShapeWithPointOnDateline() {
PolygonBuilder builder = ShapeBuilder.newPolygon()
.point(180, 0)
@ -429,11 +410,9 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
.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()
@ -456,7 +435,6 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
assertPolygon(shape);
}
@Test
public void testShapeWithBoundaryHoles() {
// test case 1: test the positive side of the dateline
PolygonBuilder builder = ShapeBuilder.newPolygon()
@ -481,7 +459,7 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
.point(179, 10)
.point(179, -10)
.point(-176, -15)
.point(-172,0);
.point(-172, 0);
builder.hole()
.point(-176, 10)
.point(-176, -10)
@ -492,6 +470,89 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
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
*/
@ -510,7 +571,7 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
@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)
@ -531,4 +592,16 @@ public class ShapeBuilderTests extends ElasticsearchTestCase {
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);
}
}