Mutating a node's rotation array doesn't update the node

[rotu]

Describe the bug
The array returned by c.getRotation is mutable and live but updates aren't reflected

To Reproduce
Steps to reproduce the behavior:

1. Go to https://gltf.report/
2. Open a model (I used my favorite ducky)
3. Run this script and observe no visible change.

document.getRoot().getDefaultScene().listChildren().forEach(
  c=> {
    let s = c.getScale();
    s[0]*=2
    // c.setRotation(c.getRotation())
  }
)

4. Run it again with the commented out line uncommented and observe that the model gets stretched

Expected behavior
Either mutating the scale array has no effect or mutating the scale array updates the visible scene.

Versions:

• Version: v4.0.2
• Environment: Browser

Additional context
It's not clear from documentation whether getScale and setScale operate by value or by reference. It seems that getScale is by reference but setScale is by value.

It might be sufficient to mark the returned vec3 with the TypeScript readonly modifier and/or Object.freeze() the array so the distinction does not matter.

[donmccurdy]

Primarily this is a limitation in the https://gltf.report/ website — it does change detection, and can't tell changes were made if no setters were called, so it does not sync the scene in three.js.

Still, marking the returned array as readonly, or perhaps accepting a target array in the getter, could be interesting to make this explicit in future versions.

[rotu]

I figured the visual aspect of this was from the "Observer" code in @gltf-transform/view. But I don't think that, e.g. rebuilding the entire THREE scene from the GLTF graph would be a good fix here (even though it would solve the visual problem).

Another fix would be making the returned getter objects a custom class or a Proxy which would notify the observer, but I frankly hate it.

I think the right thing to do here is to use value semantics and not expose the mutable array object. I'm not a huge fan of the getters in three.js and ml-matrix that require you to pass in a container for the result; it tends to hurt readability IMO.

[donmccurdy]

What do you mean by "value semantics" – would readonly be an example of that?

I do agree that the getPosition( target )-style parameters used in three.js and gl-matrix hurt readability, but they pay off in performance, as allocations in the render loop can be a show-stopper. But so far I've used them only in "hot" methods here, like Accessor#getElement.

[rotu]

What do you mean by "value semantics" – would readonly be an example of that?

Reference semantics: The getter and setter take and return an array object which has shared state with the node.

Value semantics: The getter and setter copy the rotation so there is no shared state after the getter/setter is called.

readonly is still reference semantics. A node can expose a readonly reference to its rotation that nevertheless gets mutated by the node itself.

I do agree that the getPosition( target )-style parameters used in three.js and gl-matrix hurt readability

I might call it getPositionInto so it doesn't seem like a plain getter.

they pay off in performance, as allocations in the render loop can be a show-stopper.

I think I have found that returning plain number-keyed objects {0:x, 1:y} is slightly faster than arrays. But then, I'm sure you have far more experience here, and the allocations are a bigger deal!

[rotu]

In experimenting with my own code, I'm becoming very fond of this convention:

getPosition(opt?: {into: vec4})

You get the simplicity of calling it as a regular getter plus the option name serves as inline documentation. And you can use it as a drop-in optimization to extract allocation from a loop.

e.g.

for (const mesh of meshes){
  let rot = mesh.getRotation()
  // ...
}

let temp = new Float32Array(4)
for (const mesh of meshes){
  let rot = mesh.getRotation({into: temp})
  // ...
}

const product1 = vec4.multiply(a, b)

const temp = [0, 0, 0, 0]
const product2 = vec4.multiply(a, b, {into: temp})