Skip to content

cgltf_node transform attributes are now store in double#239

Open
bcomb wants to merge 2 commits intojkuhlmann:masterfrom
VirtualGeo:master
Open

cgltf_node transform attributes are now store in double#239
bcomb wants to merge 2 commits intojkuhlmann:masterfrom
VirtualGeo:master

Conversation

@bcomb
Copy link
Contributor

@bcomb bcomb commented Nov 27, 2023

Some GLTF contain huge value in translation and reading these as float cause lost in precision. Google store big translation in their 'Photorealistic 3D Tiles' data

Some GLTF contain huge value in translation and reading these as float cause lost in precision. Google store big translation in their 'Photorealistic 3D Tiles' data
@jkuhlmann
Copy link
Owner

Hey! The tests failed unfortunately.
Also, do you think it would make sense to combine this with the changes in #229?

@zeux
Copy link
Contributor

zeux commented Nov 27, 2023

Related: #228 outlines the caveats around compatibility with an unconditional change like this. That said, #229 has a much wider scope that technically isn't required there. Maybe a good middleground would be something like cgltf_xfloat that's only used for transform components, still defaults to float (unlike this change), can be configured via a macro? That would keep backwards compatibility but also have a narrow scope unlike #229.

@bcomb
Copy link
Contributor Author

bcomb commented Nov 27, 2023

Hi, I've fix the test.

Technically, all attributes of the JSON text block should be read in double precision; the GLTF specification does not specify a storage of floating-point numbers in 32-bit. In practice, I do not see the necessity of transforming everything into double precision except for the transform related components.

I understand the compatibility caveat of this kind of modification, but the required modification is still very straightforward. However, if it seems essential for CGLTF, the solution proposed by @zeux looks good to me (cgltf_xfloat used only for the transform related attributes and a CGLTF_USE_DOUBLE_PRECISION_TRANSFORM macro to control activation)

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

Successfully merging this pull request may close these issues.

3 participants