Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the determinant formula for calculating the orientation of a polygon #27967

Merged
merged 9 commits into from
Jul 31, 2018
Original file line number Diff line number Diff line change
Expand Up @@ -642,14 +642,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
Expand Down Expand Up @@ -678,6 +672,30 @@ 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;

// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a couple questions about this assertion:

  • I think the determinant could be 0 if the three points lie in a straight line. Is my understanding correct that this won't happen because of the way we choose top? It might be nice to add a quick note to explain.
  • Are we concerned about numeric underflow here? I'm curious as to how we handle numeric issues in other parts of the geo code (in particular, whether we should throw a runtime exception when an issue is detected).

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++) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}