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

Detach unused pool elements from THREE.js scene #5186

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

diarmidmackenzie
Copy link
Contributor

@diarmidmackenzie diarmidmackenzie commented Dec 19, 2022

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.

This improves performance of updateMatrixWorld() in render cycle.
docsLint.js appears to not like URLs that end with a "-", though this is necessary to link to the anchors for these functions...

[error]: docs/components/attached.md
    Page does not exist: [attach]: ../core/entity.md#attachtoscene-
    Page does not exist: [detach]: ../core/entity.md#detachfromscene-
@diarmidmackenzie
Copy link
Contributor Author

diarmidmackenzie commented Dec 19, 2022

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
Copy link
Member

dmarcos commented Dec 21, 2022

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
Copy link
Member

dmarcos commented Dec 21, 2022

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
Copy link
Contributor Author

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
Copy link
Member

dmarcos commented Dec 22, 2022

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

@diarmidmackenzie
Copy link
Contributor Author

diarmidmackenzie commented Dec 22, 2022

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
Copy link
Member

dmarcos commented Dec 23, 2022

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
Copy link
Contributor Author

diarmidmackenzie commented Dec 23, 2022

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
Copy link
Member

dmarcos commented Dec 23, 2022

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
Copy link
Contributor Author

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
Copy link
Member

dmarcos commented Dec 23, 2022

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
Copy link
Member

dmarcos commented Dec 24, 2022

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

@diarmidmackenzie
Copy link
Contributor Author

Looking at it now...

@diarmidmackenzie
Copy link
Contributor Author

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
Copy link
Contributor Author

diarmidmackenzie commented Dec 24, 2022

#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.

dmarcos pushed a commit that referenced this pull request Dec 24, 2022
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