-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
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? |
Thanks for the PR @LeXXik . |
const scaling = system._getNodeScaling(node); | ||
triMeshShape.setLocalScaling(scaling); | ||
Ammo.destroy(scaling); | ||
if (!scale) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
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.