Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

s2 cannot handle more than 2^31 - 1 vertices in polyline/polygon #305

Open
MBkkt opened this issue Jan 17, 2023 · 7 comments
Open

s2 cannot handle more than 2^31 - 1 vertices in polyline/polygon #305

MBkkt opened this issue Jan 17, 2023 · 7 comments

Comments

@MBkkt
Copy link
Contributor

MBkkt commented Jan 17, 2023

https://github.com/google/s2geometry/blob/master/src/s2/encoded_s2point_vector.h#L131

Access by int, uint32 size_, by size encoded and decoded as size_t

So in general s2 cannot handle more than 2^31 - 1 vertices in polyline/polygon :(

It looks pretty easy to fix and also looks like current encoding/decoding can handle this.

Do you plan to fix this?

@smcallis
Copy link
Collaborator

smcallis commented Jan 17, 2023

I'm sure that we could, but no one's asked before, so you plan to have more than 4B vertices? That's ~96GB of vertices in a single polygon.

@MBkkt
Copy link
Contributor Author

MBkkt commented Jan 17, 2023

I agree it's not very realistic, but at the same time it is just strange.

Also I think it is possible in more simplified case:

For an example if you have just a lot of points, and you just want to serialize/deserialize it.

Best way which I see now is using:
s2coding::EncodeS2PointVector and EncodedS2PointVector

In both cases it's impossible to use something more larger.

Also it's smaller count of bytes, it's maximum without compression 2^31 * 2^3 * 3 = 48GB

@smcallis
Copy link
Collaborator

Makes sense, I can get it fixed but I'll do it internally for the next release since it doesn't sound urgently needed.

@MBkkt
Copy link
Contributor Author

MBkkt commented Jan 17, 2023

Nice, thanks!

@jmr
Copy link
Member

jmr commented Jan 18, 2023

There are a lot of int/int32s. How extensively are you going to change them?

Do we ever have enough array indexes stored so that changing the type is going to blow up the memory use and halve the cache efficiency?

I see some typedefs in S2Builder:

using InputVertexId = int32;

@MBkkt
Copy link
Contributor Author

MBkkt commented Jan 18, 2023

I agree it a lot of places, I just noticed difference between serialization and runtime and decide to ask, do you plan to fix runtime or not

@smcallis
Copy link
Collaborator

TBD, I'll have to poke around and see how big of a lift it would be, it might not be worth the trouble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants