Remove force flag and make Object3D.matrixWorldAutoUpdate work properly again

[DolphinIQ]

Remove force flag set from Object3D.updateMatrixWorld(), which makes Object3D.matrixWorldAutoUpdate useless.
This is a continuation of #27245

Motivation

I am setting object.matrixWorldAutoUpdate = false; in my app and updating the world matrices manually. However due to the issue, Object3D.matrixWorldAutoUpdate simply doesn't work.

Description

At the moment Object3D.matrixWorldAutoUpdate flag doesn't do anything. Its true by default (which is why it hasnt been caught yet I assume), but if you set it to false, the object will still have its world matrix recalculated every frame. Why? Because of force = true inside Object3D.updateMatrixWorld().

The Problem

So what happens?

1. Every frame WebGLRenderer.render() calls if ( scene.matrixWorldAutoUpdate === true ) scene.updateMatrixWorld();
2. Then scene.updateMatrixWorld() calls if ( this.matrixAutoUpdate ) this.updateMatrix();
3. scene.updateMatrix() composes the local matrix and sets this.matrixWorldNeedsUpdate = true;
4. Back inside scene.updateMatrixWorld the condition is called: if ( this.matrixWorldNeedsUpdate || force ) {, which will run because of point 3.
5. World matrix is calculated and most importantly for this PR: force = true; is called (for what reason, I do not know)
6. Next scene updates its children. How does it determine whether they should be updated? The check is performed:
   if ( child.matrixWorldAutoUpdate === true || force === true ) { - HERE WE SEE child.matrixWorldAutoUpdate DOES NOT MATTER! because force is true.
7. Scene calls child.updateMatrixWorld( force ); updating the entire scene graph, now with force flag set to true SO FROM NOW ON IT WILL ALWAYS BE TRUE FOR EVERY OBJECT IN THE SCENE GRAPH! And since it will always be passed down as true, the check from point 6. if ( child.matrixWorldAutoUpdate === true || force === true ) { will always run, making child.matrixWorldAutoUpdate not matter at all!

Here is a rundown in the Object3D matrix updates code, with steps above pointed out in the comments:

    updateMatrix() {
        this.matrix.compose( this.position, this.quaternion, this.scale );
        this.matrixWorldNeedsUpdate = true; // <-- 3rd point
    }

    updateMatrixWorld( force ) {
                // on 1st run "this" is Scene and "force" is false

        if ( this.matrixAutoUpdate ) this.updateMatrix(); // <-- 2nd point - sets 
        // `this.matrixWorldNeedsUpdate = true` which makes the next condition always run

        if ( this.matrixWorldNeedsUpdate || force ) { // <-- 4th point - `matrixWorldNeedsUpdate` is set
        // in `updateMatrix()` above, so condition always happens

            if ( this.parent === null ) {

                this.matrixWorld.copy( this.matrix );

            } else {

                this.matrixWorld.multiplyMatrices( this.parent.matrixWorld, this.matrix );

            }

            this.matrixWorldNeedsUpdate = false;

            force = true; // <-- 5th point - "force" is now true and will ALWAYS 
            // be true throughout this `updateMatrixWorld` traversal

        }

        // update children

        const children = this.children;

        for ( let i = 0, l = children.length; i < l; i ++ ) {

            const child = children[ i ];

            if ( child.matrixWorldAutoUpdate === true || force === true ) { // <-- 6th point - 
            // `child.matrixWorldAutoUpdate` now doesnt matter because `force` is true

                child.updateMatrixWorld( force ); // <-- 7th point - and now `force` will ALWAYS be true 
                // throughout this entire scene graph `updateMatrixWorld` traversal

            }

        }

    }

We see that in the current configuration as long as the root Scene object updates its local matrix, ALL of its descendants will have their world matrices updated, whether they like it or not (regardless of child.matrixWorldAutoUpdate. Here is JSfiddle proof: https://jsfiddle.net/fpb09am8/
Its very easy to test. Just do:

// Setup
const parent = new THREE.Mesh( geometry, material );

const child= mesh.clone();
child.matrixWorldAutoUpdate = false;
child.position.x = 2;

scene.add( parent);
parent.add( child );

// Render loop
parent.rotation.y = timeElapsed / 1000;

Here we see that even though child doesnt want to have its world matrix updated automatically (child.matrixWorldAutoUpdate = false) it still happens because the scene will force it on all its descendants as shown in the 7 points above.

Solution

Remove force = true; from inside Object3D.updateMatrixWorld. It is setting itself to true by itself, making auto update checks ineffective. It is confusing if you set force to false (or leave undefined) because it will turn to true in just 1 generation by itself anyway. It is not clear at all that its basically always true.
If I understand correctly from docs, force should be an external flag set by the users if they want it.

Also important: force forced all world matrix updates. Without it, objects that have matrixAutoUpdate = false will no longer update their world matrices, since matrixWorldNeedsUpdate wont be set to true (point 3.). This would for example break many Helpers since they have matrixAutoUpdate set to false. Necessity of objects' world matrix computation shouldnt rely solely on local matrix update, thus I also added an extra this.matrixWorldAutoUpdate === true check to the second if:

// if ( this.matrixWorldNeedsUpdate || force ) {
// ^-- above `if` turns into the `if` below --v
if ( this.matrixWorldNeedsUpdate || this.matrixWorldAutoUpdate === true || force ) {

This way world matrix recomputation also depends on matrixWorldAutoUpdate.
Now since force forced all world matrix updates previously and matrixWorldAutoUpdate is true by default, this should avoid breaking 99.99% of existing code.

[github-actions]

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev	Filesize PR	Diff
668.7 kB (166 kB)	668.9 kB (166 kB)	+248 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev	Filesize PR	Diff
449.8 kB (108.9 kB)	449.8 kB (108.9 kB)	+28 B

[DolphinIQ]

Omw to fix examples again

[mrdoob]

Do you know if these issues were introduced in #24028?

[DolphinIQ]

@mrdoob Doesnt seem like it. Cody introduced Object3D.matrixWorldAutoUpdate, but it already didnt work properly then. The reason why this test https://github.com/mrdoob/three.js/pull/24028/files#diff-c76fd3129caa0e2b681e901cf5b3ca42e76f646ec5420c94e166a0cc2eb23512R789 worked is because in a previous test above it, parent and children matrixAutoUpdate has been set to false which in turn, inside regular scene matrix traversal object.updateMatrixWorld(), made object.updateMatrix() not run and thus not set
this.matrixWorldNeedsUpdate = true; // <-- 3rd point
and thus the second if inside object.updateMatrixWorld() didnt run and didnt set the problematic force = true; which is the entire issue.

The bug has miraculously avoided detection in tests, because tests are run inline one after the other and default properties ( like object.matrixAutoUpdate & object.matrixWorldAutoUpdate ) arent reset after each test. I checked and once I add the defaults

parent.matrixAutoUpdate = true;
child.matrixAutoUpdate = true;

to Cody's -- matrixWorldAutoUpdate = false test, it fails, because force takes over the entire scene graph update and updates the child world matrix anyway.

Edit: @makc helped track this force change and seems like its very old, reaching 2011 or even older 😅 Although judging by the 👀 reaction, looks like you already found it haha 1e5fc98#r133473586
Seems like world matrix updates have been done automatically by the scene in the past, until Cody introduced Object3D.matrixWorldAutoUpdate, but it simply doesnt work at the moment due to the force bug (easily tested by the linked JSFiddle)

[DolphinIQ]

@Mugen87 I fixed the examples. Please have a look again when you have a moment. It would be great to have it in the upcoming release^^

[krispya]

I'm so glad this is getting attention. I thought I was going insane!

[Mugen87]

I had some time now to study the PR more closely (and matrixWorldAutoUpdate in general) and I think this PR goes in the right direction!

However, there is one thing that confuses me: The comment update children in this method is actually wrong. This should actually be called update descendants since when a world matrix changes, you want to update not just the world matrices of children but of all subsequent nodes in the hierarchy.

Hence, I think it was wrong to add child.matrixWorldAutoUpdate === true to this code block because it prevents updates that might be wanted further down the hierarchy. This PR puts the matrixWorldAutoUpdate check at the right place (further up in the method where the world matrix of a node is actually computed).

So how about writing the code like so now?

// make sure descendants are updated if required

const children = this.children;

for ( let i = 0, l = children.length; i < l; i ++ ) {

	const child = children[ i ];
	
	child.updateMatrixWorld( force );

}

[DolphinIQ]

Hi @Mugen87 , thanks for having a look!

Have you tested your code? Your argument about descendants makes sense conceptually, however in the current configuration I believe it would cause world matrix updates regardless of .matrixWorldAutoUpdate, because:

1. Scene calls updateMatrixWorld() on its descendants
2. Descendant checks descendant.matrixAutoUpdate and performs descendant.updateMatrix()
3. updateMatrix() sets descendant.matrixWorldNeedsUpdate = true
4. The next statement if ( this.matrixWorldNeedsUpdate || this.matrixWorldAutoUpdate === true || force ) { passes, once again regardless of descendant.matrixWorldAutoUpdate

In other words, as long as object has object.matrixAutoUpdate set to true, it will override object.matrixWorldAutoUpdate. If we wanted to go the way you propose and keep object.matrixWorldNeedsUpdate relevant, we would have to either:
a) eliminate descendant.matrixWorldNeedsUpdate = true from updateMatrix() or...
b) in the second if statement if ( this.matrixWorldNeedsUpdate || this.matrixWorldAutoUpdate === true || force ) { have this.matrixWorldAutoUpdate somehow override the this.matrixWorldNeedsUpdate flag if necessary.

My use-case (which has led to discovery of this bug) was setting skinned mesh's rootBone.matrixWorldAutoUpdate = false (rootBone.matrixAutoUpdate is left to default true) to prevent skeleton matrices updates and perform it manually when needed. The current way is convenient because setting .matrixWorldAutoUpdate on the root object automatically prevents world updates of its descendants. You might say it makes .matrixAutoUpdate obsolete because the local matrix wont be updated either, but I think there is no reason to update local matrix if we dont update world matrix, since it is the latter that is sent to the shader anyway.

Thus I believe it is the object.matrixWorldAutoUpdate that should take precedence over object.matrixAutoUpdate.

It would be less convenient (although maybe more logical? 🤔) if traversal of descendants happens regardless (in the case of your change), as then in order to prevent all world matrix updates, you'd have to first traverse the entire object and set .matrixWorldAutoUpdate to false on each and every descendant instead of just the root object.
I am not sure if it even makes sense for descendant to update world matrix

child.matrixWorld.multiplyMatrices( parent.matrixWorld, child.matrix );

if the parent.matrixWorld stayed the same. But maybe it makes sense for descendants deeper in the graph?
What would be some use-cases for this?🤔

[Mugen87]

I assumed matrixAutoUpdate and matrixWorldAutoUpdate are set to false at the same time if an app wants to control the update matrix process.

[Mugen87]

Side note: This entire thread is another example of why the flag based approach is cumbersome and too complicated. At some point, we should revisit the entire world matrix update computation and implement a solution based on dirty flags and caches and remove the update flags altogether. Even if this means some breaking changes like making Object3D.matrix private.

[DolphinIQ]

This entire thread is another example of why the flag based approach is cumbersome and too complicated.

It is cumbersome, but I've found this force = true flag removal to be a good workaround for now. How would that dirty flags and caches solution work?

I assumed matrixAutoUpdate and matrixWorldAutoUpdate are set to false at the same time if an app wants to control the update matrix process.

Then maybe it makes sense to merge these flags into one? Or maybe we could implement an enum-like option for matrices update to reduce the number of impossible states? Its generally better than having booleans for every option. Something like

const MATRIX_UPDATE_TEST_NAME_ENUM = {
    LOCAL_AND_WORLD: 0,
    ONLY_LOCAL: 1,
    NONE: 2,
};

object.matrixAutoUpdate = MATRIX_UPDATE_TEST_NAME_ENUM.NONE;

Also I would avoid making Object3D.matrix private. I know in more advanced performance-focused projects people omit position/rotation/scale properties and just deal with matrices directly.

[Mugen87]

How would that dirty flags and caches solution work?

The first step would be to introduce dirty flags in Vector3 and Quaternion so you know when position, quaternion or scale has been changed. Only then you recompute the local matrix which means matrixAutoUpdate could be removed.

Also I would avoid making Object3D.matrix private.

You could still set a matrix with a new method called Object3D.setMatrix(). It's just important that the matrix property itself isn't writable anymore otherwise we can't properly detect a state change.

We have tried to implement something like this in #14138 but stopped because of Object3D.matrix is writable. Besides, at that time I wanted to explore caching first since it requires no changes to the math classes. Turned out that caching is slower than the dirty flag approach.

[WestLangley]

@mrdoob said:

Do you know if these issues were introduced in #24028?

Yes. For the previous decade the code was correct -- at least as it pertains to the 'force' flag.