Improve behaviour of GeometryFixer for holes outside shell

modules/core/src/main/java/org/locationtech/jts/geom/util/GeometryFixer.java

@@ -26,6 +26,8 @@
 import org.locationtech.jts.geom.MultiPolygon;
 import org.locationtech.jts.geom.Point;
 import org.locationtech.jts.geom.Polygon;
+import org.locationtech.jts.geom.prep.PreparedGeometry;
+import org.locationtech.jts.geom.prep.PreparedGeometryFactory;
 import org.locationtech.jts.operation.buffer.BufferOp;
 import org.locationtech.jts.operation.overlayng.OverlayNG;
 import org.locationtech.jts.operation.overlayng.OverlayNGRobust;
@@ -45,18 +47,23 @@
  * <li>Empty atomic geometries are valid and are returned unchanged</li>
  * <li>Empty elements are removed from collections</li>
  * <li><code>Point</code>: keep valid coordinate, or EMPTY</li>
- * <li><code>LineString</code>: fix coordinate list</li>
- * <li><code>LinearRing</code>: fix coordinate list, return as valid ring or else <code>LineString</code></li>
+ * <li><code>LineString</code>: coordinates are fixed</li>
+ * <li><code>LinearRing</code>: coordinates are fixed.  Keep valid ring, or else convert into <code>LineString</code></li>
  * <li><code>Polygon</code>: transform into a valid polygon, 
- * preserving as much of the extent and vertices as possible</li>
- * <li><code>MultiPolygon</code>: fix each polygon, 
- * then ensure result is non-overlapping (via union)</li>
- * <li><code>GeometryCollection</code>: fix each element</li>
+ * preserving as much of the extent and vertices as possible.
+ * <ul>
+ *   <li>Rings are fixed to ensure they are valid</li>  
+ *   <li>Holes intersecting the shell are subtracted from the shell</li>
+ *   <li>Holes outside the shell are converted into polygons</li>
+ * </ul></li>
+ * <li><code>MultiPolygon</code>: each polygon is fixed, 
+ * then result made non-overlapping (via union)</li>
+ * <li><code>GeometryCollection</code>: each element is fixed</li>
  * <li>Collapsed lines and polygons are handled as follows,
  * depending on the <code>keepCollapsed</code> setting:
  * <ul>
- * <li><code>false</code>: (default) collapses are converted to empty geometries</li>
- * <li><code>true</code>: collapses are converted to a valid geometry of lower dimension</li>
+ *   <li><code>false</code>: (default) collapses are converted to empty geometries</li>
+ *   <li><code>true</code>: collapses are converted to a valid geometry of lower dimension</li>
  * </ul>
  * </li>
  * </ol>
@@ -263,25 +270,85 @@ private Geometry fixPolygonElement(Polygon geom) {
       if (isKeepCollapsed) {
         return fixLineString(shell);
       }
-      //-- if not allowing collapses then return empty polygon
+      //--- if not allowing collapses then return empty polygon
       return null;      
     }
-    // if no holes then done
+    //--- if no holes then done
     if (geom.getNumInteriorRing() == 0) {
       return fixShell;
     }
-    Geometry fixHoles = fixHoles(geom);
-    Geometry result = removeHoles(fixShell, fixHoles);
+
+    //--- fix holes, classify, and construct shell-true holes
+    List<Geometry> holesFixed = fixHoles(geom);
+    List<Geometry> holes = new ArrayList<Geometry>();
+    List<Geometry> shells = new ArrayList<Geometry>();
+    classifyHoles(fixShell, holesFixed, holes, shells);
+    Geometry polyWithHoles = difference(fixShell, holes);
+    if (shells.size() == 0) {
+      return polyWithHoles;
+    }
+
+    //--- if some holes converted to shells, union all shells
+    shells.add(polyWithHoles);
+    Geometry result = union(shells);
     return result;
   }
 
