mapbox / vector-tile-spec

Mapbox Vector Tile specification
https://www.mapbox.com/vector-tiles/specification/
Other
890 stars 209 forks source link

Elevation 32 or 64 bit? #129

Closed joto closed 5 years ago

joto commented 5 years ago

Currently the elevation is in the spec as 64 bit:

repeated sint64 elevation = 7 [ packed = true];

Isn't 32 bit enough here? With the max elevation on Earth between -11km and +9 km, 32 bit gives us sub-millimeter precision already. And we have the scaling on top...

e-n-f commented 5 years ago

32 bits is probably plenty, but is there any advantage to using 32 when they are represented the same on the wire anyway?

flippmoke commented 5 years ago

@ericfischer I had the same thought, if they are the same size for most situations on the wire, why not support 64? I understand that this makes decoders pull out slightly larger integers perhaps, but I think this is a fair trade off in some ways.

32 bit could be nice though because at some point the scaling calculation will fall off entirely and make it take up more room then simply using double or float fixed precision here. Perhaps limiting it to 32 makes this intent more clear?

Overall, I am not for or against either way in particular.

joto commented 5 years ago

x/y coordinates are already defined as 32 bit, so this would simply make it consistent.

FWIW, this issue came up for me when implementing the delta decoding. With 64bit signed integer arithmetic there is the possibility of undefined behaviour due to an overflow, but when adding 32bit integers I can avoid that by casting to 64bit integers first (and back afterwards). That's the trick I am using for x/y coordinates already (https://github.com/mapbox/vtzero/blob/master/include/vtzero/geometry.hpp#L234-L246).

e-n-f commented 5 years ago

OK, I'm fine with 32 bits if it makes avoiding overflow easier, since 32 bits is plenty for any realistic need. I'll make the edit to the spec.

e-n-f commented 5 years ago

(Changed in 0773350)