Fix DepthSegment compareTo method (#920)

* Fix DepthSegment comparison method

Signed-off-by: Martin Davis <[email protected]>

modules/core/src/main/java/org/locationtech/jts/operation/buffer/SubgraphDepthLocater.java

@@ -162,29 +162,25 @@ static class DepthSegment
 
     public DepthSegment(LineSegment seg, int depth)
     {
-      // input seg is assumed to be normalized
+      // Assert: input seg is upward (p0.y <= p1.y)
       upwardSeg = new LineSegment(seg);
-      //upwardSeg.normalize();
       this.leftDepth = depth;
     }
+
+    public boolean isUpward() {
+      return upwardSeg.p0.y <= upwardSeg.p1.y;
+    }
+
     /**
-     * Defines a comparison operation on DepthSegments
-     * which orders them left to right.
-     * Assumes the segments are normalized.
+     * A comparison operation
+     * which orders segments left to right.
      * <p>
      * The definition of the ordering is:
      * <ul>
      * <li>-1 : if DS1.seg is left of or below DS2.seg (DS1 < DS2)
-     * <li>1 : if   DS1.seg is right of or above DS2.seg (DS1 > DS2) 
+     * <li>1 : if  DS1.seg is right of or above DS2.seg (DS1 > DS2) 
      * <li>0 : if the segments are identical 
      * </ul>
-     *
-     * KNOWN BUGS:
-     * <ul>
-     * <li>The logic does not obey the {@link Comparator.compareTo} contract. 
-     * This is acceptable for the intended usage, but may cause problems if used with some
-     * utilities in the Java standard library (e.g. {@link Collections.sort()}.
-     * </ul>
      * 
      * @param obj a DepthSegment
      * @return the comparison value
@@ -193,9 +189,48 @@ public int compareTo(Object obj)
     {
       DepthSegment other = (DepthSegment) obj;
 
+      /**
+       * If segment envelopes do not overlap, then
+       * can use standard segment lexicographic ordering.
+       */
+      if (upwardSeg.minX() >= other.upwardSeg.maxX()
+          || upwardSeg.maxX() <= other.upwardSeg.minX()
+          || upwardSeg.minY() >= other.upwardSeg.maxX()
+          || upwardSeg.maxY() <= other.upwardSeg.minY()) {
+        return upwardSeg.compareTo(other.upwardSeg);
+      };
+
+      /**
+       * Otherwise if envelopes overlap, use relative segment orientation.
+       * 
+       * Collinear segments should be evaluated by previous logic
+       */
+      int orientIndex = upwardSeg.orientationIndex(other.upwardSeg);
+      if (orientIndex != 0) return orientIndex;
+
+      /**
+       * If comparison between this and other is indeterminate,
+       * try the opposite call order.
+       * The sign of the result needs to be flipped.
+       */
+      orientIndex = -1 * other.upwardSeg.orientationIndex(upwardSeg);
+      if (orientIndex != 0) return orientIndex;
+
+      /**
+       * If segment envelopes overlap and they are collinear,
+       * since segments do not cross they must be equal.
+       */
+      // assert: segments are equal
+      return 0;
+    }
+
+    public int OLDcompareTo(Object obj)
+    {
+      DepthSegment other = (DepthSegment) obj;
+
       // fast check if segments are trivially ordered along X
-      if (upwardSeg.minX() >= other.upwardSeg.maxX()) return 1;
-      if (upwardSeg.maxX() <= other.upwardSeg.minX()) return -1;
+      if (upwardSeg.minX() > other.upwardSeg.maxX()) return 1;
+      if (upwardSeg.maxX() < other.upwardSeg.minX()) return -1;
 
       /**
        * try and compute a determinate orientation for the segments.
@@ -212,30 +247,10 @@ public int compareTo(Object obj)
       orientIndex = -1 * other.upwardSeg.orientationIndex(upwardSeg);
       if (orientIndex != 0) return orientIndex;
 
-      // otherwise, use standard lexocographic segment ordering
+      // otherwise, use standard lexicographic segment ordering
       return upwardSeg.compareTo(other.upwardSeg);
     }
 
-    /**
-     * Compare two collinear segments for left-most ordering.
-     * If segs are vertical, use vertical ordering for comparison.
-     * If segs are equal, return 0.
-     * Segments are assumed to be directed so that the second coordinate is >= to the first
-     * (e.g. up and to the right).
-     *
-     * @param seg0 a segment to compare
-     * @param seg1 a segment to compare
-     * @return
-     */
-    private int compareX(LineSegment seg0, LineSegment seg1)
-    {
-      int compare0 = seg0.p0.compareTo(seg1.p0);
-      if (compare0 != 0)
-        return compare0;
-      return seg0.p1.compareTo(seg1.p1);
-
-    }
-
     public String toString()
     {
       return upwardSeg.toString();

modules/core/src/test/java/org/locationtech/jts/operation/buffer/DepthSegmentTest.java

@@ -30,28 +30,53 @@ public static void main(String[] args) {
     junit.textui.TestRunner.run(DepthSegmentTest.class);
   }
 
-  public void testContractTipToTail() throws Exception
+  public void testCompareTipToTail() throws Exception
   {
     SubgraphDepthLocater.DepthSegment ds0 = depthSeg(0.7, 0.2, 1.4, 0.9);
-    SubgraphDepthLocater.DepthSegment ds1 = depthSeg(0.3, 1.1, 0.7, 0.2);
-    checkContract(ds0, ds1);
+    SubgraphDepthLocater.DepthSegment ds1 = depthSeg(0.7, 0.2, 0.3, 1.1);
+    checkCompare(ds0, ds1, 1);
   }
 
-  public void testContract2() throws Exception
+  public void testCompare2() throws Exception
   {
-    SubgraphDepthLocater.DepthSegment ds0 = depthSeg(0.1, 1.9, 0.5, 1.0);
+    SubgraphDepthLocater.DepthSegment ds0 = depthSeg(0.5, 1.0, 0.1, 1.9);
     SubgraphDepthLocater.DepthSegment ds1 = depthSeg(1.0, 0.9, 1.9, 1.4);
-    checkContract(ds0, ds1);
+    checkCompare(ds0, ds1, -1);
   }
 
-  private void checkContract(
+  public void testCompareVertical() throws Exception
+  {
+    SubgraphDepthLocater.DepthSegment ds0 = depthSeg(1, 1, 1, 2);
+    SubgraphDepthLocater.DepthSegment ds1 = depthSeg(1, 0, 1, 1);
+    checkCompare(ds0, ds1, 1);
+  }
+
+  public void testCompareOrientBug() throws Exception
+  {
+    SubgraphDepthLocater.DepthSegment ds0 = depthSeg(146.268, -8.42361, 146.263, -8.3875);
+    SubgraphDepthLocater.DepthSegment ds1 = depthSeg(146.269, -8.42889, 146.268, -8.42361);
+    checkCompare(ds0, ds1, -1);
+  }
+
+  public void testCompareEqual() throws Exception
+  {
+    SubgraphDepthLocater.DepthSegment ds0 = depthSeg(1, 1, 2, 2);
+    checkCompare(ds0, ds0, 0);
+  }
+
+  private void checkCompare(
      SubgraphDepthLocater.DepthSegment ds0,
-     SubgraphDepthLocater.DepthSegment ds1) {
-    // should never have ds1 < ds2 && ds2 < ds1
-    int cmp0 = ds0.compareTo(ds1);
-    int cmp1 = ds1.compareTo(ds0);
-    boolean isFail = cmp0 != 0 && cmp0 == cmp1;
-    assertTrue(! isFail);
+     SubgraphDepthLocater.DepthSegment ds1, 
+     int expectedComp) 
+  {
+    assertTrue(ds0.isUpward());
+    assertTrue(ds1.isUpward());
+
+    // check compareTo contract - should never have ds1 < ds2 && ds2 < ds1
+    int comp0 = ds0.compareTo(ds1);
+    int comp1 = ds1.compareTo(ds0);
+    assertEquals(expectedComp, comp0);
+    assertTrue( comp0 == -comp1);
   }
 
   private SubgraphDepthLocater.DepthSegment depthSeg(double x0, double y0, double x1, double y1) {