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.

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

Object3D: Fix world matrix update bug in .attach() #20759

Merged
merged 2 commits into from
Feb 6, 2021

Conversation

takahirox
Copy link
Collaborator

@takahirox takahirox commented Nov 25, 2020

Description

This PR fixes a bug of world matrix update in Object3D.attach() and adds a unit test

Currently Object3D.attach() moves an object to under a new parent while keeping its world matrix values by doing the following.

  1. Calculate the object's new position/quaternion/scale/local matrix
  2. Call Object3D.updateWorldMatrix()
  3. Add an object to the new parent

But Object3D.updateWorldMatrix() should be called after adding an object to the new parent. Otherwise, the object's world matrix will be wrong because the world matrix is calculated as the object is under the old parent.

This PR moves Object3D.updateWorldMatrix() after the line adding an object to the new parent.

You can check this change works by using the unit test this PR adds. Without the change in Object3D.attach() the test fails while with the change the test passes.

@takahirox
Copy link
Collaborator Author

takahirox commented Nov 26, 2020

No Object3D.updateWorldMatrix() call may be another option because object.matrixWorld values are maintained. (I thought calling it is for safety.)

Copy link
Collaborator

@WestLangley WestLangley left a comment

Choose a reason for hiding this comment

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

Note that object.add() does not update the world matrices. So is that a 'bug', too?

I think this PR is an improvement, in any event.

src/core/Object3D.js Outdated Show resolved Hide resolved
@takahirox
Copy link
Collaborator Author

Note that object.add() does not update the world matrices. So is that a 'bug', too?

Yeah, .add() doesn't update the world matrices. I speculate it's intentional for optimization. Because in most of use cases user doesn't directly care world matrices. And the world matrices are efficiently updated in renderer (if scene.autoUpdate is true). So updating the world matrix at the time an object is added may be inefficient.

Anyways,

I think this PR is an improvement, in any event.

I think so too. Calculating the world matrix as the object is under an old parent doesn't make sense.

@takahirox
Copy link
Collaborator Author

takahirox commented Nov 28, 2020

I replaced object.updateWorldMatrix( false, false ) with object.updateWorldMatrix( false, true ) to update children's world matrices.

Another option is cutting off oblect.updateWorldMatrix() call because the world matrix should be maintained so .attach() should not have an effect to the object's and children's world matrices.

What is the original purpose of calling it in .attach()? Matrix4.decompose() is used inside so that we take care of the case where .decompose result will be wrong due to parents' non-uniform scale as mentioned in #3845? In that case, the world matrix values can change then yes we need to call object.updateWorldMatrix( false, true ).

@mrdoob mrdoob added this to the r126 milestone Feb 6, 2021
@mrdoob mrdoob merged commit 27fd915 into mrdoob:dev Feb 6, 2021
@mrdoob
Copy link
Owner

mrdoob commented Feb 6, 2021

Thanks!

@takahirox takahirox deleted the FixAttach branch February 6, 2021 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants