Detach unused pool elements from THREE.js scene

[diarmidmackenzie]

Description:

A PR based on the performance enhancement described here:
diarmidmackenzie/moonrider#3 (comment)

The scope of this has grown a little. I know you are pretty conservative about adding new components & API changes for A-Frame, so if you prefer me to dial back the scope of changes here, let me know.

To explain how I got from my original fix to this PR...

• Detaching entities from the scene & re-attaching might be valuable outside of the pool component, so better to provide a clean API on a-entity and handle the fiddly details (like window conditions around initialization) inside a-entity

• There are existing use cases for which the new API is actually a great fit. Use case described in visible component where you can toggle a group of entities in / out of the scene is actually better met by the new API. Making such elements invisible cuts any cost of the GPU, but still leaves some CPU cost for updateMatrixWorld() and raycasting. The new API would get rid of the CPU cost as well, as well as eliminating raycasting false positives, which have caused me problems in the past when using this pattern.

• Support for this use case requires a component, so that detachment from THREE.js scene can be encoded declaratively in HTML. Hence new attached component.

• It's important for raycasting to ignore object3Ds that are not part of the main scene graph: they are orphaned, and matrixWorld values are not meaningful. Hence updates to raycaster to explicitly ignore them.

• For raycaster auto-refresh to work, new events were needed to indicate detachment from scene & re-attachment to it. I had a slight concern about perf impacts here, but I think any impacts that do occur can be mitigated by disabling autoRefresh on the raycaster component, as already described here

Changes proposed:

• a-entity offers detachFromScene() and attachToScene() API calls
• a-entity emits new events, attached-to-scene and detached-from-scene indicating when these new operations complete.
• pool component uses these to ensure that unused pool entities do not appear in the scene graph (gives major perf boost for Moonrider)
• New attached component gives HTML-l;evel contol of detachment from THREE,js scene graph, very similar to visible component.
• raycaster updated to ignore all objects that are not rooted in the main THREE.js scene graph (including auto-updates)
• Documentation & UT for the above. Documentation for toggling on/off a part of the HTML scene graph now points to attach component rather than visible component.

[diarmidmackenzie]

Just been reviewing this THREE.js thread, and wondering whether it would be simpler to:

• leave "detached" object3Ds in the scene graph
• add a new special flag (e.g. "detached") to them
• update updateMatrixWorld() in super-three.js to return when it sees the "detached" flag on an Object3D.

Per this comment it may be a little smoother / less performance-impactful to set a flag, rather than removing object3Ds from the scene graph...?

EDIT: On further reflection, less convinced this is any better. THREE.js rendering code would also need updating to treat "detached" like "visible". Overall, feeling like a more complex solution, rather than less complex. Propose leaving the PR as-is.

[dmarcos]

Maybe we can do this more specific / self contained to the pool component. It handles internally attaching / detaching of entities to the scene graph. We might not need modifying a-entity, attach components, new events...

[dmarcos]

Just been reviewing this THREE.js thread, and wondering whether it would be simpler to:

• leave "detached" object3Ds in the scene graph
• add a new special flag (e.g. "detached") to them
• update updateMatrixWorld() in super-three.js to return when it sees the "detached" flag on an Object3D.

Per this comment it may be a little smoother / less performance-impactful to set a flag, rather than removing object3Ds from the scene graph...?

EDIT: On further reflection, less convinced this is any better. THREE.js rendering code would also need updating to treat "detached" like "visible". Overall, feeling like a more complex solution, rather than less complex. Propose leaving the PR as-is.

Maybe a more specific skipUpdateMatrix flag? Other scenarios where we might want to skip besides detached objects?

[diarmidmackenzie]

Maybe a more specific skipUpdateMatrix flag? Other scenarios where we might want to skip besides detached objects?

There is a pretty simillar existing flag in THREE.js matrixWorldAutoUpdate, which can be set to false.

Differences from what we'd need:

• this can be overridden using the "force" option on updateMatrixWorld. I can't see any circumstances in which we'd want the new flag overridden
• it doesn't prevent rendering, only matrixUpdate calculations.

But having given this some more thought, I'm against the flag-based approach.

• It doesn't reduce code complexity - it just pushes complexity from A-Frame into super-three, where it's actually harder to manage, as code merges with evolving THREE.js code would have to be handled manually.
• I also suspect a flag would make things worse, rather than better from an overall performance point of view....

The new flag would need to be checked twice every frame for every object (once on updateMatrixWorld(), one on rendering).
Better way to minimize perf impact of these objects is to not have them in the tree at all.

Looking at the code for Object3D.add and Object3D.remove, for the case where you are adding / removing a single object, they basically boil down to an Array.push() for add, and an Array.splice() for remove, plus a single event emitted in each case. I don't see how that is going to amount to a significant performance impact.

[dmarcos]

What's your recommendation for this PR then? Still worth it?

[diarmidmackenzie]

Yes, absolutely I think the PR is worth it.

Just think we should stick with the code as per the current PR, deleting from / re-adding to the object tree.

My suggestion to use flags instead of adding/removing was not a good one, I think.

[dmarcos]

Can we make this more self contained to the pool component? No new events, no attached component, or modifications to a-entity? If it turns useful we can later generalize

[diarmidmackenzie]

Happy to drop "attached" component.

Dropping the events will leave some poor functionality around raycasting auto-updates.

Detached objects will still be scanned for raycasting (perf impact) and will have outdated matrixWorlds (functional impact: raycasting triggering in the wrong places).

In order to keep the events, I think we need to put the changes into a-entity, not just the pool component (in theory I guess the pool component could emit the events, but that feels really messy vs. emitting them from a-entity).

[dmarcos]

To keep it simple I think we can query for the raycaster entities in the pool component and call setDirty there instead of relying on an event.

[diarmidmackenzie]

Feels like a bit of a hack, but yes it can work.

As well as this, I'll also create an "attached" component external to A-Frame, and see whether the community finds it useful.

[dmarcos]

Yes, It couples the pool and raycaster component but relying to much on events makes code harder to understand too. In this case not super worried since the raycaster API has been stable for a while. If the attach / reattach approach is adopted and yields noticeable results we can revisit and generalize to something like your original PR. Thanks for being flexible

[dmarcos]

We can squeeze this in 1.4.0 if you have some time. Thanks!

[diarmidmackenzie]

Looking at it now...

[diarmidmackenzie]

Alternative fix now available at #5188. However I'm concerned about the perf impact of querySelectorAll('[raycaster]') every time we add to / remove from a pool.

Personally I think that using events is safer in terms of performance regression risks, but awaiting your feedback on this point...

[diarmidmackenzie]

#5189 offered as an alternative, about which I have fewer perf concerns, in case it is preferred. We should close whichever of #5188 or #5189 is not taken.

This one, I propose we pend as a possible eventual fix, in the event that the "attach" component (which I'll publish as an external component for now) proves to be broadly useful.