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: matrixNeedsUpdate and matrix/orientation optimizations #28534

Draft
wants to merge 17 commits into
base: dev
Choose a base branch
from

Conversation

CodyJasonBennett
Copy link
Contributor

@CodyJasonBennett CodyJasonBennett commented Jun 1, 2024

Related issue: #21387, #25115

Description

Implements matrixNeedsUpdate set from mutations to position/quaternion/rotation/scale properties in Object3D, reducing the update depth and frequency of local matrix updates and consequent world matrix updates to descendants (related: #14138, #28533). I've also removed the _onChangeCallback system in math classes and synchronized rotation and quaternion locally in Object3D once on read (versus unconditionally on write), where it's now possible to skip this conversion if users keep to one orientation property at a time. This is not something I've seen regarded anywhere than maybe #14044 (comment), but should be a nice benefit for both maintenance and performance outside of matrices. Most of the diff in this PR is from removing prior hacks.

World matrices are also no longer updated unconditionally, as we will only set matrixWorldNeedsUpdate when the local matrix (or parent world matrix) updates. This removes the primary use of matrixWorldAutoUpdate for those looking to squeeze out performance, as these optimizations are performed automatically. It's possible to further improve updateWorldMatrix in this regard so it can skip updates to ancestors -- #28533 (comment). The introduction of matrixNeedsUpdate is similarly useful for polling in user-land, which I think is a reasonable compromise for those hoping to detect updates in the scene-graph (e.g. updating bounding volumes or even using the scene as a virtual API for BatchedMesh).

Future work could include optimizing the cost of matrix composition itself (#14138 (comment)). Although position is a simple mutation, scale would be more involved by dividing/multiplying new/old scales, and quaternion the worst-case-scenario with a complete update to the upper-left 3x3 matrix. It's possible this can be performed in user-land, or we can reduce this check to matrix.setPosition or matrix.compose depending on whether orientation/scale change. This is not something I'll consider further, as I believe this is best handled in user-land where we can compile WASM etc., but I've outlined a solution below.

Partial Matrix Composition Example
// Partial Matrix4.compose https://github.com/mrdoob/three.js/blob/dev/src/math/Matrix4.js
const m = this.matrix.elements;

if ( this.quaternionChanged ) {

	const x = quaternion.x, y = quaternion.y, z = quaternion.z, w = quaternion.w;
	const x2 = x + x,	y2 = y + y, z2 = z + z;
	const xx = x * x2, xy = x * y2, xz = x * z2;
	const yy = y * y2, yz = y * z2, zz = z * z2;
	const wx = w * x2, wy = w * y2, wz = w * z2;

	const sx = scale.x, sy = scale.y, sz = scale.z;

	m[ 0 ] = ( 1 - ( yy + zz ) ) * sx;
	m[ 1 ] = ( xy + wz ) * sx;
	m[ 2 ] = ( xz - wy ) * sx;

	m[ 4 ] = ( xy - wz ) * sy;
	m[ 5 ] = ( 1 - ( xx + zz ) ) * sy;
	m[ 6 ] = ( yz + wx ) * sy;

	m[ 8 ] = ( xz + wy ) * sz;
	m[ 9 ] = ( yz - wx ) * sz;
	m[ 10 ] = ( 1 - ( xx + yy ) ) * sz;

} else if ( this.scaleChanged ) {

	// NOTE: this relies on either a cache or decomposing into two parts when orientation is changed:
	// - dividing old scale when orientation changes (either Quaternion or Euler)
	// - multiplying new scale when updating matrix
	const sx = ( 1 / this.oldScale.x ) * scale.x;
	const sy = ( 1 / this.oldScale.y ) * scale.y;
	const sz = ( 1 / this.oldScale.z ) * scale.z;

	m[ 0 ] *= sx;
	m[ 1 ] *= sx;
	m[ 2 ] *= sx;

	m[ 4 ] *= sy;
	m[ 5 ] *= sy;
	m[ 6 ] *= sy;

	m[ 8 ] *= sz;
	m[ 9 ] *= sz;
	m[ 10 ] *= sz;

}

// NOTE: might be best to do so unconditionally and leave tracking with orientation
if ( this.positionChanged ) {

	m[ 12 ] = this.position.x;
	m[ 13 ] = this.position.y;
	m[ 14 ] = this.position.z;

}

Copy link

github-actions bot commented Jun 1, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
685.1 kB (169.6 kB) 685 kB (169.4 kB) -67 B

🌳 Bundle size after tree-shaking

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

Filesize dev Filesize PR Diff
462 kB (111.4 kB) 461.8 kB (111.4 kB) -140 B

@CodyJasonBennett CodyJasonBennett marked this pull request as draft June 1, 2024 04:26
@CodyJasonBennett CodyJasonBennett marked this pull request as ready for review June 1, 2024 06:01
@CodyJasonBennett CodyJasonBennett changed the title Object3D: matrixNeedsUpdate and flags for matrix/orientation sync Object3D: matrixNeedsUpdate and change detection for matrix/orientation sync Jun 1, 2024
@CodyJasonBennett CodyJasonBennett changed the title Object3D: matrixNeedsUpdate and change detection for matrix/orientation sync Object3D: matrixNeedsUpdate and change detection for matrix/orientation Jun 1, 2024
src/core/Object3D.js Outdated Show resolved Hide resolved
@CodyJasonBennett CodyJasonBennett changed the title Object3D: matrixNeedsUpdate and change detection for matrix/orientation Object3D: matrixNeedsUpdate and matrix/orientation optimizations Jun 4, 2024
Comment on lines +166 to +170
listenToProperties( position, [ 'x', 'y', 'z' ], undefined, onWrite );
listenToProperties( rotation, [ 'x', 'y', 'z', 'order' ], onRotationRead, onRotationWrite );
listenToProperties( quaternion, [ 'x', 'y', 'z', 'w' ], onQuaternionRead, onQuaternionWrite );
listenToProperties( scale, [ 'x', 'y', 'z' ], undefined, onWrite );
listenToMethods( matrix, onMatrixChange );
Copy link
Contributor Author

@CodyJasonBennett CodyJasonBennett Jun 4, 2024

Choose a reason for hiding this comment

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

listenToProperties adds an on-read and on-write callback to named keys (not a deopt), which we use here to set matrixNeedsUpdate when transform properties are modified. On-read is used to convert quaternion/rotation like before, but only if their counterpart is dirty when read, preventing many unneeded conversions on mutation (which can be many times). Notably, if users stick to quaternion, they will avoid this conversion entirely. This is nice so we don't have to modify math classes like before.

listenToMethods I have as a special case for the local matrix whose members would be deopt if I just listened to all elements in its array. This is because array accessors are not named and unstable, so v8 would need to create a layer of indirection to make them stable. We want matrixWorldNeedsUpdate to be set whenever the local matrix updates to preserve existing behavior, so I instead handle the cases where local matrix is modified by its methods (which are named keys), and don't handle the case where matrix.elements is mutated directly (matrixWorldNeedsUpdate has to then be set by the user). This keeps us on the fast path for common usage without breaking APIs who expected a 1-1 relationship between local and world matrix updates.

Comment on lines -110 to +111
this.matrix = camera.matrixWorld;
this.matrixAutoUpdate = false;
this.matrixWorld = camera.matrixWorld;
this.matrixWorldAutoUpdate = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This relied on the fact that world matrices were always updated even transiently. Since #28533, we can do this instead.

@clibequilibrium
Copy link

@CodyJasonBennett might be a breaking change in Euler changing members from _x, _y, _z, _order to x ,y ,z , order for apps that store Euler in JSON format. It saves the data to _x , _y format but with this PR it will expect x , y, z, order. What's your take on it ?

@CodyJasonBennett
Copy link
Contributor Author

CodyJasonBennett commented Jul 6, 2024

I know Quaternion implements toJSON with toJSON() { return this.toArray() } which could work here, but we can simply implement something that outputs the old structure.

{
    "isEuler": true,
    "_x": 0,
    "_y": 0,
    "_z": 0,
    "_order": "XYZ"
}

Regardless of approach -- including caching or any user-land optimizations (e.g. WASM or potentially all of the above) -- we need to address the overhead from _onChangeCallback in Euler/Quaternion classes. That's most of the diff from this PR, and I think it should move to a separate PR as we clear this up.

@CodyJasonBennett CodyJasonBennett marked this pull request as draft July 6, 2024 15:56
@CodyJasonBennett CodyJasonBennett marked this pull request as ready for review July 6, 2024 22:16
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.

2 participants