Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove force flag and make Object3D.matrixWorldAutoUpdate work properly again #27261

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Remove force flag and make Object3D.matrixWorldAutoUpdate work properly again #27261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
772955d
Remove force flag set, which makes object3d.matrixWorldAutoUpdate red…
DolphinIQ Nov 25, 2023
237a984
Add matrixWorldAutoUpdate condition to obj.updateMatrixWorld
DolphinIQ Nov 27, 2023
16314fc
Restore tests
DolphinIQ Nov 27, 2023
cfb0516
restore volume stuff
DolphinIQ Nov 27, 2023
e49af41
fix tests
DolphinIQ Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions examples/jsm/objects/Refractor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class Refractor extends Mesh {

const virtualCamera = this.camera;
virtualCamera.matrixAutoUpdate = false;
virtualCamera.matrixWorldAutoUpdate = false;
virtualCamera.userData.refractor = true;

//
Expand Down
5 changes: 5 additions & 0 deletions src/cameras/StereoCamera.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@ class StereoCamera {
this.cameraL = new PerspectiveCamera();
this.cameraL.layers.enable( 1 );
this.cameraL.matrixAutoUpdate = false;
this.cameraL.matrixWorldAutoUpdate = false;

this.cameraR = new PerspectiveCamera();
this.cameraR.layers.enable( 2 );
this.cameraR.matrixAutoUpdate = false;
this.cameraR.matrixWorldAutoUpdate = false;

this._cache = {
focus: null,
Expand Down Expand Up @@ -93,6 +95,9 @@ class StereoCamera {
this.cameraL.matrixWorld.copy( camera.matrixWorld ).multiply( _eyeLeft );
this.cameraR.matrixWorld.copy( camera.matrixWorld ).multiply( _eyeRight );

this.cameraL.matrixWorldInverse.copy( this.cameraL.matrixWorld ).invert();
this.cameraR.matrixWorldInverse.copy( this.cameraR.matrixWorld ).invert();

}

}
Expand Down
4 changes: 1 addition & 3 deletions src/core/Object3D.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ class Object3D extends EventDispatcher {

if ( this.matrixAutoUpdate ) this.updateMatrix();

if ( this.matrixWorldNeedsUpdate || force ) {
if ( this.matrixWorldNeedsUpdate || this.matrixWorldAutoUpdate === true || force ) {

if ( this.parent === null ) {

Expand All @@ -583,8 +583,6 @@ class Object3D extends EventDispatcher {

this.matrixWorldNeedsUpdate = false;

force = true;

}

// update children
Copy link
Collaborator

@Mugen87 Mugen87 Dec 10, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 );

}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@DolphinIQ DolphinIQ Dec 10, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@Mugen87 Mugen87 Dec 10, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Expand Down
2 changes: 2 additions & 0 deletions test/unit/src/core/Object3D.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -985,6 +985,8 @@ export default QUnit.module( 'Core', () => {

// -- matrixWorldAutoUpdate = false test

parent.matrixAutoUpdate = true;
child.matrixAutoUpdate = true;
parent.position.set( 3, 2, 1 );
parent.updateMatrix();
parent.matrixWorldNeedsUpdate = false;
Expand Down