Fix IsValidOp for repeated node points

modules/core/src/main/java/org/locationtech/jts/operation/valid/IndexedNestedHoleTester.java

@@ -88,15 +88,9 @@ public boolean isNested()
         if (! testHole.getEnvelopeInternal().covers( hole.getEnvelopeInternal()) )
           continue;
 
-        /**
-         * Checks nesting via a point-in-polygon test, 
-         * or if the point lies on the boundary via 
-         * the topology of the incident edges.
-         */
-        Coordinate holePt0 = hole.getCoordinateN(0);
-        Coordinate holePt1 = hole.getCoordinateN(1);
-        if (PolygonTopologyAnalyzer.isSegmentInRing(holePt0, holePt1, testHole)) {
-          nestedPt = holePt0;
+        if (PolygonTopologyAnalyzer.isRingNested(hole, testHole)) {
+          //TODO: find a hole point known to be inside
+          nestedPt = hole.getCoordinateN(0);
           return true;  
         }
       }

modules/core/src/main/java/org/locationtech/jts/operation/valid/IndexedNestedPolygonTester.java

@@ -25,11 +25,12 @@
 
 /**
  * Tests whether a MultiPolygon has any element polygon
- * nested inside another polygon, using a spatial
+ * improperly nested inside another polygon, using a spatial
  * index to speed up the comparisons.
  * <p>
- * The logic assumes that the polygons do not overlap and have no collinear segments
- * (so they are properly nested, and there are no duplicate rings).
+ * The logic assumes that the polygons do not overlap and have no collinear segments.
+ * So the polygon rings may touch at discrete points,
+ * but they are properly nested, and there are no duplicate rings.
  */
 class IndexedNestedPolygonTester
 {
@@ -75,7 +76,7 @@ private IndexedPointInAreaLocator getLocator(int polyIndex) {
   public Coordinate getNestedPoint() { return nestedPt; }
 
   /**
-   * Tests if any polygon is nested (contained) within another polygon.
+   * Tests if any polygon is improperly nested (contained) within another polygon.
    * This is invalid.
    * The nested point will be set to reflect this.
    * @return true if some polygon is nested
@@ -106,6 +107,14 @@ public boolean isNested()
     return false;
   }
 
+  /**
+   * Finds an improperly nested point, if one exists.
+   * 
+   * @param shell the test polygon shell
+   * @param possibleOuterPoly a polygon which may contain it
+   * @param locator the locator for the outer polygon
+   * @return a nested point, if one exists, or null
+   */
   private Coordinate findNestedPoint(LinearRing shell, 
       Polygon possibleOuterPoly, IndexedPointInAreaLocator locator) 
   {    
@@ -119,7 +128,7 @@ private Coordinate findNestedPoint(LinearRing shell,
       return shellPt0;           
     }
 
-    Coordinate shellPt1 = shell.getCoordinateN(0);
+    Coordinate shellPt1 = shell.getCoordinateN(1);
     int loc1 = locator.locate(shellPt1);
     if (loc1 == Location.EXTERIOR) return null;
     if (loc1 == Location.INTERIOR) {
@@ -131,27 +140,24 @@ private Coordinate findNestedPoint(LinearRing shell,
      * the polygon.
      * Nesting can be checked via the topology of the incident edges.
      */
-    return findSegmentInPolygon(shell, possibleOuterPoly);
+    return findIncidentSegmentNestedPoint(shell, possibleOuterPoly);
   }
 
   /**
    * Finds a point of a shell segment which lies inside a polygon, if any.
-   * The shell is assume to touch the polyon only at shell vertices, 
+   * The shell is assumed to touch the polygon only at shell vertices, 
    * and does not cross the polygon.
    * 
    * @param shell the shell to test
    * @param poly the polygon to test against
    * @return an interior segment point, or null if the shell is nested correctly
    */
-  private static Coordinate findSegmentInPolygon(LinearRing shell, Polygon poly)
+  private static Coordinate findIncidentSegmentNestedPoint(LinearRing shell, Polygon poly)
   {
     LinearRing polyShell = poly.getExteriorRing();
     if (polyShell.isEmpty()) return null;
 
-    Coordinate shell0 = shell.getCoordinateN(0);
-    Coordinate shell1 = shell.getCoordinateN(1);
-
-    if (! PolygonTopologyAnalyzer.isSegmentInRing(shell0, shell1, polyShell))
+    if (! PolygonTopologyAnalyzer.isRingNested(shell, polyShell))
       return null;
 
     /**
@@ -161,7 +167,7 @@ private static Coordinate findSegmentInPolygon(LinearRing shell, Polygon poly)
     for (int i = 0; i < poly.getNumInteriorRing(); i++) {
       LinearRing hole = poly.getInteriorRingN(i);
       if (hole.getEnvelopeInternal().covers(shell.getEnvelopeInternal())
-          && PolygonTopologyAnalyzer.isSegmentInRing(shell0, shell1, hole)) {
+          && PolygonTopologyAnalyzer.isRingNested(shell, hole)) {
         return null;
       }
     }
@@ -170,6 +176,6 @@ private static Coordinate findSegmentInPolygon(LinearRing shell, Polygon poly)
      * The shell is contained in the polygon, but is not contained in a hole.
      * This is invalid.
      */
-    return shell0;
+    return shell.getCoordinateN(0);
   } 
 }

modules/core/src/main/java/org/locationtech/jts/operation/valid/IsValidOp.java

@@ -471,15 +471,16 @@ private void checkHolesOutsideShell(Polygon poly)
    */
   private Coordinate findHoleOutsideShellPoint(LinearRing hole, LinearRing shell) {
     Coordinate holePt0 = hole.getCoordinateN(0);
-    Coordinate holePt1 = hole.getCoordinateN(1);
     /**
      * If hole envelope is not covered by shell, it must be outside
      */
     if (! shell.getEnvelopeInternal().covers( hole.getEnvelopeInternal() ))
-        return holePt0;
+      //TODO: find hole pt outside shell env
+      return holePt0;
 
-    if (PolygonTopologyAnalyzer.isSegmentInRing(holePt0, holePt1, shell))
-      return null;    
+    if (PolygonTopologyAnalyzer.isRingNested(hole, shell))
+      return null;  
+    //TODO: find hole point outside shell
     return holePt0;
   }
 

modules/core/src/main/java/org/locationtech/jts/operation/valid/PolygonTopologyAnalyzer.java

@@ -43,45 +43,47 @@
 class PolygonTopologyAnalyzer {
 
   /**
-   * Finds a self-intersection (if any) in a {@link LinearRing}.
-   * 
-   * @param ring the ring to analyze
-   * @return a self-intersection point if one exists, or null
-   */
-  public static Coordinate findSelfIntersection(LinearRing ring) {
-    PolygonTopologyAnalyzer ata = new PolygonTopologyAnalyzer(ring, false);
-    if (ata.hasInvalidIntersection())
-      return ata.getInvalidLocation();
-    return null;
-  }
-
-  /**
-   * Tests whether a segment p0-p1 is inside or outside a ring.
+   * Tests whether a ring is nested inside another ring.
    * <p>
    * Preconditions:
    * <ul>
-   * <li>The segment intersects the ring only at the endpoints
-   * <li>One, none or both of the segment endpoints may lie on the ring
-   * <li>The ring does not self-cross, but it may self-touch
+   * <li>The rings do not cross (i.e. the test is wholly inside or outside the target)
+   * <li>The rings may touch at discrete points only
+   * <li>The target ring does not self-cross, but it may self-touch
    * </ul>  
+   * If the test ring start point is properly inside or outside, that provides the result.
+   * Otherwise the start point is on the target ring, 
+   * and the incident start segment (accounting for repeated points) is
+   * tested for its topology relative to the target ring.
    *  
-   * @param p0 a segment vertex
-   * @param p1 a segment vertex
-   * @param ring the ring to test
-   * @return true if the segment lies inside the ring
+   * @param test the ring to test
+   * @param target the ring to test against
+   * @return true if the test ring lies inside the target ring
    */
-  public static boolean isSegmentInRing(Coordinate p0, Coordinate p1, LinearRing ring) {
-    Coordinate[] ringPts = ring.getCoordinates();
-    int loc = PointLocation.locateInRing(p0, ringPts);
+  public static boolean isRingNested(LinearRing test, LinearRing target) {
+    Coordinate p0 = test.getCoordinateN(0);
+    Coordinate[] targetPts = target.getCoordinates();
+    int loc = PointLocation.locateInRing(p0, targetPts);
     if (loc == Location.EXTERIOR) return false;
     if (loc == Location.INTERIOR) return true;
 
     /**
-     * The segment point is on the boundary of the ring.
+     * The start point is on the boundary of the ring.
      * Use the topology at the node to check if the segment
      * is inside or outside the ring.
      */
-    return isIncidentSegmentInRing(p0, p1, ringPts);
+    Coordinate p1 = findNonEqualVertex(test, p0);
+    return isIncidentSegmentInRing(p0, p1, targetPts);
+  }
+
+  private static Coordinate findNonEqualVertex(LinearRing ring, Coordinate p) {
+    int i = 1;
+    Coordinate next = ring.getCoordinateN(i);
+    while (next.equals2D(p) && i < ring.getNumPoints() - 1) {
+      i += 1;
+      next = ring.getCoordinateN(i);
+    }
+    return next;
   }
 
   /**
@@ -96,21 +98,18 @@ public static boolean isSegmentInRing(Coordinate p0, Coordinate p1, LinearRing r
    * This works for both shells and holes, but the caller must know
    * the ring role.
    * 
-   * @param p0 the first vertex of the segment
+   * @param p0 the touching vertex of the segment
    * @param p1 the second vertex of the segment 
    * @param ringPts the points of the ring
    * @return true if the segment is inside the ring.
    */
-  public static boolean isIncidentSegmentInRing(Coordinate p0, Coordinate p1, Coordinate[] ringPts) {
+  private static boolean isIncidentSegmentInRing(Coordinate p0, Coordinate p1, Coordinate[] ringPts) {
     int index = intersectingSegIndex(ringPts, p0);
     if (index < 0) {
       throw new IllegalArgumentException("Segment vertex does not intersect ring");
     }
-    Coordinate rPrev = ringPts[index];
-    Coordinate rNext = ringPts[index + 1];
-    if (p0.equals2D(ringPts[index])) {
-      rPrev = ringPts[ringIndexPrev(ringPts, index)];
-    }
+    Coordinate rPrev = findRingVertexPrev(ringPts, index, p0);
+    Coordinate rNext = findRingVertexNext(ringPts, index, p0);
     /**
      * If ring orientation is not normalized, flip the corner orientation
      */
@@ -122,6 +121,61 @@ public static boolean isIncidentSegmentInRing(Coordinate p0, Coordinate p1, Coor
     }
     return PolygonNode.isInteriorSegment(p0, rPrev, rNext, p1);
   }
+
+  /**
+   * Finds the ring vertex previous to a node point on a ring
+   * (which is contained in the index'th segment,
+   * as either the start vertex or an interior point). 
+   * Repeated points are skipped over.
+   * @param ringPts the ring
+   * @param index the index of the segment containing the node
+   * @param node the node point
+   * 
+   * @return the previous ring vertex
+   */
+  private static Coordinate findRingVertexPrev(Coordinate[] ringPts, int index, Coordinate node) {
+    int iPrev = index;
+    Coordinate prev = ringPts[iPrev];
+    while (node.equals2D(prev)) {
+      iPrev = ringIndexPrev(ringPts, iPrev);
+      prev = ringPts[iPrev];
+    }
+    return prev;
+  }  
+
+  private static int ringIndexPrev(Coordinate[] ringPts, int index) {
+    int iPrev = index - 1;
+    if (iPrev < 0) iPrev = ringPts.length - 2;
+    return iPrev;
+  }
+
+  /**
+   * Finds the ring vertex next from a node point on a ring
+   * (which is contained in the index'th segment,
+   * as either the start vertex or an interior point). 
+   * Repeated points are skipped over.
+   * @param ringPts the ring
+   * @param index the index of the segment containing the node
+   * @param node the node point
+   * 
+   * @return the next ring vertex
+   */
+  private static Coordinate findRingVertexNext(Coordinate[] ringPts, int index, Coordinate node) {
+    //-- safe, since index is always the start of a ring segment
+    int iNext = index + 1;
+    Coordinate next = ringPts[iNext];
+    while (node.equals2D(next)) {
+      iNext = ringIndexNext(ringPts, iNext);
+      next = ringPts[iNext];
+    }
+    return next;
+  }
+
+  private static int ringIndexNext(Coordinate[] ringPts, int index) {
+    int iNext = index + 1;
+    if (iNext > ringPts.length - 2) iNext = 0;
+    return iNext;
+  }
 
   /**
    * Computes the index of the segment which intersects a given point.
@@ -143,11 +197,18 @@ private static int intersectingSegIndex(Coordinate[] ringPts, Coordinate pt) {
     }
     return -1;
   }
-
-  private static int ringIndexPrev(Coordinate[] ringPts, int index) {
-    int iPrev = index - 1;
-    if (index == 0) iPrev = ringPts.length - 2;
-    return iPrev;
+
+  /**
+   * Finds a self-intersection (if any) in a {@link LinearRing}.
+   * 
+   * @param ring the ring to analyze
+   * @return a self-intersection point if one exists, or null
+   */
+  public static Coordinate findSelfIntersection(LinearRing ring) {
+    PolygonTopologyAnalyzer ata = new PolygonTopologyAnalyzer(ring, false);
+    if (ata.hasInvalidIntersection())
+      return ata.getInvalidLocation();
+    return null;
   }
 
   private boolean isInvertedRingValid;

modules/core/src/test/java/org/locationtech/jts/operation/valid/IsValidTest.java

@@ -156,6 +156,27 @@ public void testLinearRingSelfCrossing2() {
           "LINEARRING (0 0, 100 100, 100 0, 0 100, 0 0)");
   }
 
+  /**
+   * Tests that repeated points at nodes are handled correctly.
+   * 
+   * See https://github.com/locationtech/jts/issues/843
+   */
+  public void testPolygonHoleWithRepeatedShellPointTouch() {
+    checkValid( "POLYGON ((90 10, 10 10, 50 90, 50 90, 90 10), (50 90, 60 30, 40 30, 50 90))");
+  }
+
+  public void testPolygonHoleWithRepeatedShellPointTouchMultiple() {
+    checkValid( "POLYGON ((90 10, 10 10, 50 90, 50 90, 50 90, 50 90, 90 10), (50 90, 60 30, 40 30, 50 90))");
+  }
+
+  public void testPolygonHoleWithRepeatedTouchEndPoint() {
+    checkValid( "POLYGON ((90 10, 10 10, 50 90, 90 10, 90 10), (90 10, 40 30, 60 50, 90 10))");
+  }
+
+  public void testPolygonHoleWithRepeatedHolePointTouch() {
+    checkValid( "POLYGON ((50 90, 10 10, 90 10, 50 90), (50 90, 50 90, 60 40, 60 40, 40 40, 50 90))");
+  }
+
   //=============================================
 
   private void checkValid(String wkt) {