Open rotu opened 1 month ago
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.
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.
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
.
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!
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})
Describe the bug The array returned by
c.getRotation
is mutable and live but updates aren't reflectedTo Reproduce Steps to reproduce the behavior:
Expected behavior Either mutating the scale array has no effect or mutating the scale array updates the visible scene.
Versions:
Additional context It's not clear from documentation whether
getScale
andsetScale
operate by value or by reference. It seems thatgetScale
is by reference butsetScale
is by value.It might be sufficient to mark the returned
vec3
with the TypeScriptreadonly
modifier and/orObject.freeze()
the array so the distinction does not matter.