Skip to content

Commit

Permalink
s2: Fix a bug in Polygon.initLoopProperties.
Browse files Browse the repository at this point in the history
Before this change to initLoopProperties to start the bounds as an
EmptyRect instead of it defaulting to the zero rect, the bounds
computations failed. This is because C++ gets default constructors on
Rect which default to Empty.

The bounds Union checks were doing Lo checks of the Min of 0 and the
other, and Hi would to Max of 0 and the other. So polygons that had low
corners above zero or high corners below zero would end up with
oversized bounds.

The same logic on updating bounds was in decodeCompressed. The TODO
there was to do the right way once it was addded which it was a while
back.

Signed-off-by: David Symonds <[email protected]>
  • Loading branch information
rsned authored and dsymonds committed May 7, 2019
1 parent 1929a04 commit b1eb033
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 12 deletions.
14 changes: 2 additions & 12 deletions s2/polygon.go
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ func (p *Polygon) initOneLoop() {
// initLoopProperties sets the properties for polygons with multiple loops.
func (p *Polygon) initLoopProperties() {
// the loops depths are set by initNested/initOriented prior to this.

p.bound = EmptyRect()
p.hasHoles = false
for _, l := range p.loops {
if l.IsHole() {
Expand Down Expand Up @@ -1177,18 +1177,8 @@ func (p *Polygon) decodeCompressed(d *decoder) {
for i := range p.loops {
p.loops[i] = new(Loop)
p.loops[i].decodeCompressed(d, snapLevel)
// TODO(roberts): Update this bound.Union call when initLoopProperties is implemented.
p.bound = p.bound.Union(p.loops[i].bound)
p.numVertices += len(p.loops[i].vertices)
}
if d.err != nil {
return
}
if p.numVertices == 0 {
p.bound = EmptyRect()
}
p.subregionBound = ExpandForSubregions(p.bound)
p.initEdgesAndIndex()
p.initLoopProperties()
}

// TODO(roberts): Differences from C++
Expand Down
18 changes: 18 additions & 0 deletions s2/polygon_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,24 @@ func TestPolygonFull(t *testing.T) {
}
}

func TestPolygonInitLoopPropertiesGetsRightBounds(t *testing.T) {
// Before the change to initLoopProperties to start the bounds as an
// EmptyRect instead of it default to the zero rect, the bounds
// computations failed. Lo was set to min (0, 12.55) and Hi was set to
// max (0, -70.02). So this poly would have a bounds of
// Lo: [0, -70.05], Hi: [12.58, 0]] instead of:
// Lo: [12.55, -70.05], Hi: [12.58, -70.02]]

p := PolygonFromLoops([]*Loop{
makeLoop("12.55:-70.05, 12.55:-70.02, 12.58:-70.02, 12.58:-70.05"),
makeLoop("12.56:-70.04, 12.56:-70.03, 12.58:-70.03, 12.58:-70.04"),
})
want := rectFromDegrees(12.55, -70.05, 12.58, -70.02)
if got := p.RectBound(); !rectsApproxEqual(got, want, 1e-6, 1e-6) {
t.Errorf("%v.RectBound() = %v, want %v", p, got, want)
}
}

func TestPolygonShape(t *testing.T) {
const numLoops = 100
const numVerticesPerLoop = 6
Expand Down

0 comments on commit b1eb033

Please sign in to comment.