google / s2geometry

Computational geometry and spatial indexing on the sphere
http://s2geometry.io/
Apache License 2.0
2.29k stars 302 forks source link

Add access to S2Polygon::subregion_bound_ #302

Closed MBkkt closed 3 months ago

jmr commented 1 year ago

What is the larger problem you're trying to solve?

MBkkt commented 1 year ago

What is the larger problem you're trying to solve?

@jmr Written in comment.

In general I need to check S2Polygon contains S2LatLngRect.

I can create S2Polygon from S2LatLngRect, but it's need allocations, etc.

So firstly I make checks which don't need to construct Polygon

MBkkt commented 1 year ago

@jmr https://github.com/arangodb/arangodb/blob/devel/lib/Geo/ShapeContainer.cpp#L64

Example

jmr commented 1 year ago

I still don't understand. Is this for performance or correctness?

jmr commented 1 year ago

By correctness, I mean, will it be wrong if you use GetRectBound()?

MBkkt commented 1 year ago

By correctness, I mean, will it be wrong if you use GetRectBound()?

Yes, because otherwise S2Polygon also should use rect bound not subregion bound.

If I understand correctly can be !A.rect.contains(B.rect) && A.contains(B)

To avoid this S2Polygon compute subregion bound

MBkkt commented 1 year ago

https://github.com/google/s2geometry/blob/f28f17930d128a8d879132f4d1f9d68c5de27402/src/s2/s2polygon.h#L975

@jmr Yep, check it please

jmr commented 1 year ago

I'm not sure. You're checking containment of an S2LatLngRect, not an S2Region, so the extra expansion may not be required. Since the boundaries are S2LatLngs, I think bound_ is enough.

I don't quite understand the situation described by the subregion_bound_ comments.

No tests fail if I change the use of subregion_bound_.Contains to bound_.Contains. Ideally, we'd have some, then determine if it's needed for checking S2LatLngRect containment.

MBkkt commented 1 year ago

Hmm, ok, I will try to write test

MBkkt commented 3 months ago

No need this functions for now