updateMatrixWorld() --> ensureMatrices()#32962
Open
PoseidonEnergy wants to merge 5 commits intomrdoob:devfrom
Open
updateMatrixWorld() --> ensureMatrices()#32962PoseidonEnergy wants to merge 5 commits intomrdoob:devfrom
updateMatrixWorld() --> ensureMatrices()#32962PoseidonEnergy wants to merge 5 commits intomrdoob:devfrom
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related: #32894, #21387, #14138, #18344, #20220, #25115, #27261, #14543
This PR is an attempt to fix issues surrounding updateMatrixWorld() and how it is used internally. It also gives more control to users over the world matrix update process that is very much needed. This PR does not address performance issues related to updateMatrixWorld(). It only fixes single-responsibility problems and does not change internal logic.
👍
updateMatrix()is all goodCurrently, and true to its name,
updateMatrix()updates the local matrix only. It works even ifmatrixAutoUpdateisfalse, which is what I would expect -- if the user has opted out of "automatic updates" by settingmatrixAutoUpdatetofalse, the user can still manually update the local matrix when they feel it is necessary.Because
updateMatrix()has a single responsibility (to calculate the local matrix), this means methods that overrideupdateMatrix()only need to worry about doing just that, and nothing else.❓
updateMatrixWorld()is a different storyUnfortunately,
updateMatrixWorld()does not follow the same pattern asupdateMatrix():matrixWorldAutoUpdateisfalse, it is impossible for a user to update the world matrix by just calling the method alone. They must first setmatrixWorldAutoUpdatetotrue, call the method, then setmatrixWorldAutoUpdateback tofalse. This deviates from the behavior ofupdateMatrix()as described above. This is surprising.Here's an example of where this behavioral inconsistency between
updateMatrix()andupdateMatrixWorld()is an issue. In the following snippet, the call toobject.updateMatrixWorld()doesn't do anything, becausematrixAutoUpdatewas set tofalsefor all of the objects in the scene. This means thatmatrixWorldNeedsUpdatenever gets set totrue, which means the world matrix is never actually calculated. The author of this example probably assumed it would though:three.js/examples/webgl_math_obb.html
Lines 211 to 212 in 1f2fea7
matrixAutoUpdateistruewithout adding code to unsetmatrixAutoUpdatebefore calling, and resettingmatrixAutoUpdateafter calling. This is inconvenient.You can see the local matrix get double-calculated in several places in three.js:
three.js/examples/jsm/tsl/shadows/TileShadowNode.js
Lines 385 to 386 in 1f2fea7
updateMatrixWorld()calculate the local and world matrices, it also traverses through all descendants to do the same to them. BecauseupdateMatrixWorld()tries to do three things at once, it is more complicated to override the method, because each override must also implement the local matrix update and descendant update logic.😔 Override Sadness
Because of the issues with overriding
updateMatrixWorld()mentioned above, you have things like this in three.js. In the following excerpt fromGyroscope.js, notice that all this method wants to do is override the world matrix calculation, but becauseObject3D.updateWorldMatrix()currently does three different things,Gyroscope.jshas to re-implement the local matrix calculation and child traversal code in addition to the world matrix calculation:three.js/examples/jsm/misc/Gyroscope.js
Lines 35 to 74 in 59335f4
The following excerpt is another issue caused by
updateMatrixWorld()doing more than it should. InCamera.js, notice that bothupdateMatrixWorld()andupdateWorldMatrix()have to be overridden (with the same identical code). This is because the actual world matrix calculation is not isolated into its own method:three.js/src/cameras/Camera.js
Lines 112 to 150 in 59335f4
♻️
updateWorldMatrix()is justupdateMatrixWorld(true)with extra argumentsRight now,
updateWorldMatrix()is identical toupdateMatrixWorld(), except that it ignoresmatrixWorldNeedsUpdate, and has the ability to control whether parents or descendants are traversed. If we simply added the argumentsupdateParents,updateChildren, andrespectAutoUpdateFlagstoupdateMatrixWorld(), then we can makeupdateWorldMatrix()internally callupdateMatrixWorld(true, ...args).💎 What this PR does
updateMatrixWorld()ONLY calculate the world matrix and ignorematrixWorldAutoUpdate, to be consistent withupdateMatrix().updateWorldMatrix(updateParents, updateChildren)into a method namedensureMatrices(force=false, updateParents=false, updateChildren=true, respectAutoUpdateFlags=true)so we recycle existing code, and also have control over whether we want to respect the "automatic update" flags.updateMatrixWorld(force)with calls toensureMatrices(force).🔨 Just 1 tiny breaking change
updateMatrixWorld(...args)should instead callensureMatrices(...args). The default arguments toensureMatrices()are set to match the old behavior ofupdateMatrixWorld(), so the only thing to update would be the function name. Best of all, there is no change required for users callingupdateWorldMatrix().🎯 Old versus new ways of calculating matrices
scene.updateMatrixWorld();scene.ensureMatrices();matrixAutoUpdateandmatrixWorldAutoUpdateflags, but ignore thematrixWorldNeedsUpdateflag.o.updateWorldMatrix();o.updateWorldMatrix();matrixWorldAutoUpdateisfalse.o.matrixWorldAutoUpdate = true;o.updateWorldMatrix();o.matrixWorldAutoUpdate = false;o.updateMatrixWorld();o.matrixAutoUpdate = false;o.updateWorldMatrix();o.matrixAutoUpdate = true;o.updateMatrixWorld();matrixWorldAutoUpdateflags arefalse.o.traverse(c => {c.matrixAutoUpdate = false;c.matrixWorldAutoUpdate = true;});o.updateWorldMatrix(false, true);o.traverse(c => {c.matrixAutoUpdate = true;c.matrixWorldAutoUpdate = false;});o.updateWorldMatrix(false, true, false, true, false);💲 API Surface & Thoughts
Here is the API surface in this PR:
As you can see, this refactor trivializes previously cumbersome matrix operations, and fixes the confusing overridability issues. Let me know of any questions you may have.