golang / geo

S2 geometry library in Go
Apache License 2.0
1.69k stars 182 forks source link

Coverer.CellUnion(Point) can have multiple cells? #58

Closed jsmorph closed 4 years ago

jsmorph commented 4 years ago

In S2, in some cases, a Coverer.CellUnion() of a Point can contain two cells. Is that the correct behavior? If so, what's the story?

Example:

package main

import (
    "fmt"

    "github.com/golang/geo/s2"
)

func main() {
    var (
        level   = 7
        coverer = s2.RegionCoverer{
            MinLevel: level,
            MaxLevel: level,
        }
        ll    = s2.LatLng{Lat: -0.8248776721189366, Lng: -2.038611228784389}
        p     = s2.PointFromLatLng(ll)
        union = coverer.CellUnion(p)
    )
    fmt.Println(len(union))
}

At the playground.

jsmorph commented 4 years ago

5/0030111 and 5/0101000 both contain s2.LatLng{Lat: -0.8248776721189366, Lng: -2.038611228784389} per the playground. Surprised me!

rsned commented 4 years ago

The RegionCoverer operates on Regions, and even though you are only using a single Point, it is interpreted as a Region. The Coverer uses the CellUnionBound() for the given Point which is a CellID slice of (top left, top right, bottom left, bottom right) as the bounding box on the point at the lowest level (30).

Even though the bounding box is microscopic, it does cross over the boundary leading to it being in two cells at the same time. You can see in the attached images a view of the two level 7 cells with a tiny tiny tiny spot. Zooming in, the spot turns out to be the 4 cells.

fmt.Printf("%+v\n", p.CellUnionBound()):

[5/01010003233230110323000103222 5/01010003233230110323000100111

5/00301110322301001032333010333 5/00301110322301001032333011000]

You happened to get lucky (or unlucky) in that the LatLng you used happened to be close enough to the edge of the two cells so that it's bounding box spilled over.

-- Robert

6eLhYruSp8U KUY3ob5sLBe

On Sun, Jan 19, 2020 at 9:47 PM Jamie Stephens notifications@github.com wrote:

5/0030111 and 5/0101000 both contain s2.LatLng{Lat: -0.8248776721189366, Lng: -2.038611228784389} per the playground https://play.golang.org/p/DKMl-pYCoea. Surprised me!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/golang/geo/issues/58?email_source=notifications&email_token=ACBHGBEX74YZN2WRXXNJASLQ6U3GXA5CNFSM4KI6QX3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJLNPZA#issuecomment-576116708, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACBHGBH3JN3E57P4BKS6SOTQ6U3GXANCNFSM4KI6QX3A .

rsned commented 4 years ago

6eLhYruSp8U KUY3ob5sLBe

jsmorph commented 4 years ago

@rsned, thanks for the quick, detailed response.

For posterity:

Since Cell.ContainsPoint(Point) reports true for those two cells and that point, I decided to read the documentation, which says to convert a Cell to a Loop. That works.