Fix Loop contains which contained a missing negation and add test cases. (#156)

Loop's hasCrossingRelation() had the wrong condition on one branch. This
has been resolved and test cases added ensuring it was fixed.

Fix #77 
Fix #78

---------

Co-authored-by: Alan Strohm <astrohm@google.com>

Author: rsned

s2/loop.go

@@ -1584,11 +1584,11 @@ func (l *loopCrosser) hasCrossing(ai, bi *rangeIterator) bool {
 	l.bCells = nil
 
 	for {
-		if n := bi.it.IndexCell().shapes[0].numEdges(); n > 0 {
+		if n := bi.clipped().numEdges(); n > 0 {
 			totalEdges += n
 			if totalEdges >= edgeQueryMinEdges {
 				// There are too many edges to test them directly, so use CrossingEdgeQuery.
-				if l.cellCrossesAnySubcell(ai.it.IndexCell().shapes[0], ai.cellID()) {
+				if l.cellCrossesAnySubcell(ai.clipped(), ai.cellID()) {
 					return true
 				}
 				bi.seekBeyond(ai)
@@ -1604,20 +1604,20 @@ func (l *loopCrosser) hasCrossing(ai, bi *rangeIterator) bool {
 
 	// Test all the edge crossings directly.
 	for _, c := range l.bCells {
-		if l.cellCrossesCell(ai.it.IndexCell().shapes[0], c.shapes[0]) {
+		if l.cellCrossesCell(ai.clipped(), c.shapes[0]) {
 			return true
 		}
 	}
 
 	return false
 }
 
-// containsCenterMatches reports if the clippedShapes containsCenter boolean corresponds
-// to the crossing target type given. (This is to work around C++ allowing false == 0,
-// true == 1 type implicit conversions and comparisons)
-func containsCenterMatches(a *clippedShape, target crossingTarget) bool {
-	return (!a.containsCenter && target == crossingTargetDontCross) ||
-		(a.containsCenter && target == crossingTargetCross)
+// containsCenterMatches reports if the clippedShapes containsCenter boolean
+// corresponds to the crossing target type given. (This is to work around C++
+// allowing false == 0, true == 1 type implicit conversions and comparisons)
+func containsCenterMatches(containsCenter bool, target crossingTarget) bool {
+	return (!containsCenter && target == crossingTargetDontCross) ||
+		(containsCenter && target == crossingTargetCross)
 }
 
 // hasCrossingRelation reports whether given two iterators positioned such that
@@ -1626,7 +1626,8 @@ func containsCenterMatches(a *clippedShape, target crossingTarget) bool {
 // is an edge crossing, a wedge crossing, or a point P that matches both relations
 // crossing targets. This function advances both iterators past ai.cellID.
 func (l *loopCrosser) hasCrossingRelation(ai, bi *rangeIterator) bool {
-	aClipped := ai.it.IndexCell().shapes[0]
+	// ABSL_DCHECK(ai->id().contains(bi->id()));
+	aClipped := ai.clipped()
 	if aClipped.numEdges() != 0 {
 		// The current cell of A has at least one edge, so check for crossings.
 		if l.hasCrossing(ai, bi) {
@@ -1636,8 +1637,9 @@ func (l *loopCrosser) hasCrossingRelation(ai, bi *rangeIterator) bool {
 		return false
 	}
 
-	if containsCenterMatches(aClipped, l.aCrossingTarget) {
-		// The crossing target for A is not satisfied, so we skip over these cells of B.
+	if !containsCenterMatches(ai.containsCenter(), l.aCrossingTarget) {
+		// The crossing target for A is not satisfied, so we skip over
+		// these cells of B.
 		bi.seekBeyond(ai)
 		ai.next()
 		return false
@@ -1647,8 +1649,7 @@ func (l *loopCrosser) hasCrossingRelation(ai, bi *rangeIterator) bool {
 	// worth iterating through the cells of B to see whether any cell
 	// centers also satisfy the crossing target for B.
 	for bi.cellID() <= ai.rangeMax {
-		bClipped := bi.it.IndexCell().shapes[0]
-		if containsCenterMatches(bClipped, l.bCrossingTarget) {
+		if containsCenterMatches(bi.containsCenter(), l.bCrossingTarget) {
 			return true
 		}
 		bi.next()
@@ -1701,16 +1702,16 @@ func hasCrossingRelation(a, b *Loop, relation loopRelation) bool {
 					return true
 				}
 			} else {
-				// The A and B cells are the same. Since the two cells
-				// have the same center point P, check whether P satisfies
-				// the crossing targets.
-				aClipped := ai.it.IndexCell().shapes[0]
-				bClipped := bi.it.IndexCell().shapes[0]
-				if containsCenterMatches(aClipped, ab.aCrossingTarget) &&
-					containsCenterMatches(bClipped, ab.bCrossingTarget) {
+				// The A and B cells are the same. Since the two
+				// cells have the same center point P, check
+				// whether P satisfies the crossing targets.
+				if containsCenterMatches(ai.containsCenter(), ab.aCrossingTarget) &&
+					containsCenterMatches(bi.containsCenter(), ab.bCrossingTarget) {
 					return true
 				}
 				// Otherwise test all the edge crossings directly.
+				aClipped := ai.clipped()
+				bClipped := bi.clipped()
 				if aClipped.numEdges() > 0 && bClipped.numEdges() > 0 && ab.cellCrossesCell(aClipped, bClipped) {
 					return true
 				}

s2/loop_test.go

@@ -805,6 +805,29 @@ func TestLoopContainsMatchesCrossingSign(t *testing.T) {
 }
 
 func TestLoopRelations(t *testing.T) {
+	// this loop is used in the regression test cases below.
+	containingLoop := LoopFromPoints([]Point{
+		PointFromLatLng(LatLngFromDegrees(-38.0, -135.0)),
+		PointFromLatLng(LatLngFromDegrees(-38.0, 149.0)),
+		PointFromLatLng(LatLngFromDegrees(77.0, 149.0)),
+		PointFromLatLng(LatLngFromDegrees(77.0, -135.0)),
+	})
+
+	innerTile := LoopFromPoints([]Point{
+		PointFromLatLng(LatLngFromDegrees(37.99616267972809, 13.007812500000002)),
+		PointFromLatLng(LatLngFromDegrees(37.99616267972809, 13.359375000000002)),
+		PointFromLatLng(LatLngFromDegrees(38.272819658516866, 13.359375000000002)),
+		PointFromLatLng(LatLngFromDegrees(38.272819658516866, 13.007812500000002)),
+	})
+
+	// +0.2 lat +0.2 lon from innerTile
+	extendedTile := LoopFromPoints([]Point{
+		PointFromLatLng(LatLngFromDegrees(37.99616267972809, 13.007812500000002)),
+		PointFromLatLng(LatLngFromDegrees(37.99616267972809, 13.559375000000002)),
+		PointFromLatLng(LatLngFromDegrees(38.472819658516866, 13.559375000000002)),
+		PointFromLatLng(LatLngFromDegrees(38.472819658516866, 13.007812500000002)),
+	})
+
 	tests := []struct {
 		a, b       *Loop
 		contains   bool // A contains B
@@ -1332,6 +1355,19 @@ func TestLoopRelations(t *testing.T) {
 			contains:   true,
 			sharedEdge: true,
 		},
+		// Cases below here prevent regressions of bugs which
+		// previously appeared in the golang version
+		// and which are missing test coverage in the C++ codebase.
+		{
+			a:        containingLoop,
+			b:        innerTile,
+			contains: true,
+		},
+		{
+			a:        containingLoop,
+			b:        extendedTile,
+			contains: true,
+		},
 	}
 
 	for _, test := range tests {
@@ -1817,3 +1853,17 @@ func BenchmarkLoopContainsPoint(b *testing.B) {
 		vertices *= 2
 	}
 }
+
+// TODO(rsned): Differences from C++
+// TEST_F(S2LoopTestBase, CompressedEncodedLoopDecodesApproxEqual) {
+// TEST_F(S2LoopTestBase, DistanceMethods) {
+// TEST_F(S2LoopTestBase, GetAreaAccuracy) {
+// TEST_F(S2LoopTestBase, GetAreaConsistentWithSign) {
+// TEST_F(S2LoopTestBase, MakeRegularLoop) {
+// TEST(S2Loop, BoundaryNear) {
+// TEST(S2Loop, BoundsForLoopContainment) {
+// TEST(S2LoopDeathTest, IsValidDetectsInvalidLoops) {
+// TEST(S2Loop, DefaultLoopIsInvalid) {
+// TEST(S2Loop, EmptyFullLossyConversions) {
+// TEST(S2Loop, EncodeDecode) {
+// TEST(S2Loop, LoopRelations2) {

s2/shapeindex.go

@@ -129,6 +129,14 @@ func (s *ShapeIndexCell) numEdges() int {
 	return e
 }
 
+// clipped returns the clipped shape at the given index. Shapes are kept sorted in
+// increasing order of shape id.
+//
+// Requires: 0 <= i < len(shapes)
+func (s *ShapeIndexCell) clipped(i int) *clippedShape {
+	return s.shapes[i]
+}
+
 // add adds the given clipped shape to this index cell.
 func (s *ShapeIndexCell) add(c *clippedShape) {
 	// C++ uses a set, so it's ordered and unique. We don't currently catch

s2/shapeutil.go

@@ -52,6 +52,8 @@ func newRangeIterator(index *ShapeIndex) *rangeIterator {
 
 func (r *rangeIterator) cellID() CellID             { return r.it.CellID() }
 func (r *rangeIterator) indexCell() *ShapeIndexCell { return r.it.IndexCell() }
+func (r *rangeIterator) clipped() *clippedShape     { return r.indexCell().clipped(0) }
+func (r *rangeIterator) containsCenter() bool       { return r.clipped().containsCenter }
 func (r *rangeIterator) next()                      { r.it.Next(); r.refresh() }
 func (r *rangeIterator) done() bool                 { return r.it.Done() }