Fix memory leak on mesh collider destroy

[MushAsterion]

Fixes #4267

I know it doesn't match the suggestion from the issue. However I think having a CollisionSystemImpl#destroyShape function sound more beneficial than making the parent class aware of an extension. It also removes override of the CollisionMeshSystemImpl#remove function and make sure there is an actual shape to destroy.

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

[willeastcott]

Hey @LeXXik - you submitted the original issue. Got any thoughts here? 😄

[LeXXik]

@willeastcott so, are we keeping the data object on the collision component then? I thought you might have been cooking an update for it, hence the last one didn't go through.

@MushAsterion, I think it would be good if we add a test that covers this change. If you are short on time, I can add it later this week. The test would need to cover at least clearing of the collision shapes (primitive, compounds, mesh and triggers). We don't need to test Ammo, so its methods can be stubbed. We just need to make sure we are calling the destroy methods on our side whenever needed.

[MushAsterion]

I think it would be good if we add a test that covers this change. If you are short on time, I can add it later this week. The test would need to cover at least clearing of the collision shapes (primitive, compounds, mesh and triggers). We don't need to test Ammo, so its methods can be stubbed. We just need to make sure we are calling the destroy methods on our side whenever needed.

That makes sense and time is not really a problem to me. However I have to admit that I have absolutely no knowledge or skills on this matter and when I look at the existing tests none already handle Ammo which sound quite complex to mirror especially on complex shapes... I would love to learn so if you have any example I can use to do so I'll be more than happy to give it a try!

[LeXXik]

@MushAsterion you can take a look at these tests that handle Ammo, as an example:

2a07dac

[MushAsterion]

Thank you @LeXXik! Took a bit of time but I managed to mirror Ammo based on the IDL from playcanvas/ammo.js repo and added the tests for destruction. Did my best to keep it simple but it might be still a bit messy...

I wasn't sure about creating a .mjs file only for Ammo but this way it keeps it separate since it is quite long. I know not all functions are used by the engine at the moment but as I was on it I just added everything based off the IDL so it won't cause issue if more features are used.

[LeXXik]

This is amazing. I'd say you went a bit overkill with the Ammo stub module :) However, as you mentioned it can definitely be used to cover all current and potential future features we add to the engine.

[mvaligursky]

The PR looks amazing. I added a small suggestion to the code, to make it slightly more compact.

The test looks a lot better than expected, I'm happy here.

Approving, but would like @willeastcott's thoughts here as well.

[MushAsterion]

The test looks a lot better than expected, I'm happy here.

In fact it was based on the ammo IDL from archived playcanvas/ammo.js, I think it might be useful to wait for the addition of latest Ammo.js version as @willeastcott told me it should come soon, unless it's not anymore it would then be beneficial to know the current IDL to make sure the Ammo tested version is the one actually used within editor as I was not aware the repo was outdated.

[mvaligursky]

Perhaps you can then split this PR into two .. one which fixes the memory leak, and the test as a separate. This would allow us to merge the fix while waiting for latest Ammo.

[MushAsterion]

Moved the tests to #5322!

[mvaligursky]

Perfect, thanks!