make s2.Loop Regions & serializable + Fix loop.IntersectsCell

[azr]

Hello there,

I really need a way to serialize a big bunch of s2.Loops and also @buckhx made them Region compliant. ( which I needed too )

I'm pushing this here as some people might need that too. Just tell me if something is wrong I'll change it :).

Thoughts: it's much more convenient if everything is public in a struct that is returned by a package. Specially for serialization cases. If you are afraid someone will tweak too much variables and they should not, well they just better know what they are doing :).

o/

EDIT: sorry, made the pull request before asking anything.

[azr]

I resolved some conflicts. :)

[azr]

I just found a bug case where the Covering returns an empty CellUnion where it should not.

This happens because the generated cell by FastCovering is much bigger than the actual Region ( which is a Loop ) and Cells for whichregion.IntersectsCell(cell) == false are filtered out inside coverer.newCandidate.

As the region is fully contained by the cell it does not intersects it.
I think intersect should return true if the first point of the Loop is inside the cell.

I added a test case. I'll fix it.

[azr]

Here is a fix, tests are passing.

[azr]

Hello there ! I just rebased this.

Do you even accept prs at all ?

[dsymonds]

Sorry, this isn't a sufficient approach. Exporting all the fields is easy and makes some things simpler, but makes for a worse API.

[dsymonds]

Also, please keep features separate from bug fixes. This is way too noisy to untangle if we were interested in taking just a bug fix.

[azr]

Hello @dsymonds,

For the mixed code, it's actually fixing some code that made the loop a Region

On the field exporting, I disagree, this is golang, not C++.

The struct has :

func (l Loop) RectBound() Rect {
    return l.bound
}
func (l Loop) Vertices() []Point {
    return l.vertices
}
func (l Loop) ContainsOrigin() bool {
    return l.originInside
}

and may be misses a SubregionBound() []Rect func.

I mean, instead of documenting the funcs, the fields could be Public and documented ( and they already are ! :) )

Plus as I said before if you are scared people tweak those fields just because they are public, then they just better know what they do.

And this would make a better golang API to me.

[azr]

If you have any better/cooler idea to be able to encode this struct easily, I'm in to do it.

One weird way :

type publicLoop struct {
        Vertices []Point
        OriginInside bool
        Bound Rect
        SubregionBound Rect
}

func (l Loop) Fields() interface{} {
    return publicLoop{
        Vertices: l.vertices,
        OriginInside: l.originInside,
        Bound: l.bound,
        SubregionBound: l.subregionBound,
    }
}

Another way would be to implement every serialization known out there.

[dsymonds]

Loop is a long way from complete. Its internals are going to change a fair bit as it gets filled out, which is why things are unexported.

[azr]

aah okay, you don't want to break the api all the time :)

but do you have any preferences if the lib ever had a way to dump its structs to disk to later reload ?

What do you think of this approach ?

func (l Loop) Fields() interface{} {}
func (l Loop) SetFields(interface{}) error {}

[dsymonds]

There's a specific serialization format that C++ already defines, and we're going to match those. Someone is working on that already.

[azr]

Oki doki ! I'll just wait on my branch then :) thanks for your time