yjh0502 / rust-s2

S2 geometry library in Rust
Apache License 2.0
77 stars 20 forks source link

`st_to_ij` returns `i32` instead of `u32` #35

Closed austenadler closed 1 year ago

austenadler commented 1 year ago

The function here returns i32: https://github.com/yjh0502/rust-s2/blob/530829fdd053a1de4dc340e5233b46baa782553d/src/s2/cellid.rs#L752-L755 and https://github.com/yjh0502/rust-s2/blob/530829fdd053a1de4dc340e5233b46baa782553d/src/s2/cellid.rs#L130

But the S2 documentation says:

"i" and "j" are integers in the range [0, 2^30-1]

Maybe this is a bug in the go version that this one is based off?

https://github.com/golang/geo/blob/740aa86cb551d6388f5cf4a8f39568d52fac6ed7/s2/cellid.go#L637-L639

I don't think this would cause any issue in end-to-end calls because it is treated as a bitwise value, so that might be why it wasn't caught before.

yjh0502 commented 1 year ago

c++ version also returns signed integer, so I guess Golang version followed it.

https://github.com/google/s2geometry/blob/d4a1257f65f1456c8e668aace203b8140ef75137/src/s2/s2coords.h#L333-L336

brawer commented 1 year ago

range [0, 2^30-1]

What’s the problem? An i32 can represent positive integers up to 2^31 – 1.

austenadler commented 1 year ago

That's a good point. I was thinking sign was more significant, forgetting i32 could also hold this entire type.

This is not an issue; closing