From e9e13d5cfc5b891843e007c888911921a8e71d44 Mon Sep 17 00:00:00 2001 From: Nicholas Knize Date: Tue, 2 Dec 2014 16:31:30 -0600 Subject: [PATCH] Computational geometry logic changes to support OGC standards This commit adds the logic necessary for supporting polygon vertex ordering per OGC standards. Exterior rings will be treated in ccw (right-handed rule) and interior rings will be treated in cw (left-handed rule). This feature change supports polygons that cross the dateline, and those that span the globe/map. The unit tests have been updated and corrected to test various situations. Greater test coverage will be provided in future commits. Addresses #8672 --- .../geo/builders/BasePolygonBuilder.java | 23 ++++--- .../common/geo/builders/PointCollection.java | 1 + .../common/geo/builders/ShapeBuilder.java | 52 ++++++++++++-- .../common/geo/ShapeBuilderTests.java | 69 +++++++++++-------- 4 files changed, 100 insertions(+), 45 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 cb1e5b40558..ca1405253ed 100644 --- a/src/main/java/org/elasticsearch/common/geo/builders/BasePolygonBuilder.java +++ b/src/main/java/org/elasticsearch/common/geo/builders/BasePolygonBuilder.java @@ -125,10 +125,9 @@ public abstract class BasePolygonBuilder> extend Edge[] edges = new Edge[numEdges]; Edge[] holeComponents = new Edge[holes.size()]; - - int offset = createEdges(0, false, shell, edges, 0); + int offset = createEdges(0, false, shell, null, edges, 0); for (int i = 0; i < holes.size(); i++) { - int length = createEdges(i+1, true, this.holes.get(i), edges, offset); + int length = createEdges(i+1, true, shell, this.holes.get(i), edges, offset); holeComponents[i] = edges[offset]; offset += length; } @@ -396,16 +395,20 @@ public abstract class BasePolygonBuilder> extend holes[numHoles] = null; } // only connect edges if intersections are pairwise - // per the comment above, the edge array is sorted by y-value of the intersection + // 1. per the comment above, the edge array is sorted by y-value of the intersection // with the dateline. Two edges have the same y intercept when they cross the // dateline thus they appear sequentially (pairwise) in the edge array. Two edges // do not have the same y intercept when we're forming a multi-poly from a poly // that wraps the dateline (but there are 2 ordered intercepts). // The connect method creates a new edge for these paired edges in the linked list. // For boundary conditions (e.g., intersect but not crossing) there is no sibling edge - // to connect. Thus the following enforces the pairwise rule + // to connect. Thus the first logic check enforces the pairwise rule + // 2. the second logic ensures the two candidate edges aren't already connected by an + // existing along the dateline - this is necessary due to a logic change that + // computes dateline edges as valid intersect points in support of OGC standards if (e1.intersect != Edge.MAX_COORDINATE && e2.intersect != Edge.MAX_COORDINATE - && (e1.next.next.coordinate != e2.coordinate) ) { + && !(e1.next.next.coordinate.equals3D(e2.coordinate) && Math.abs(e1.next.coordinate.x) == DATELINE + && Math.abs(e2.coordinate.x) == DATELINE) ) { connect(e1, e2); } } @@ -449,9 +452,11 @@ public abstract class BasePolygonBuilder> extend } } - private static int createEdges(int component, boolean direction, BaseLineStringBuilder line, Edge[] edges, int offset) { - Coordinate[] points = line.coordinates(false); // last point is repeated - Edge.ring(component, direction, points, 0, edges, offset, points.length-1); + private static int createEdges(int component, boolean direction, BaseLineStringBuilder shell, BaseLineStringBuilder hole, + Edge[] edges, int offset) { + // set the points array accordingly (shell or hole) + Coordinate[] points = (hole != null) ? hole.coordinates(false) : shell.coordinates(false); + Edge.ring(component, direction, shell, points, 0, edges, offset, points.length-1); return points.length-1; } diff --git a/src/main/java/org/elasticsearch/common/geo/builders/PointCollection.java b/src/main/java/org/elasticsearch/common/geo/builders/PointCollection.java index 99e3981ceb5..8174efacfc6 100644 --- a/src/main/java/org/elasticsearch/common/geo/builders/PointCollection.java +++ b/src/main/java/org/elasticsearch/common/geo/builders/PointCollection.java @@ -34,6 +34,7 @@ import com.vividsolutions.jts.geom.Coordinate; public abstract class PointCollection> extends ShapeBuilder { protected final ArrayList points; + protected boolean translated = false; protected PointCollection() { this(new ArrayList()); 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 8de41603223..07281a5b279 100644 --- a/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java +++ b/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java @@ -25,6 +25,7 @@ import com.spatial4j.core.shape.jts.JtsGeometry; import com.vividsolutions.jts.geom.Coordinate; import com.vividsolutions.jts.geom.Geometry; import com.vividsolutions.jts.geom.GeometryFactory; +import org.apache.commons.lang3.tuple.Pair; import org.elasticsearch.ElasticsearchIllegalArgumentException; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.logging.ESLogger; @@ -405,6 +406,29 @@ public abstract class ShapeBuilder implements ToXContent { return top; } + private static final Pair range(Coordinate[] points, int offset, int length) { + double minX = points[0].x; + double maxX = points[0].x; + double minY = points[0].y; + double maxY = points[0].y; + // compute the bounding coordinates (@todo: cleanup brute force) + for (int i = 1; i < length; ++i) { + if (points[offset + i].x < minX) { + minX = points[offset + i].x; + } + if (points[offset + i].x > maxX) { + maxX = points[offset + i].x; + } + if (points[offset + i].y < minY) { + minY = points[offset + i].y; + } + if (points[offset + i].y > maxY) { + maxY = points[offset + i].y; + } + } + return Pair.of(Pair.of(minX, maxX), Pair.of(minY, maxY)); + } + /** * Concatenate a set of points to a polygon * @@ -461,8 +485,8 @@ public abstract class ShapeBuilder implements ToXContent { * number of points * @return Array of edges */ - protected static Edge[] ring(int component, boolean direction, Coordinate[] points, int offset, Edge[] edges, int toffset, - int length) { + protected static Edge[] ring(int component, boolean direction, BaseLineStringBuilder shell, Coordinate[] points, int offset, + Edge[] edges, int toffset, int length) { // calculate the direction of the points: // find the point a the top of the set and check its // neighbors orientation. So direction is equivalent @@ -474,12 +498,26 @@ public abstract class ShapeBuilder implements ToXContent { // 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 - // thus no need to compute orientation at runtime (which fails on ambiguous polys anyway - final int rngIdx = (orientation) ? next : prev; - if ( ((orientation && component == 0) || (!orientation && component < 0)) && - points[offset+top].x - points[offset+rngIdx].x > DATELINE) { + // thus if orientation is computed as cw, the logic will translate points across dateline + // and convert to a right handed system + + // compute the bounding box and calculate range + Pair range = range(points, offset, length); + final double rng = (Double)range.getLeft().getRight() - (Double)range.getLeft().getLeft(); + // translate the points if the following is true + // 1. range is greater than a hemisphere (180 degrees) but not spanning 2 hemispheres (translation would result in + // a collapsed poly) + // 2. the shell of the candidate hole has been translated (to preserve the coordinate system) + if ((rng > DATELINE && rng != 2*DATELINE && orientation) || (shell.translated && component != 0)) { transform(points); - orientation = !orientation; + // flip the translation bit if the shell is being translated + if (component == 0 && !shell.translated) { + shell.translated = true; + } + // correct the orientation post translation (ccw for shell, cw for holes) + if ((component == 0 && orientation) || (component != 0 && !orientation)) { + orientation = !orientation; + } } return concat(component, direction ^ orientation, points, offset, edges, toffset, length); } diff --git a/src/test/java/org/elasticsearch/common/geo/ShapeBuilderTests.java b/src/test/java/org/elasticsearch/common/geo/ShapeBuilderTests.java index 222666f937d..3203fac0c96 100644 --- a/src/test/java/org/elasticsearch/common/geo/ShapeBuilderTests.java +++ b/src/test/java/org/elasticsearch/common/geo/ShapeBuilderTests.java @@ -246,40 +246,40 @@ public class ShapeBuilderTests extends ElasticsearchTestCase { // a giant c shape PolygonBuilder builder = ShapeBuilder.newPolygon() - .point(-186,0) - .point(-176,0) - .point(-176,3) - .point(-183,3) - .point(-183,5) - .point(-176,5) - .point(-176,8) - .point(-186,8) - .point(-186,0); + .point(174,0) + .point(-176,0) + .point(-176,3) + .point(177,3) + .point(177,5) + .point(-176,5) + .point(-176,8) + .point(174,8) + .point(174,0); // 3/4 of an embedded 'c', crossing dateline once builder.hole() - .point(-185,1) - .point(-181,1) - .point(-181,2) - .point(-184,2) - .point(-184,6) - .point(-178,6) - .point(-178,7) - .point(-185,7) - .point(-185,1); + .point(175, 1) + .point(175, 7) + .point(-178, 7) + .point(-178, 6) + .point(176, 6) + .point(176, 2) + .point(179, 2) + .point(179,1) + .point(175, 1); // embedded hole right of the dateline builder.hole() - .point(-179,1) - .point(-177,1) - .point(-177,2) - .point(-179,2) - .point(-179,1); + .point(-179, 1) + .point(-179, 2) + .point(-177, 2) + .point(-177,1) + .point(-179,1); Shape shape = builder.close().build(); - assertMultiPolygon(shape); - } + assertMultiPolygon(shape); + } @Test public void testComplexShapeWithHole() { @@ -443,9 +443,9 @@ public class ShapeBuilderTests extends ElasticsearchTestCase { .point(-172,0); builder.hole() .point(-176, 10) - .point(-180, 5) - .point(-180, -5) .point(-176, -10) + .point(-180, -5) + .point(-180, 5) .point(-176, 10); shape = builder.close().build(); assertMultiPolygon(shape); @@ -468,7 +468,8 @@ public class ShapeBuilderTests extends ElasticsearchTestCase { } @Test - public void testShapeWithEdgeAcrossDateline() { + public void testShapeWithAlternateOrientation() { + // ccw: should produce a single polygon spanning hemispheres PolygonBuilder builder = ShapeBuilder.newPolygon() .point(180, 0) .point(176, 4) @@ -476,7 +477,17 @@ public class ShapeBuilderTests extends ElasticsearchTestCase { .point(180, 0); Shape shape = builder.close().build(); + assertPolygon(shape); - assertPolygon(shape); + // cw: geo core will convert to ccw across the dateline + builder = ShapeBuilder.newPolygon() + .point(180, 0) + .point(-176, 4) + .point(176, 4) + .point(180, 0); + + shape = builder.close().build(); + + assertMultiPolygon(shape); } }