Handling of optional transforms

[mokiat]

Hi,

First of all, great library, thanks for creating it.

This is both an issue and a support request, I guess. This one might be hard to explain and argument so bear with me.

It seems that the Node type does not conform well to the glTF specification. It defines Matrix, Translation, Rotation and Scale as required (i.e. non-pointer fields) but the specification clearly states that they are not required: https://www.khronos.org/registry/glTF/specs/2.0/glTF-2.0.html#reference-node

My initial expectation was that these values would be zeroes if not specified in the glTF files. This was based on the following methods and their implementations:

• https://pkg.go.dev/github.com/qmuntal/gltf#Node.MatrixOrDefault
• https://pkg.go.dev/github.com/qmuntal/gltf#Node.RotationOrDefault
• https://pkg.go.dev/github.com/qmuntal/gltf#Node.ScaleOrDefault
• https://pkg.go.dev/github.com/qmuntal/gltf#Node.TranslationOrDefault

However, it turned out (and this was tricky to spot and caught me off guard) that these fields will only be zeroes if they are specified in the glTF file as zeroes.

So as a result I now have the following questions:

• How do I determine whether I should use the Matrix field or the TRS fields?
  • One option for me would be to compare the Matrix with the identity matrix instead and if it is the identity to use the TRS values.
  • An alternative is to multiply the Matrix field with the TRS fields (converted to a Matrix) and hope that there isn't a non-compliant exporter out there that produces both (resulting in a double transform).
• When should I use ScaleOrDefault and the like? It seems that just using Scale directly should be better. In fact, it would allow me to handle situations where a zero scale is specified in order to make a node invisible (described in the spec here)

Maybe there is something I am missing since I am still fairly new to the API. And I also understand that the current design makes it more user-friendly, though personally the hidden behavior was confusing for me.

Something that can be considered for a backward-incompatible version in the future is to have the Matrix, Translation, Rotation and Scale fields be pointer types and have the respective MatrixOrDefault, RotationOrDefault, ScaleOrDefault and TranslationOrDefault produce default transforms when the field is nil.

[qmuntal]

How do I determine whether I should use the Matrix field or the TRS fields?

When I had to decide whether to use matrix or TRS fields I've always compared matrix with the identity matrix (gltf.DefaultMatrix), as you mention in the first bullet.

*OrDefault methods are useful if you want to be robust to Node instances being constructed without the appropriate default values. Decoding always sets them right, but if you instantiate a Node in any other place, it is possible that you forget to set set matrix or any TRS property.

When I defined the Node properties I thought it would be better not to use pointers for these properties, even if they are optional, but I'm now starting to think it causes more confusion than good. It's probably better to have a runtime panic due to dereferencing a nil pointer than silently applying a wrong transform.

Will think about it and might change the API, as this module is still at v0.

[mokiat]

Yes, I ended up comparing against the Identity Matrix as well and it worked for me.

Thanks for explaining the intention behind the methods. I guess it makes sense. While I think that nil transforms would have probably been more explanatory, I am not sure changing it now is worth it. Probably a bit more documentation or even the discussion in this issue here might be enough to help others out.