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

Performance optimization by optionally sharing matrix between child & parent #25142

Closed

Conversation

diarmidmackenzie
Copy link
Contributor

@diarmidmackenzie diarmidmackenzie commented Dec 14, 2022

Description

This is a somewhat speculative PR. It builds on #25114, to offer a couple of new functions on Object3D that can be used to substantially boost performance of updateMatrixWorld(), permitting more objects in a scene.

#25114 added a new class Object3DMatrixData, and a new tree of Object3DMatrixData objects that mirrors the existing scene graph of Object3Ds.

The motivation for this was to allow monomorphic (hence, higher performance) iteration through the scene graph when updating matrices in updateMatrixWorld().

However, the existence of this "matrix graph" that exists parallel to the "scene graph" opens up other possibilities for further performance gains. In particular one interesting option is to prune unecessary nodes from the "matrix graph", so that it is smaller, and hence faster to iterate through.

One context where this would be particularly valuable is in A-Frame. In A-Frame, every <a-entity> is instantiated as a THREE.Group, with additional child Object3Ds as required to instantiate Meshes, Lights etc. This means that the THREE.js scene graph for a typical A-Frame scene consists of a THREE.Group and a THREE.Mesh for each entity.

Typically the THREE.Group has a meaningful matrix, encoding position, rotation & scale, while the THREE.Mesh matrix is an identity matrix, with default position, scale & rotation.

This can create performance issues in some applications, particularly on updateMatrixWorld() e.g. Hubs-Foundation/hubs#5311

This PR provides 2 new methods on the Object3D:

  • shareParentMatrix() configures the Object3D to use it's parent's Object3DMatrixData, and trims this Object3D's Object3DMatrixData out of the "matrix graph".
  • restorePrivateMatrix() reverses the above operation.

Either A-Frame itself, or an A-Frame application, could call shareParentMatrix() on every THREE.Mesh that is a child of a THREE.Group, and does not itself have any position, rotation or scale.

If done across the scene, this would result in a substantial (up to 50%) reduction to the size of the "matrix tree" and thus a substantial gain (again, up to 50%) in performance of updateMatrixWorld()

When rendering the mesh, the renderer would use the matrix data from its parent, which would still result in the correct rendering position.

Applications could also make use of this in other specific situations where objects/entities are known to always take the identity/default transformation matrix.

** Current state of the PR **

  • Code is written to implement the new functionality. This should at least be sufficient to provide clarity on what is proposed.
  • Existing Unit Tests run, but the new functionality has not been tested.
  • Considering cases where multiple tiers of the bject tree could be fused together, and then separated agan, in various orders there are various complicated scenarios that may not be adequately handled by the existing code. Additional Unit Test and possibly bug fixes is needed here.
  • Apart from updateMatrixWorld() it will be necessary to ensure that all code that combines matrices up/down the scene graph is moved across to use the new "matrix graph". One example is updateWorldMatrix() and there may be others as well.
  • No documentation of new methods yet written.
  • And probably more... I'd be keen to get feedback on the core ideas of this PR + also on updateMatrixWorld Optimization using new Object3DMatrixData class #25114 (which it depends on) before doing further work on this.

@LeviPesin
Copy link
Contributor

I'm afraid this is a too specific use case... I think there are not many meshes with default position, rotation, and scale (which could benefit from this) normally, except in that use case you described.

This also broke unit tests as it creates infinite recursion when comparing objects.
Code left over from having started with a slightly different implementation for extending updateMatrixWorld().  Can be removed now.
@diarmidmackenzie
Copy link
Contributor Author

Caught up with latest proposed code on #25114

@diarmidmackenzie
Copy link
Contributor Author

diarmidmackenzie commented Dec 14, 2022

I'm afraid this is a too specific use case... I think there are not many meshes with default position, rotation, and scale (which could benefit from this) normally, except in that use case you described.

Maybe. Another use case where I find myself creating objects with default position / rotation & scale is where a 3D artist has already taken care of positioning & encoded it in the model's origin. That's quite common in my experience, as it's much easier for a 3D artist to line up multiple objects just as they want them in their 3D design environment, and just export, rather than having to separately communicate correct positioning of each object.

With this approach, many/most 3D objects can end up with the transform of their root THREE.Group just being the default position/rotation/scale.

@diarmidmackenzie
Copy link
Contributor Author

diarmidmackenzie commented Dec 15, 2022

For anyone interested, I've been experimenting with a version of this in super-three (the A-Frame fork of three) and A-Frame.

Branches here:
https://github.com/diarmidmackenzie/three.js/tree/super-r144-performance
and
https://github.com/diarmidmackenzie/aframe/tree/updateMatrixWorldPerf

Been using some A-Frame-ified versions of the THREE.js Benchmarks. Performance gains on updateMatrixWorld() are extremely promising... 4+ times speed-up...

image

Build here should be viable if anyone wants to try this out...
https://github.com/diarmidmackenzie/aframe/blob/updateMatrixWorldPerf/dist/aframe-master.min.js
or
https://cdn.jsdelivr.net/gh/diarmidmackenzie/aframe@updateMatrixWorldPerf/dist/aframe-master.min.js

@diarmidmackenzie
Copy link
Contributor Author

diarmidmackenzie commented Dec 15, 2022

Chrome perf profiling showing reduction in updateMatrixWorld() CPU usage in this example: (about 67% reduction)

https://diarmidmackenzie.github.io/instanced-mesh/examples/coloring-blocks/index.html

image

@takahirox This may be of interest to you.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 19, 2022

Thanks for your work on this but unfortunately this PR contains way to many substantial API changes that can not be applied to the library. Besides, sharing matrices between child and parent nodes is not something users should care about. Providing the opportunity to explicitly enable/disable matrix sharing with method calls is the wrong way, imo.

The goal has to be speeding up the world matrix computation process without affecting user level code as much as possible. Everything else is not acceptable regarding the maturity level of three.js. Looking at the changes to the examples, the PR does not fulfill this premise.

@Mugen87 Mugen87 closed this Dec 19, 2022
@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 19, 2022

Further explanation: We can't go for best possible performance in three.js's world matrix computations since we have to retain the current API as best as possible. So there will be a compromise. I suggest before filing any other PRs and issue, the first step is to dicsuss making Object3D.matrix private which would open up optimization possibilties (like dirty flags, caches) in a clean way.

@diarmidmackenzie
Copy link
Contributor Author

diarmidmackenzie commented Dec 19, 2022

Thanks for the feedback on this.

This point:

Looking at the changes to the examples, the PR does not fulfill this premise.

actually applies to #25114 (which this PR builds on), and turns out to be a fixable issue. I have pushed an update to that PR.

On this point:

Providing the opportunity to explicitly enable/disable matrix sharing with method calls is the wrong way, imo.

I have some sympathy with this - the code in this PR was intended to illustrate & explore breaking the alignment between the Object3D graph, and the Object3DMatrixData graph introduced by #25114. I'm not particularly attached to any particular mechanism for mediating the relationships between the two layers.

I do think there are potentially very significant performance gains to be made by relaxing the assumption that every Object3D needs have its own unique transform. But I'm happy to concede that more work would be needed to deliver this in a way that minimizes the level of interface change for existing users.

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.

None yet

3 participants