three.js icon indicating copy to clipboard operation
three.js copied to clipboard

Rework updateWorldMatrix / scene.autoUpdate

Open OndrejSpanel opened this issue 4 years ago • 20 comments

I think I would perhaps be able to implement a smarter system of handling matrixWorldNeedsUpdate based on a tri-state value ("no update needed", "I need update", "some children need update"). With such implementation it would not be necessary to always traverse all objects in scene.updateMatrixWorld, which I think is the main performance reason why people are disabling scene.autoUpdate and resort to calling updateMatrixWorld manually.

I think the implementation would be mostly backwards compatible (i.e. handling updateMatrixWorld manually would still be possible, and the interface would not change when relying on automatic updates).

@WestLangley @mrdoob @Mugen87 Before I start experimenting with this, I would like to know what is the general sentiment here regarding such change. Would it be welcome, or do you prefer the things to stay as they are? If you feel this is a step in right direction, are there any special requirements / restrictions you would require from such change?

OndrejSpanel avatar Mar 02 '21 12:03 OndrejSpanel

I think such change would need to have big wins on CPU usage. So we'd need to see performance comparisons in a different set of scene graphs.

mrdoob avatar Mar 02 '21 13:03 mrdoob

Related #14138, #18344, #20220 (and actually many more^^).

I think you should closely study these discussions first before you try something new.

Mugen87 avatar Mar 02 '21 13:03 Mugen87

The point of the system I am suggesting is not only reducing updateMatrix computations, but especially reducing time spent in scene traversal in with scene.autoUpdate enabled in scenes with many (>10 000) static objects. A JSFiddle demonstrating the motivation:

https://jsfiddle.net/OndrejSpanel/u34zca9w/18/

updateMatrixWorld timing on Chrome / Win10 x64/ Intel 3.6Ghz quad core, timing done in Chrome performance tools:

enableMatrixUpdates enableSceneUpdates time in updateMatrixWorld
true true 5-6 ms
false true 2-2.5 ms
false false N/A

The configuration enableMatrixUpdates = false and enableSceneUpdates = true is what is normally used in the sample https://threejs.org/examples/webgl_performance_static.html. Even in this sample with 7700 objects it should be possible to save 0.5 ms of CPU time on my system (and 7700 objects in not very much). Moreover, it would be no longer necessary to disable scene.matrixAutoUpdate, which the sample currently does.

OndrejSpanel avatar Mar 02 '21 15:03 OndrejSpanel

I see 👍

How would the API look like?

mrdoob avatar Mar 02 '21 16:03 mrdoob

I hope to be able to keep the API unchanged, only to introduce some internal variable to track the tri-state. I hope to be able to maintain backward compatibility either directly, or by replacing matrixWorldNeedsUpdate with a property (getter and setter) and there may be a new property allowing the access to the tri-state.

If you think this is something which might prove interesting, I will try to prepare some proof of concept implementation.

OndrejSpanel avatar Mar 02 '21 17:03 OndrejSpanel

If you need to automatically detect matrices update in your solution I would recommend to read through especially this long (sorry!) thread #14138 as @Mugen87 suggested. We tried the detection without messing the code and without overhead of updating matrices for dynamic objects, but ended up giving it up because of Three.js flexible API and we wait for making matrices private for now.

takahirox avatar Mar 03 '21 02:03 takahirox

...and we wait for making matrices private for now.

Many users set the object matrix directly in their app. It is a feature that has been supported since the inception of three.js, and it is a feature we must continue to support.

WestLangley avatar Mar 03 '21 03:03 WestLangley

I've been thinking of adding a new method Object3D.setMatrix() if users accept. It makes easier to detect matrices update and keeps direct matrix update feature.

Many users

This is just my guess but I guess major use cases are updating position/rotation(quaternion)/scale, not directly updating matrix. I know there actually be users or libs directly updating matrices so I don't opposed to keep the feature tho. Just my two cents.

takahirox avatar Mar 03 '21 03:03 takahirox

World matrix computations

I am more concerned about the scene traversal and world matrix computations with scene.autoUpdate.

Many users set the object matrix directly in their app.

When they do, I think they:

  • set matrixAutoUpdate = false, as otherwise the matrix would be overwritten from position / quaternion
  • set matrixWorldNeedsUpdate = true or call updateMatrixWorld() on their matrix update, as otherwise the world matrix would not be computed properly

It should be possible to verify those assumptions if there is any source / example demonstrating such use.

Automatic matrix updates

Regarding a full automatic mode with matrixAutoUpdate = true, I think understand the difficulties of #14138 with detecting position / quaternion changes. I would like to start with something simpler: I would not try to differentiate between reads and writes, any access to the position / quaternion by the user would be assumed to change it and set the matrix dirty to be recomputed during the scene render. I think users are not reading positions of most objects often, therefore this should be a win for a typical scene. This would be done by wrapping current position and quaternion in a property (getter). This would be a bit similar to the way quaternions wrap their inner fields to recompute Euler on change, but I do not feel bold enough to do the same for Vector3 - I would rather leave it unchanged.

I would introduce read API (I will call it positionRead, quaterionRead for now) and use this API in the renderer wherever possible, so that the renderer itself does not trigger any matrix computations - it certainly needs to be able to read those fields. The API could be made available to users as well, so that they can read those properties in a performant way.

Even for users not using positionRead, quaterionRead the situation would not be worse than it is now - both matrices and world matrices are always recomputed for them when they use matrixAutoUpdate = true. Users using matrixAutoUpdate = false would not be affected, for such users I would rely on the matrixUpdate() call.

Implement smart world matrix computations first

Even if the Automatic matrix updates prove to be impossible or a change which is not welcome, I think the first part (smart scene traversal in updateMatrixWorld) is worth implementing. That way it would still require users to call updateMatrix(), which is not convenient, but working.

OndrejSpanel avatar Mar 03 '21 07:03 OndrejSpanel

I've been thinking of adding a new method Object3D.setMatrix() if users accept.

There is already a PR for this, see #18344.

I'm actually not a fan of promoting matrix based workflows because other (game) engines don't do that, too. A good example for this is Unity. However, I'm okay with accepting Object3D.setLocalMatrix() as a backwards-compatible option if Object3D.matrix would be private instead. Such a setup would easily open up many optimization options.

I would introduce read API (I will call it positionRead, quaterionRead for now) and use this API in the renderer wherever possible, so that the renderer itself does not trigger any matrix computations - it certainly needs to be able to read those fields. The API could be made available to users as well, so that they can read those properties in a performant way.

TBH, this sounds quite confusing to me.

Mugen87 avatar Mar 03 '21 10:03 Mugen87

For a purpose of optimizations like this it would be better to encapsulate position / quaternion access, instead of allowing users to modify it directly (exposing a data member directly is always limiting). I am afraid an API change drastic like this is out of question at this point, therefore I try to come up with something backward compatible, even if suboptimal. An alternative would be to encapsulate x. y, z in the Vector3 instead, the way Quaternion already does it, and use a on-change callback to be notified of write access. I would prefer such solution, but I am afraid there is higher chance of breaking something (some performance implications or some backwards compatibility issues).

OndrejSpanel avatar Mar 03 '21 11:03 OndrejSpanel

An alternative would be to encapsulate x. y, z in the Vector3 instead, the way Quaternion already does it, and use a on-change callback to be notified of write access. I would prefer such solution, but I am afraid there is higher chance of breaking something (some performance implications or some backwards compatibility issues).

This was already tried by #14138.

Mugen87 avatar Mar 03 '21 11:03 Mugen87

For the matrix API: being able to apply a matrix directly is really needed for a lot of cases. Having a setLocalMatrix(input: Matrix4) makes total sense to me, same for the getter: getLocalMatrix(out: Matrix4).

I was also thinking about something: can't we, at least in the meantime, just make updateWorldMatrix return a boolean to avoid re-computing the matrices for nothing? Something like:

updateWorldMatrix( updateParents, updateChildren ) {

		const parent = this.parent;
		
		let dirty = this.matrixWorldNeedsUpdate;

		if ( updateParents === true && parent !== null ) {

			const parentDirty = parent.updateWorldMatrix( true, false );
                        dirty = dirty || parentDirty;

		}

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

		      if ( this.parent === null ) {
      
			      this.matrixWorld.copy( this.matrix );
      
		      } else {
      
			      this.matrixWorld.multiplyMatrices( this.parent.matrixWorld, this.matrix );
      
		      }
		      this.matrixWorldNeedsUpdate = false;

                }

		// update children

		if (dirty &&  updateChildren === true ) {

			const children = this.children;

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

                                children[ i ].matrixWorldNeedsUpdate = true;
				children[ i ].updateWorldMatrix( false, true );

			}

		}
		
		return dirty;

}

This would allow the methods Object3D.getWorld*() to at least not re-compute anything when it's not needed?

DavidPeicho avatar Feb 23 '22 11:02 DavidPeicho

