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

Mesh collision optimization #6087

Merged
merged 2 commits into from Feb 27, 2024
Merged

Mesh collision optimization #6087

merged 2 commits into from Feb 27, 2024

Conversation

LeXXik
Copy link
Contributor

@LeXXik LeXXik commented Feb 25, 2024

This is a follow up on #5818

After we have created a mesh collider, the engine proceeds with setting the scale of the mesh. This is an expensive operation in Ammo, as it triggers another loop over all verts and recalculating AABB, which was already calculated once when we created btBvhTriangleMeshShape.
Since we are already looping over verts on JS side, we can apply scale in place instead.

This. further improves collider generation (24k verts) from ~181 ms down to ~115 ms.

Note:
This optimization is only for Render Asset types. There is no change to legacy Model Assets, which will continue using Ammo scaling as they do now.

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

@LeXXik
Copy link
Contributor Author

LeXXik commented Feb 25, 2024

We could probably change that for Model Assets as well, but I am not too familiar with the legacy format. Can the nodes on model mesh instances be of any other scale, than 1?

@mvaligursky
Copy link
Contributor

Thanks for the PR @LeXXik .
Yes, the model component works the same way, mesh instances have GraphNodes and those would have scales matching those from render component.

@mvaligursky mvaligursky added performance Relating to load times or frame rate area: physics Physics related issue labels Feb 26, 2024
const scaling = system._getNodeScaling(node);
triMeshShape.setLocalScaling(scaling);
Ammo.destroy(scaling);
if (!scale) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if instead of passing the scale parameter to the function, and then either using it, or extracting it from the node, we should not pass it in, and always extract it from the node? And apply directly to vertices that you introduced here. I'm just not sure what the advantage is in having to pass it in, considering we can extract it from the node?

Copy link
Contributor Author

@LeXXik LeXXik Feb 26, 2024

Choose a reason for hiding this comment

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

Hmm, in case of Render Asset that node is a temp node, which will always have a scale 1, so not needed. In case of Model Asset it will be a node from the mesh instance. I wasn't sure if nodes on model mesh instances could have a different scale, so left it there as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see .. there were two places we apply scale to ammo mesh. One is based on the node of the mesh-instance, and the other is based on the node of the collision component. Your change only handles the first of those.

I somehow assumed we have just one scale applied.

Copy link
Contributor

@mvaligursky mvaligursky left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@mvaligursky mvaligursky merged commit 53d2ce4 into playcanvas:main Feb 27, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: physics Physics related issue performance Relating to load times or frame rate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants