From bcfb796b6884651f5761b2cfb257fa70175b8d47 Mon Sep 17 00:00:00 2001 From: David Turner Date: Fri, 22 Dec 2017 19:12:34 +0000 Subject: [PATCH 1/5] Use the determinant formula for calculating the orientation of a polygon --- .../common/geo/builders/PolygonBuilder.java | 32 +++++++++++++------ .../geo/builders/PolygonBuilderTests.java | 11 +++++++ 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java index b0b37dbafa9a3..b17e5128fb35e 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java @@ -33,7 +33,6 @@ import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.common.xcontent.XContentBuilder; import org.locationtech.spatial4j.exception.InvalidShapeException; -import org.locationtech.spatial4j.shape.Shape; import org.locationtech.spatial4j.shape.jts.JtsGeometry; import java.io.IOException; @@ -634,14 +633,8 @@ private static int createEdges(int component, Orientation orientation, LineStrin */ private static Edge[] ring(int component, boolean direction, boolean handedness, Coordinate[] points, int offset, Edge[] edges, int toffset, int length, final AtomicBoolean translated) { - // calculate the direction of the points: - // find the point a the top of the set and check its - // neighbors orientation. So direction is equivalent - // to clockwise/counterclockwise - final int top = top(points, offset, length); - final int prev = (offset + ((top + length - 1) % length)); - final int next = (offset + ((top + 1) % length)); - boolean orientation = points[offset + prev].x > points[offset + next].x; + + boolean orientation = getOrientation(points, offset, length); // OGC requires shell as ccw (Right-Handedness) and holes as cw (Left-Handedness) // since GeoJSON doesn't specify (and doesn't need to) GEO core will assume OGC standards @@ -670,6 +663,27 @@ private static Edge[] ring(int component, boolean direction, boolean handedness, return concat(component, direction ^ orientation, points, offset, edges, toffset, length); } + /** + * @return whether the points are clockwise (true) or anticlockwise (false) + */ + private static boolean getOrientation(Coordinate[] points, int offset, int length) { + // calculate the direction of the points: find the southernmost point + // and check its neighbors orientation. + + final int top = top(points, offset, length); + final int prev = (top + length - 1) % length; + final int next = (top + 1) % length; + final double determinant + = (points[offset + next].x - points[offset + top].x) * (points[offset + prev].y - points[offset + top].y) + - (points[offset + prev].x - points[offset + top].x) * (points[offset + next].y - points[offset + top].y); + assert determinant != 0.0; + return determinant < 0.0; + } + + /** + * @return the (offset) index of the point that is furthest west amongst + * those points that are the furthest south in the set. + */ private static 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++) { diff --git a/core/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java b/core/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java index 8501760d1e772..29a40da2ae60c 100644 --- a/core/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java @@ -143,4 +143,15 @@ public void testHoleThatIsNorthOfPolygon() { assertEquals("Hole lies outside shell at or near point (4.0, 3.0, NaN)", e.getMessage()); } + + public void testWidePolygonWithConfusingOrientation() { + // A valid polygon that is oriented correctly (anticlockwise) but which + // confounds a naive algorithm for determining its orientation leading + // ES to believe that it crosses the dateline and "fixing" it in a way + // that self-intersects. + + PolygonBuilder pb = new PolygonBuilder(new CoordinatesBuilder() + .coordinate(10, -20).coordinate(100, 0).coordinate(-100, 0).coordinate(20, -45).coordinate(40, -60).close()); + pb.build(); // Should not throw an exception + } } From 0efe0d9fd9e7eae28634ada61aa18759d43947c4 Mon Sep 17 00:00:00 2001 From: David Turner Date: Wed, 28 Mar 2018 09:03:56 +0100 Subject: [PATCH 2/5] Reinstate code lost in merge --- .../common/geo/builders/PolygonBuilder.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java b/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java index b17e5128fb35e..8aa1d8cdff053 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java +++ b/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java @@ -282,6 +282,15 @@ public GeoShapeType type() { return TYPE; } + @Override + public int numDimensions() { + if (shell == null) { + throw new IllegalStateException("unable to get number of dimensions, " + + "Polygon has not yet been initialized"); + } + return shell.numDimensions(); + } + protected static Polygon polygon(GeometryFactory factory, Coordinate[][] polygon) { LinearRing shell = factory.createLinearRing(polygon[0]); LinearRing[] holes; From b4ba0da5694212bcdcce8bf9925b5d05821f42f4 Mon Sep 17 00:00:00 2001 From: David Turner Date: Wed, 28 Mar 2018 09:09:03 +0100 Subject: [PATCH 3/5] Add TODO note --- .../org/elasticsearch/common/geo/builders/PolygonBuilder.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java b/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java index 8aa1d8cdff053..a3d3130758e37 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java +++ b/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java @@ -682,6 +682,9 @@ private static boolean getOrientation(Coordinate[] points, int offset, int lengt final int top = top(points, offset, length); final int prev = (top + length - 1) % length; final int next = (top + 1) % length; + + // This duplicates the functionality of org.apache.lucene.geo.Polygon2D#orient which is, at time of writing, private. + // TODO make Lucene's method public and then use it here instead. final double determinant = (points[offset + next].x - points[offset + top].x) * (points[offset + prev].y - points[offset + top].y) - (points[offset + prev].x - points[offset + top].x) * (points[offset + next].y - points[offset + top].y); From 2e61a5ac526e39ad87ef9e8625e37001daa6bb6d Mon Sep 17 00:00:00 2001 From: David Turner Date: Mon, 30 Jul 2018 06:49:06 +0100 Subject: [PATCH 4/5] Use method from Lucene --- .../common/geo/builders/PolygonBuilder.java | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java b/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java index 25248ce854d77..8584d1fc0e5ad 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java +++ b/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java @@ -46,6 +46,8 @@ import java.util.Objects; import java.util.concurrent.atomic.AtomicBoolean; +import static org.apache.lucene.geo.GeoUtils.orient; + /** * The {@link PolygonBuilder} implements the groundwork to create polygons. This contains * Methods to wrap polygons at the dateline and building shapes from the data held by the @@ -683,13 +685,13 @@ private static boolean getOrientation(Coordinate[] points, int offset, int lengt final int prev = (top + length - 1) % length; final int next = (top + 1) % length; - // This duplicates the functionality of org.apache.lucene.geo.Polygon2D#orient which is, at time of writing, private. - // TODO make Lucene's method public and then use it here instead. - final double determinant - = (points[offset + next].x - points[offset + top].x) * (points[offset + prev].y - points[offset + top].y) - - (points[offset + prev].x - points[offset + top].x) * (points[offset + next].y - points[offset + top].y); - assert determinant != 0.0; - return determinant < 0.0; + final int determinantSign = orient( + points[offset + prev].x, points[offset + prev].y, + points[offset + top].x, points[offset + top].y, + points[offset + next].x, points[offset + next].y); + + assert determinantSign != 0; + return determinantSign < 0; } /** From 0084311b297d8b2426b87b5f9e715849b8940e99 Mon Sep 17 00:00:00 2001 From: David Turner Date: Mon, 30 Jul 2018 06:50:26 +0100 Subject: [PATCH 5/5] Throw InvalidShapeException on collinear points --- .../elasticsearch/common/geo/builders/PolygonBuilder.java | 7 ++++++- .../common/geo/builders/PolygonBuilderTests.java | 7 +++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java b/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java index 8584d1fc0e5ad..e4d48b8df4566 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java +++ b/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java @@ -690,7 +690,12 @@ private static boolean getOrientation(Coordinate[] points, int offset, int lengt points[offset + top].x, points[offset + top].y, points[offset + next].x, points[offset + next].y); - assert determinantSign != 0; + if (determinantSign == 0) { + // Points are collinear, but `top` is not in the middle if so, so the edges either side of `top` are intersecting. + throw new InvalidShapeException("Cannot determine orientation: edges adjacent to (" + + points[offset + top].x + "," + points[offset +top].y + ") coincide"); + } + return determinantSign < 0; } diff --git a/server/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java b/server/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java index 19fbc35a5753f..83bf2bae6a66f 100644 --- a/server/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java @@ -154,4 +154,11 @@ public void testWidePolygonWithConfusingOrientation() { .coordinate(10, -20).coordinate(100, 0).coordinate(-100, 0).coordinate(20, -45).coordinate(40, -60).close()); pb.build(); // Should not throw an exception } + + public void testPolygonWithUndefinedOrientationDueToCollinearPoints() { + PolygonBuilder pb = new PolygonBuilder(new CoordinatesBuilder() + .coordinate(0.0, 0.0).coordinate(1.0, 1.0).coordinate(-1.0, -1.0).close()); + InvalidShapeException e = expectThrows(InvalidShapeException.class, pb::build); + assertEquals("Cannot determine orientation: edges adjacent to (-1.0,-1.0) coincide", e.getMessage()); + } }