-  private Geometry removeHoles(Geometry shell, Geometry holes) {
-    if (holes == null) 
+  private List<Geometry> fixHoles(Polygon geom) {
+    List<Geometry> holes = new ArrayList<Geometry>();
+    for (int i = 0; i < geom.getNumInteriorRing(); i++) {
+      Geometry holeRep = fixRing(geom.getInteriorRingN(i));
+      if (holeRep != null) {
+        holes.add(holeRep);
+      }
+    }
+    return holes;
+  }
+
+  private void classifyHoles(Geometry shell, List<Geometry> holesFixed, List<Geometry> holes, List<Geometry> shells) {
+    PreparedGeometry shellPrep = PreparedGeometryFactory.prepare(shell);
+    for (Geometry hole : holesFixed) {
+      if (shellPrep.intersects(hole)) {
+        holes.add(hole);
+      }
+      else {
+        shells.add(hole);
+      }
+    }
+  }
+
+  /**
+   * Subtracts a list of polygonal geometries from a polygonal geometry.
+   * 
+   * @param shell polygonal geometry for shell
+   * @param holes polygonal geometries to subtract
+   * @return the result geometry
+   */
+  private Geometry difference(Geometry shell, List<Geometry> holes) {
+    if (holes == null || holes.size() == 0) 
       return shell;
-    return OverlayNGRobust.overlay(shell, holes, OverlayNG.DIFFERENCE);
+    Geometry holesUnion = union(holes);
+    return OverlayNGRobust.overlay(shell, holesUnion, OverlayNG.DIFFERENCE);
+  }
+
+  /**
+   * Unions a list of polygonal geometries.
+   * Optimizes case of zero or one input geometries.
+   * Requires that the inputs are net new objects.
+   * 
+   * @param polys the polygonal geometries to union
+   * @return the union of the inputs
+   */
+  private Geometry union(List<Geometry> polys) {
+    if (polys.size() == 0) return factory.createPolygon();
+    if (polys.size() == 1) {
+      return polys.get(0);
+    }
+    // TODO: replace with holes.union() once OverlayNG is the default
+    return OverlayNGRobust.union(polys);
   }
 
-  private Geometry fixHoles(Polygon geom) {
+  private Geometry OLDfixHoles(Polygon geom) {
     List<Geometry> holes = new ArrayList<Geometry>();
     for (int i = 0; i < geom.getNumInteriorRing(); i++) {
       Geometry holeRep = fixRing(geom.getInteriorRingN(i));
@@ -299,9 +366,9 @@ private Geometry fixHoles(Polygon geom) {
   }
 
   private Geometry fixRing(LinearRing ring) {
-    //-- always execute fix, since it may remove repeated coords etc
+    //-- always execute fix, since it may remove repeated/invalid coords etc
+    // TODO: would it be faster to check ring validity first?
     Geometry poly = factory.createPolygon(ring);
-    // TOD: check if buffer removes invalid coordinates
     return BufferOp.bufferByZero(poly, true);
   }
 
@@ -318,7 +385,7 @@ private Geometry fixMultiPolygon(MultiPolygon geom) {
       return factory.createMultiPolygon();
     }
     // TODO: replace with polys.union() once OverlayNG is the default
-    Geometry result = OverlayNGRobust.union(polys);
+    Geometry result = union(polys);
     return result;    
   }
 

modules/core/src/test/java/org/locationtech/jts/geom/util/GeometryFixerTest.java

@@ -253,6 +253,11 @@ public void testPolygonHoleKeepCollapse() {
         "POLYGON ((10 10, 10 90, 90 90, 90 10, 10 10))");
   }
 
+  public void testPolygonHoleOverlapAndOutsideOverlap() {
+    checkFix("POLYGON ((50 90, 80 90, 80 10, 50 10, 50 90), (70 80, 90 80, 90 20, 70 20, 70 80), (40 80, 40 50, 0 50, 0 80, 40 80), (30 40, 10 40, 10 60, 30 60, 30 40), (60 70, 80 70, 80 30, 60 30, 60 70))",
+        "MULTIPOLYGON (((10 40, 10 50, 0 50, 0 80, 40 80, 40 50, 30 50, 30 40, 10 40)), ((70 80, 70 70, 60 70, 60 30, 70 30, 70 20, 80 20, 80 10, 50 10, 50 90, 80 90, 80 80, 70 80)))");
+  }
+
   //----------------------------------------
 
   public void testMultiPolygonEmpty() {

modules/core/src/test/java/test/jts/perf/geom/util/GeometryFixerFuzzer.java

@@ -15,11 +15,15 @@
 import org.locationtech.jts.geom.Geometry;
 import org.locationtech.jts.geom.GeometryFactory;
 import org.locationtech.jts.geom.LinearRing;
+import org.locationtech.jts.geom.Point;
+import org.locationtech.jts.geom.Polygon;
 import org.locationtech.jts.geom.util.GeometryFixer;
 
 public class GeometryFixerFuzzer {
 
+  private static final int GEOM_EXTENT_SIZE = 100;
   private static final int NUM_ITER = 10000;
+  private static final boolean IS_VERBOSE = false;
 
   public static void main(String[] args) {
     GeometryFixerFuzzer.run();
@@ -37,33 +41,38 @@ public GeometryFixerFuzzer() {
   }
 
   private void run(int numIter) {
+    System.out.println("GeometryFixer fuzzer: iterations = " + numIter);
     for (int i = 0; i < numIter; i++) {
       int numHoles = (int) (10 * Math.random());
-      Geometry invalidPoly = createRandomPoly(100, numHoles);
+      //Geometry invalidPoly = createRandomLinePoly(100, numHoles);
+      Geometry invalidPoly = createRandomCirclePoly(100, numHoles);
       Geometry result = GeometryFixer.fix(invalidPoly);
       boolean isValid = result.isValid();
-      String status =  isValid ? "valid" : "INVALID";
-      String msg = String.format("%d: Pts - input %d, output %d - %s",
-          i, invalidPoly.getNumPoints(), result.getNumPoints(), status);
-      //System.out.println(invalidPoly);
-      if (! isValid) {
-        System.out.println(msg);
-        System.out.println(invalidPoly);
-      }
+      report(i, invalidPoly, result, isValid);
     }
   }
 
-  private Geometry createRandomPoly(int numPoints, int numHoles) {
+  private void report(int i, Geometry invalidPoly, Geometry result, boolean isValid) {
+    String status =  isValid ? "valid" : "INVALID";
+    String msg = String.format("%d: Pts - input %d, output %d - %s",
+        i, invalidPoly.getNumPoints(), result.getNumPoints(), status);
+    if (IS_VERBOSE || ! isValid) {
+      System.out.println(msg);
+      System.out.println(invalidPoly);
+    }
+  }
+
+  private Geometry createRandomLinePoly(int numPoints, int numHoles) {
     int numRingPoints = numPoints / (numHoles + 1);
-    LinearRing shell = createRandomRing(numRingPoints);
+    LinearRing shell = createRandomLineRing(numRingPoints);
     LinearRing[] holes = new LinearRing[numHoles];
     for (int i = 0; i < numHoles; i++) {
-      holes[i] = createRandomRing(numRingPoints);
+      holes[i] = createRandomLineRing(numRingPoints);
     }
     return factory.createPolygon(shell, holes);
   }
 
-  private LinearRing createRandomRing(int numPoints) {
+  private LinearRing createRandomLineRing(int numPoints) {
     return factory.createLinearRing(createRandomPoints(numPoints));
   }
 
@@ -78,7 +87,28 @@ private Coordinate[] createRandomPoints(int numPoints) {
   }
 
   private double randOrd() {
-    double ord = 100 * Math.random();
+    double ord = GEOM_EXTENT_SIZE * Math.random();
     return ord;
   }
+
+  private Geometry createRandomCirclePoly(int numPoints, int numHoles) {
+    int numRingPoints = numPoints / (numHoles + 1);
+    LinearRing shell = ceateRandomCircleRing(numRingPoints);
+    LinearRing[] holes = new LinearRing[numHoles];
+    for (int i = 0; i < numHoles; i++) {
+      holes[i] = ceateRandomCircleRing(numRingPoints);
+    }
+    return factory.createPolygon(shell, holes);
+  }
+
+  private LinearRing ceateRandomCircleRing(int numPoints) {
+    int numQuadSegs = (numPoints / 4) + 1;
+    if (numQuadSegs < 3) numQuadSegs = 3;
+
+    Coordinate p = new Coordinate(randOrd(), randOrd());
+    Point pt = factory.createPoint( p );
+    double radius = GEOM_EXTENT_SIZE * Math.random() / 2;
+    Polygon buffer = (Polygon) pt.buffer(radius, numQuadSegs);
+    return buffer.getExteriorRing();
+  }
 }