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

Optimize RenderComponent destroy #7194

Open
wants to merge 6 commits into
base: main_v1
Choose a base branch
from

Conversation

Maksims
Copy link
Collaborator

@Maksims Maksims commented Dec 13, 2024

RenderComponent has a property rootBone, which is just a reference to an entity, that is used for skinning. Currently, it uses overcomplex EntityReference which creates a number of event handlers in various systems, which is extremely inefficient. rootBone - is a simple property that just is either null or some Entity.
When creating/destroying entities, EntityReference has to subscribe and unsubscribe to a number of events, which leads to an unnecessary workload.

So this PR just simplifies rootBone property, which does not change any behaviours. While optimising destruction of all RenderComponents (not skinned especially!!).

Here are test results, of destroying 4706 entities with RenderComponents (not skinned, primitive cubes):

Before: 640ms
After: 84ms

This is ~7.5 times speedup, or ~87% improvement.

In our use case, we need to parametrically create a lot of entities, and remove them on user interaction, so any lag spike is extremely noticeable. I've also noticed that changing rootBone when an entity is disabled, will not be taken in account when an entity is enabled, but this is an existing current problem and should be fixed in a separate PR.

Also, a lot of other components use EntityReference, which is best to go. It is unnecessarily complex and inefficient.

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

* @private
* Sets the root bone entity (or entity guid) for the render component.
*
* @type {import('../../entity.js').Entity|string|null}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a massive fan of having types differ for a single setter/getter pair. The previous type of rootBone was Entity. Seems correct to also allow null. Was the old type definition wrong for not specifying string was valid too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Old definition was wrong, as it was accepting strings and actively does it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

asset accessor does the same with asset id.

Copy link
Contributor

@willeastcott willeastcott left a comment

Choose a reason for hiding this comment

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

LGTM. Nice! 👏 Are you gonna do a follow up to get this merged to main?

@willeastcott willeastcott added the performance Relating to load times or frame rate label Dec 16, 2024
@Maksims
Copy link
Collaborator Author

Maksims commented Dec 16, 2024

LGTM. Nice! 👏 Are you gonna do a follow up to get this merged to main?

I'm happy for the team to do the cherry-picking and manage multiple versions on their own.
Not sure that it is friendly to outside contributors to ask them to manage multiple versions as I'm for example am not familiar with v2, and we don't use it yet as it is not available in Editor, so I cannot test this PR against main, only main_v1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Relating to load times or frame rate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants