LUCENE-7225: Detect nonsense polygon as an error rather than creating failures.

lucene/spatial3d/src/java/org/apache/lucene/spatial3d/Geo3DPoint.java

@@ -192,6 +192,7 @@ public final class Geo3DPoint extends Field {
     for (final Polygon hole : theHoles) {
       holeList.add(fromPolygon(hole, !reverseMe));
     }
+    
     // Now do the polygon itself
     final double[] polyLats = polygon.getPolyLats();
     final double[] polyLons = polygon.getPolyLons();

lucene/spatial3d/src/java/org/apache/lucene/spatial3d/geom/GeoPolygonFactory.java

@@ -62,6 +62,7 @@ public class GeoPolygonFactory {
   public static GeoPolygon makeGeoPolygon(final PlanetModel planetModel,
     final List<GeoPoint> pointList,
     final List<GeoPolygon> holes) {
+    //System.err.println("points="+pointList);
     // Create a random number generator.  Effectively this furnishes us with a repeatable sequence
     // of points to use for poles.
     final Random generator = new Random(1234);
@@ -105,17 +106,20 @@ public class GeoPolygonFactory {
     // We don't know if this is the correct siding choice.  We will only know as we build the complex polygon.
     // So we need to be prepared to try both possibilities.
     GeoCompositePolygon rval = new GeoCompositePolygon();
-    if (buildPolygonShape(rval, planetModel, filteredPointList, new BitSet(), 0, 1, initialPlane, holes, testPoint) == false) {
+    MutableBoolean seenConcave = new MutableBoolean();
+    if (buildPolygonShape(rval, seenConcave, planetModel, filteredPointList, new BitSet(), 0, 1, initialPlane, holes, testPoint) == false) {
       // The testPoint was within the shape.  Was that intended?
       if (testPointInside) {
         // Yes: build it for real
         rval = new GeoCompositePolygon();
-        buildPolygonShape(rval, planetModel, filteredPointList, new BitSet(), 0, 1, initialPlane, holes, null);
+        seenConcave = new MutableBoolean();
+        buildPolygonShape(rval, seenConcave, planetModel, filteredPointList, new BitSet(), 0, 1, initialPlane, holes, null);
         return rval;
       }
       // No: do the complement and return that.
       rval = new GeoCompositePolygon();
-      buildPolygonShape(rval, planetModel, filteredPointList, new BitSet(), 0, 1, new SidedPlane(initialPlane), holes, null);
+      seenConcave = new MutableBoolean();
+      buildPolygonShape(rval, seenConcave, planetModel, filteredPointList, new BitSet(), 0, 1, new SidedPlane(initialPlane), holes, null);
       return rval;
     } else {
       // The testPoint was outside the shape.  Was that intended?
@@ -125,7 +129,8 @@ public class GeoPolygonFactory {
       }
       // No: return the complement
       rval = new GeoCompositePolygon();
-      buildPolygonShape(rval, planetModel, filteredPointList, new BitSet(), 0, 1, new SidedPlane(initialPlane), holes, null);
+      seenConcave = new MutableBoolean();
+      buildPolygonShape(rval, seenConcave, planetModel, filteredPointList, new BitSet(), 0, 1, new SidedPlane(initialPlane), holes, null);
       return rval;
     }
   }
@@ -400,6 +405,7 @@ public class GeoPolygonFactory {
 
   /** Build a GeoPolygon out of one concave part and multiple convex parts given points, starting edge, and whether starting edge is internal or not.
    * @param rval is the composite polygon to add to.
+   * @param seenConcave is true if a concave polygon has been seen in this generation yet.
    * @param planetModel is the planet model.
    * @param pointsList is a list of the GeoPoints to build an arbitrary polygon out of.
    * @param internalEdges specifies which edges are internal.
@@ -419,8 +425,9 @@ public class GeoPolygonFactory {
    *  found in the interior of the shape we create here we return false, which is a signal that we chose
    *  our initial plane sidedness backwards.
    */
-  public static boolean buildPolygonShape(
+  static boolean buildPolygonShape(
     final GeoCompositePolygon rval,
+    final MutableBoolean seenConcave,
     final PlanetModel planetModel,
     final List<GeoPoint> pointsList,
     final BitSet internalEdges,
@@ -433,7 +440,8 @@ public class GeoPolygonFactory {
     // It could be the case that we need a concave polygon.  So we need to try and look for that case
     // as part of the general code for constructing complex polygons.
 
-    // Note that there can be only one concave polygon.
+    // Note that there can be only one concave polygon.  This code will enforce that condition and will return
+    // false if it is violated.
               
     // The code here must keep track of two lists of sided planes.  The first list contains the planes consistent with
     // a concave polygon.  This list will grow and shrink.  The second list is built starting at the current edge that
@@ -585,29 +593,17 @@ public class GeoPolygonFactory {
           // This should be the only problematic part of the polygon.
           // We know that thePoint is on the "wrong" side of the edge -- that is, it's on the side that the
           // edge is pointing at.
-          final List<GeoPoint> thirdPartPoints = new ArrayList<>();
+          final List<GeoPoint> thirdPartPoints = new ArrayList<>(3);
           final BitSet thirdPartInternal = new BitSet();
           thirdPartPoints.add(checkEdge.startPoint);
           thirdPartInternal.set(0, checkEdge.isInternal);
           thirdPartPoints.add(checkEdge.endPoint);
           thirdPartInternal.set(1, true);
           thirdPartPoints.add(thePoint);
-          thirdPartInternal.set(2, true);
+          assert checkEdge.plane.isWithin(thePoint) : "Point was on wrong side of complementary plane, so must be on the right side of the non-complementary plane!";
-          //System.out.println("Doing convex part...");
+          final GeoPolygon convexPart = new GeoConvexPolygon(planetModel, thirdPartPoints, holes, thirdPartInternal, true);
-          if (buildPolygonShape(rval,
+          //System.out.println("convex part = "+convexPart);
-            planetModel,
+          rval.addShape(convexPart);
-            thirdPartPoints,
-            thirdPartInternal, 
-            0,
-            1,
-            checkEdge.plane,
-            holes,
-            testPoint) == false) {
-            return false;
-          }
-          //System.out.println("...done convex part.");
-
-          // ??? check if we get the sense right
 
           // The part preceding the bad edge, back to thePoint, needs to be recursively
           // processed.  So, assemble what we need, which is basically a list of edges.
@@ -626,6 +622,7 @@ public class GeoPolygonFactory {
           firstPartInternal.set(i, true);
           //System.out.println("Doing first part...");
           if (buildPolygonShape(rval,
+            seenConcave,
             planetModel,
             firstPartPoints,
             firstPartInternal, 
@@ -653,6 +650,7 @@ public class GeoPolygonFactory {
           secondPartInternal.set(i, true);
           //System.out.println("Doing second part...");
           if (buildPolygonShape(rval,
+            seenConcave,
             planetModel,
             secondPartPoints,
             secondPartInternal, 
@@ -673,7 +671,8 @@ public class GeoPolygonFactory {
     // No violations found: we know it's a legal concave polygon.
     
     // If there's anything left in the edge buffer, convert to concave polygon.
-    if (makeConcavePolygon(planetModel, rval, edgeBuffer, holes, testPoint) == false) {
+    //System.out.println("adding concave part");
+    if (makeConcavePolygon(planetModel, rval, seenConcave, edgeBuffer, holes, testPoint) == false) {
       return false;
     }
     
@@ -684,6 +683,7 @@ public class GeoPolygonFactory {
    * By this point, if there are any edges in the edgebuffer, they represent a concave polygon.
    * @param planetModel is the planet model.
    * @param rval is the composite polygon we're building.
+   * @param seenConcave is true if we've already seen a concave polygon.
    * @param edgeBuffer is the edge buffer.
    * @param holes is the optional list of holes.
    * @param testPoint is the optional test point.
@@ -691,13 +691,21 @@ public class GeoPolygonFactory {
    */
   private static boolean makeConcavePolygon(final PlanetModel planetModel,
     final GeoCompositePolygon rval,
+    final MutableBoolean seenConcave,
     final EdgeBuffer edgeBuffer,
     final List<GeoPolygon> holes,
     final GeoPoint testPoint) {
+      
     if (edgeBuffer.size() == 0) {
       return true;
     }
     
+    if (seenConcave.value) {
+      throw new IllegalArgumentException("Illegal polygon; polygon edges intersect each other");
+    }
+
+    seenConcave.value = true;
+    
     // If there are less than three edges, something got messed up somehow.  Don't know how this
     // can happen but check.
     if (edgeBuffer.size() < 3) {
@@ -1107,24 +1115,34 @@ public class GeoPolygonFactory {
       */
     public EdgeBuffer(final List<GeoPoint> pointList, final BitSet internalEdges, final int startPlaneStartIndex, final int startPlaneEndIndex, final SidedPlane startPlane) {
       /*
-      System.out.println("Initial points:");
+      System.out.println("Start plane index: "+startPlaneStartIndex+" End plane index: "+startPlaneEndIndex+" Initial points:");
       for (final GeoPoint p : pointList) {
         System.out.println(" "+p);
       }
       */
       
-      // We need to detect backtracks, and also situations where someone has tried to stitch together multiple segments into one long arc (> 180 degrees).
-      // To do this, every time we extend by a coplanar segment, we compute the total arc distance to the new endpoint, as
-      // well as a sum of the arc distances we've accumulated as we march forward.  If these two numbers disagree, then
-      // we know there has been a backtrack or other anomaly.
-      
-      // extend the edge, we compute the distance along the 
       final Edge startEdge = new Edge(pointList.get(startPlaneStartIndex), pointList.get(startPlaneEndIndex), startPlane, internalEdges.get(startPlaneStartIndex));
       // Fill in the EdgeBuffer by walking around creating more stuff
       Edge currentEdge = startEdge;
       int startIndex = startPlaneStartIndex;
       int endIndex = startPlaneEndIndex;
       while (true) {
+        /*
+        System.out.println("For plane "+currentEdge.plane+", the following points are in/out:");
+        for (final GeoPoint p: pointList) {
+          System.out.println(" "+p+" is: "+(currentEdge.plane.isWithin(p)?"in":"out"));
+        }
+        */
+        
+        // Check termination condition
+        if (currentEdge.endPoint == startEdge.startPoint) {
+          // We finish here.  Link the current edge to the start edge, and exit
+          previousEdges.put(startEdge, currentEdge);
+          nextEdges.put(currentEdge, startEdge);
+          edges.add(startEdge);
+          break;
+        }
+
         // Compute the next edge
         startIndex = endIndex;
         endIndex++;
@@ -1134,53 +1152,15 @@ public class GeoPolygonFactory {
         // Get the next point
         final GeoPoint newPoint = pointList.get(endIndex);
         // Build the new edge
-        // We have to be sure that the point we use as a check does not lie on the plane.
-        // In order to meet that goal, we need to go hunting for a point that meets the criteria.  If we don't
-        // find one, we've got a linear "polygon" that we cannot use.
-        
         // We need to know the sidedness of the new plane.  The point we're going to be presenting to it has
         // a certain relationship with the sided plane we already have for the current edge.  If the current edge
         // is colinear with the new edge, then we want to maintain the same relationship.  If the new edge
         // is not colinear, then we can use the new point's relationship with the current edge as our guide.
         
-        final boolean isNewPointWithin;
+        final boolean isNewPointWithin = currentEdge.plane.isWithin(newPoint);
-        final GeoPoint pointToPresent;
+        final GeoPoint pointToPresent = currentEdge.startPoint;
-        if (currentEdge.plane.evaluateIsZero(newPoint)) {
-          // The new point is colinear with the current edge.  We'll have to look backwards for the first point that isn't.
-          int checkPointIndex = -1;
-          // Compute the arc distance before we try to extend, so that we note backtracking when we see it
-          //double accumulatedDistance = newPoint.arcDistance(pointList.get(startIndex));
-          final Plane checkPlane = new Plane(pointList.get(startIndex), newPoint);
-          for (int i = 0; i < pointList.size(); i++) {
-            final int index = getLegalIndex(startIndex - 1 - i, pointList.size());
-            if (!checkPlane.evaluateIsZero(pointList.get(index))) {
-              checkPointIndex = index;
-              break;
-            } else {
-              //accumulatedDistance += pointList.get(getLegalIndex(index+1, pointList.size())).arcDistance(pointList.get(index));
-              //final double actualDistance = newPoint.arcDistance(pointList.get(index));
-              //if (Math.abs(actualDistance - accumulatedDistance) >= Vector.MINIMUM_RESOLUTION) {
-              //  throw new IllegalArgumentException("polygon backtracks over itself");
-              //}
-            }
-          }
-          if (checkPointIndex == -1) {
-            throw new IllegalArgumentException("polygon is illegal (linear)");
-          }
-          pointToPresent = pointList.get(checkPointIndex);
-          isNewPointWithin = currentEdge.plane.isWithin(pointToPresent);
-        } else {
-          isNewPointWithin = currentEdge.plane.isWithin(newPoint);
-          pointToPresent = currentEdge.startPoint;
-        }
 
         final SidedPlane newPlane = new SidedPlane(pointToPresent, isNewPointWithin, pointList.get(startIndex), newPoint);
-        /*
-        System.out.println("For next plane, the following points are in/out:");
-        for (final GeoPoint p: pointList) {
-          System.out.println(" "+p+" is: "+(newPlane.isWithin(p)?"in":"out"));
-        }
-        */
         final Edge newEdge = new Edge(pointList.get(startIndex), pointList.get(endIndex), newPlane, internalEdges.get(startIndex));
         
         // Link it in
@@ -1189,13 +1169,6 @@ public class GeoPolygonFactory {
         edges.add(newEdge);
         currentEdge = newEdge;
 
-        if (currentEdge.endPoint == startEdge.startPoint) {
-          // We finish here.  Link the current edge to the start edge, and exit
-          previousEdges.put(startEdge, currentEdge);
-          nextEdges.put(currentEdge, startEdge);
-          edges.add(startEdge);
-          break;
-        }
       }
       
       oneEdge = startEdge;
@@ -1313,4 +1286,8 @@ public class GeoPolygonFactory {
     
   }
   
+  static class MutableBoolean {
+    public boolean value = false;
+  }
+  
 }

lucene/spatial3d/src/test/org/apache/lucene/spatial3d/TestGeo3DPoint.java

@@ -544,10 +544,13 @@ public class TestGeo3DPoint extends LuceneTestCase {
         // Polygons
         final boolean isClockwise = random().nextDouble() < 0.5;
         try {
-          return Geo3DPoint.newPolygonQuery(field, makePoly(PlanetModel.WGS84,
+          final Query q = Geo3DPoint.newPolygonQuery(field, makePoly(PlanetModel.WGS84,
             new GeoPoint(PlanetModel.WGS84, toRadians(GeoTestUtil.nextLatitude()), toRadians(GeoTestUtil.nextLongitude())),
             isClockwise,
             true));
+          //System.err.println("Generated: "+q);
+          //assertTrue(false);
+          return q;
         } catch (IllegalArgumentException e) {
           continue;
         }
@@ -880,6 +883,7 @@ public class TestGeo3DPoint extends LuceneTestCase {
     * clockwise/counterclockwise rotation that way.
     */
   protected static Polygon makePoly(final PlanetModel pm, final GeoPoint pole, final boolean clockwiseDesired, final boolean createHoles) {
+    
     // Polygon edges will be arranged around the provided pole, and holes will each have a pole selected within the parent
     // polygon.
     final int pointCount = TestUtil.nextInt(random(), 3, 10);

lucene/spatial3d/src/test/org/apache/lucene/spatial3d/geom/GeoPolygonTest.java

@@ -439,7 +439,76 @@ shape:
     points.add(new GeoPoint(-0.1699323603224724, 0.8516746480592872, 0.4963385521664198));
     points.add(new GeoPoint(0.2654788898359613, 0.7380222309164597, 0.6200740473100581));
 
+    boolean illegalArgumentException = false;
+    try {
       final GeoPolygon p = GeoPolygonFactory.makeGeoPolygon(PlanetModel.WGS84, points, null);
+    } catch (IllegalArgumentException e) {
+      illegalArgumentException = true;
+    }
+    assertTrue(illegalArgumentException);
+  }
+
+  @Test
+  public void testPolygonFactoryCase2() {
+    /*
+   [[lat=-0.48522750470337056, lon=-1.7370471071224087([X=-0.14644023172524287, Y=-0.8727091042681705, Z=-0.4665895520487907])], 
+   [lat=-0.4252164254406539, lon=-1.0929282311747601([X=0.41916238097763436, Y=-0.8093435958043177, Z=-0.4127428785664968])], 
+   [lat=0.2055150822737076, lon=0.8094775925193464([X=0.6760197133035871, Y=0.7093859395658346, Z=0.20427109186920892])], 
+   [lat=-0.504360159046884, lon=-1.27628468850318([X=0.25421329462858633, Y=-0.8380671569889917, Z=-0.4834077932502288])], 
+   [lat=-0.11994023948700858, lon=0.07857194136150605([X=0.9908123546871113, Y=0.07801065055912473, Z=-0.11978097184039621])], 
+   [lat=0.39346633764155237, lon=1.306697331415816([X=0.24124272064589647, Y=0.8921189226448045, Z=0.3836311592666308])], 
+   [lat=-0.07741593942416389, lon=0.5334693210962216([X=0.8594122640512101, Y=0.50755758923985, Z=-0.07742360418968308])], 
+   [lat=0.4654236264787552, lon=1.3013260557429494([X=0.2380080413677112, Y=0.8617612419312584, Z=0.4489988990508502])], 
+   [lat=-1.2964641581620537, lon=-1.487600369139357([X=0.022467282495493006, Y=-0.26942922375508405, Z=-0.960688317984634])]]
+    */
+    final List<GeoPoint> points = new ArrayList<>();
+    points.add(new GeoPoint(PlanetModel.WGS84, -0.48522750470337056, -1.7370471071224087));
+    points.add(new GeoPoint(PlanetModel.WGS84, -0.4252164254406539, -1.0929282311747601));
+    points.add(new GeoPoint(PlanetModel.WGS84, 0.2055150822737076, 0.8094775925193464));
+    points.add(new GeoPoint(PlanetModel.WGS84, -0.504360159046884, -1.27628468850318));
+    points.add(new GeoPoint(PlanetModel.WGS84, -0.11994023948700858, 0.07857194136150605));
+    points.add(new GeoPoint(PlanetModel.WGS84, 0.39346633764155237, 1.306697331415816));
+    points.add(new GeoPoint(PlanetModel.WGS84, -0.07741593942416389, 0.5334693210962216));
+    points.add(new GeoPoint(PlanetModel.WGS84, 0.4654236264787552, 1.3013260557429494));
+    points.add(new GeoPoint(PlanetModel.WGS84, -1.2964641581620537, -1.487600369139357));
+    
+    boolean illegalArgumentException = false;
+    try {
+      final GeoPolygon p = GeoPolygonFactory.makeGeoPolygon(PlanetModel.WGS84, points, null);
+    } catch (IllegalArgumentException e) {
+      illegalArgumentException = true;
+    }
+    assertTrue(illegalArgumentException);
+  }
+  
+  @Test
+  public void testPolygonFactoryCase3() {
+    /*
+    This one failed to be detected as convex:
+
+   [junit4]   1> convex part = GeoConvexPolygon: {planetmodel=PlanetModel.WGS84, points=
+   [[lat=0.39346633764155237, lon=1.306697331415816([X=0.24124272064589647, Y=0.8921189226448045, Z=0.3836311592666308])], 
+   [lat=-0.4252164254406539, lon=-1.0929282311747601([X=0.41916238097763436, Y=-0.8093435958043177, Z=-0.4127428785664968])], 
+   [lat=0.4654236264787552, lon=1.3013260557429494([X=0.2380080413677112, Y=0.8617612419312584, Z=0.4489988990508502])]], internalEdges={0, 1, 2}}
+    */
+    final GeoPoint p3 = new GeoPoint(PlanetModel.WGS84, 0.39346633764155237, 1.306697331415816);
+    final GeoPoint p2 = new GeoPoint(PlanetModel.WGS84, -0.4252164254406539, -1.0929282311747601);
+    final GeoPoint p1 = new GeoPoint(PlanetModel.WGS84, 0.4654236264787552, 1.3013260557429494);
+    
+    final List<GeoPoint> points = new ArrayList<>();
+    points.add(p3);
+    points.add(p2);
+    points.add(p1);
+
+    final BitSet internal = new BitSet();
+    final GeoCompositePolygon rval = new GeoCompositePolygon();
+    final GeoPolygonFactory.MutableBoolean mutableBoolean = new GeoPolygonFactory.MutableBoolean();
+    
+    boolean result = GeoPolygonFactory.buildPolygonShape(rval, mutableBoolean, PlanetModel.WGS84, points, internal, 0, 1,
+      new SidedPlane(p1, p3, p2), new ArrayList<GeoPolygon>(), null);
+    
+    assertFalse(mutableBoolean.value);
+    
   }
   
 }