KhronosGroup / COLLADA2GLTF

COLLADA to glTF converter
Other
534 stars 156 forks source link

Does one need to normalize the rotation computed by TransformTRS? #155

Open ziriax opened 6 years ago

ziriax commented 6 years ago

When using the glTF library, and decomposing the matrix {1,0,0,0,0,0.5,0,0,0,0,1,0,0,2,0,1} into TRS, I get a non-unit quaternion with length 0.875. Is this intented behavior?

As a side-note, would it be interesting to use SVD to also allow shearing, by decomposing any 3x3 matrix into rotation scale rotation? This would mean introducing extra dummy nodes. For example this implemention seems to be both accurate and fast.

lasalvavida commented 6 years ago

All of the matrix and quaternion math is just a C++ rewrite of the Cesium equivalent functions, so unless I made a mistake transcribing something, I would expect the results to be the same. I also get a non-unit quaternion with that matrix in cesium.

It would seem that the method used doesn't correct for scale, which I didn't take into account. So when going from matrix -> TRS we do need to compute the scale and normalize the rotation matrix before decomposing the rotation.

edit: fixed link

edit2: Just realized this pretty much repeats what you said in the issue title. Yes, we do need to normalize the rotation.

lasalvavida commented 6 years ago

to also allow shearing, by decomposing any 3x3 matrix into rotation scale rotation

It would be interesting, but not very high priority. Remember that we only convert to TRS on nodes that are targeted by animations.

The only case where this would matter is a matrix animation target that contains shear, which I don't think I've ever seen in the wild. It could happen, but IMO there are bigger conversion issues to fix right now (supporting morph targets, issues with COLLADA instancing, etc.).