What would be the problems in the following approach?

  1. Make Object3D.matrix read-only, introduce Object3D.setMatrix(), which sets the local matrix and maybe decomposes it into object's position, rotation, and scale (not setting matrixNeedsUpdate), and sets matrixWorldNeedsUpdate to true (for the object only).
  2. If matrixNeedsUpdate is false, set it to true only when its position/rotation/quaternion/scale are updated (for rotation and quaternion using onUpdateCallback, for scale -- maybe use a setter, for position -- either add Vector3.onUpdateCallback, as in https://github.com/mrdoob/three.js/pull/14138) (preferred), or a dirty flag similar to as it was done in comments of that PR). 2.5. We can actually make matrixNeedsUpdate an array of three flags -- one for position, one for rotation, one for scale. This can possibly provide some performance benefits, because we will be able to not use Matrix4.compose(), but possibly faster methods based on Matrix3.translate()/rotate()/scale() (unsure about this).
  3. When Object3D.updateMatrixWorld( parentWasUpdated ) is called, it updates .matrix if .matrixNeedsUpdate is true, updates .matrixWorld if parentWasUpdated is true, .matrix has updated, or .matrixWorldNeedsUpdate is true, and then calls the function for all object's direct children (with parentWasUpdated set to whether object's .matrixWorld was updated).

Note that in this approach .matrixWorldNeedsUpdate remains only for situations where .matrix should not be updated by the function, but .matrixWorld should (i.e. when the local matrix was set manually by the user). (By making .matrixNeedsUpdate not a boolean, but enum, we can merge them into a single flag.) Also note that these are only partial optimizations that could be applied -- this approach does not interfere with optimizations described in https://github.com/mrdoob/three.js/issues/21387#issue-819990352 and https://github.com/mrdoob/three.js/issues/25115#issue-1490154944.

Suggested PRs order:

  • [ ] Add Vector3.onUpdateCallback(),
  • [ ] Make Object3D.matrix read-only, introduce Object3D.setMatrix() (maybe without decomposing, or maybe we can make it an option),
  • [ ] Main PR -- add Object3D.matrixNeedsUpdate and rework Object3D.updateMatrixWorld(),
  • [ ] Maybe merge Object3D.matrixNeedsUpdate and Object3D.matrixWorldNeedsUpdate into one enum-valued flag ("no updates needed", "update only world matrix", "update local and world matrices"),
  • [ ] Maybe remove Object3D.matrixAutoUpdate and just tell the user to set Object3D.matrixNeedsUpdate = false when needed,
  • [ ] Maybe make Object3D.matrixNeedsUpdate an array of three flags, one for position, rotation, and scale (but this requires to make Object3D.setMatrix() decomposing -- otherwise it will break if user calls setMatrix() and then changes position/rotation/scale).

LeviPesin avatar Dec 12 '22 05:12 LeviPesin

@Mugen87 @mrdoob @WestLangley @takahirox I can try to make a PR with that approach?

LeviPesin avatar Dec 14 '22 14:12 LeviPesin

TBH, I don't think yet another PR in this context is helpful. It's more important to agree on a strategy first.

Mugen87 avatar Dec 14 '22 14:12 Mugen87

I'm again rereading my proposal in https://github.com/mrdoob/three.js/issues/21387#issuecomment-1345878019 and I can't find any major problems with it (except a small API change -- Object3D.matrix.set( ... ) -> Object3D.setMatrix( ... ) and possibly changing semantics of the flags). But I would like to hear some feedback...

LeviPesin avatar Feb 17 '23 10:02 LeviPesin

And one of the benefits of this proposal is that it allows to make Object3D.matrix not completely unwritable, but writable by a Object3D.setMatrix() method (which can even work without decomposing matrix). And still allows to make serious performance improvements. And is quite simple. But one of the main benefits is that it still leaves room for other improvements -- reducing scene traversal (https://github.com/mrdoob/three.js/issues/21387#issue-819990352) and implementing monomorphism (https://github.com/mrdoob/three.js/issues/25115#issue-1490154944) -- it doesn't interfere with them.

LeviPesin avatar Feb 17 '23 10:02 LeviPesin

I can test my approach and write how much performance it produce.

LeviPesin avatar Feb 17 '23 11:02 LeviPesin

@Mugen87 From https://github.com/mrdoob/three.js/pull/27261#discussion_r1421784215 it seems like that you generally agree with my proposal above?

LeviPesin avatar Jan 14 '24 12:01 LeviPesin