diff --git a/s2/loop.go b/s2/loop.go index 59378c5..3254f77 100644 --- a/s2/loop.go +++ b/s2/loop.go @@ -1612,12 +1612,12 @@ func (l *loopCrosser) hasCrossing(ai, bi *rangeIterator) bool { 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(a bool, target crossingTarget) bool { + return (!a && target == crossingTargetDontCross) || + (a && 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.it.IndexCell().shapes[0] + bClipped := bi.it.IndexCell().shapes[0] if aClipped.numEdges() > 0 && bClipped.numEdges() > 0 && ab.cellCrossesCell(aClipped, bClipped) { return true } diff --git a/s2/loop_test.go b/s2/loop_test.go index 6bd6b4f..8ace24d 100644 --- a/s2/loop_test.go +++ b/s2/loop_test.go @@ -805,6 +805,31 @@ func TestLoopContainsMatchesCrossingSign(t *testing.T) { } func TestLoopRelations(t *testing.T) { + + // Test cases from https://github.com/golang/geo/issues/77 and 78 + // relating to loop relations contains giving wrong result some cases. + 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 +1357,18 @@ func TestLoopRelations(t *testing.T) { contains: true, sharedEdge: true, }, + // https://github.com/golang/geo/issues/77 and 78 + // These cases failed prior to this fix. + { + a: containingLoop, + b: innerTile, + contains: true, + }, + { + a: containingLoop, + b: extendedTile, + contains: true, + }, } for _, test := range tests { @@ -1817,3 +1854,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) { diff --git a/s2/shapeindex.go b/s2/shapeindex.go index e567959..caa0389 100644 --- a/s2/shapeindex.go +++ b/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 diff --git a/s2/shapeutil.go b/s2/shapeutil.go index 64245df..27936df 100644 --- a/s2/shapeutil.go +++ b/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() }