donmccurdy / glTF-Transform

glTF 2.0 SDK for JavaScript and TypeScript, on Web and Node.js.
https://gltf-transform.dev
MIT License
1.39k stars 149 forks source link

Non-unit quaternion yields strange results #1533

Open rotu opened 3 hours ago

rotu commented 3 hours ago

Describe the bug Calling setRotation with an incorrectly normalized quaternion results in some strange scaling and no warning.

To Reproduce Steps to reproduce the behavior:

  1. Go to gltf.report and load a model
  2. Enter this script in the box:
    document.getRoot().listScenes().forEach(
    s=>{
    s.listChildren().forEach(c=>{
    const newRoot = document.createNode();
    // note this has norm Math.hypot(0.5, 0, 0, 0.5) < 1
    newRoot.setRotation([0.5, 0, 0, 0.5]);
    newRoot.addChild(c);
    s.addChild(newRoot)
    })}
    )
  3. Click run a few times
  4. Observe that the model is not only rotated but squished toward the axis of rotation. (screenshots below)

Expected behavior One of the following should happen. I'm not sure which.

  1. An error is reported and the setRotation call fails.
  2. The setRotation call succeeds and scales the rotation to a unit quaternion before applying it. Optionally, a warning is reported.
  3. The setRotation call succeeds and treats the rotation as currently does. A warning is reported.

Additional context image image

donmccurdy commented 2 hours ago

@rotu The library does not validate inputs to core class setters. I think that would be too much, both from a maintenance and performance perspective. In many cases this means the glTF specification is necessary as an additional reference; I'd also welcome PRs expanding on the current documentation with mathematical and/or spec requirements:

https://gltf-transform.dev/modules/core/classes/Node#setRotation

rotu commented 54 minutes ago

The gltf spec doesn't really say what to do here except that exporters should avoid emitting animations with degenerate quaternions. The setMatrix setter does coerce its input to TRS, which is much more expensive than renormalization.

Incidentally, getMatrix/setMatrix can be used to convert a non-unit quaternion into a unit quaternion and a scale:

let n = document.createNode();
n.setRotation([0.5,0,0,0.5])
n.setMatrix(n.getMatrix())
console.log(n.getTranslation()) // [0, 0, 0]
console.log(n.getRotation()) // [0.3826834323650897, 0, 0, 0.9238795325112867] 
console.log(n.getScale()) // [1, 0.7071067811865476, 0.7071067811865476] 

gltf-viewer reports validation errors (ROTATION_NON_UNIT) and maybe that's the better intervention.

image

zeux commented 35 minutes ago

FWIW glTF spec does say:

rotation is a unit quaternion value, XYZW, in the local coordinate system, where W is the scalar

It is not using the normative language (e.g. MUST), but there are a lot of cases in the spec where it states the restriction but happens to omit the normative language (with occasional spec updates to make it more explicit).