microsoft / mixed-reality-extension-sdk

The Mixed Reality Extension SDK enables developers to build 3D world extensions for AltspaceVR, using Node.JS.
MIT License
142 stars 61 forks source link

Fixes several memory leaks associated with destroying an actor #749

Closed kuperavv closed 3 years ago

kuperavv commented 3 years ago

Summary of changes:

Creates a public method on actor that unobserves the collider and breaks the reference to the collider. Removes mesh references to the actor to be destroyed Removes material references to the actor to be destroyed Added the ability to set the Collider layer using setCollider.

Unobserves 'other' collider listeners when copying to a new collider.

ghost commented 3 years ago

CLA assistant check
All CLA requirements met.

kuperavv commented 3 years ago

I was able to identify and resolve the memory leak associated with calling _setCollider() from within actor.copy().

There was an existing reference to the collider through the collider geometry.

To resolve this, and to only call generateColliderGeometry once, I deferred generateColliderGeometry in setCollider() into _setCollider(). Due to the typing, I added an if/then/else block to re-assign size and center. I then call generateColliderGeometry to create a new geometry, and re-assign it to the collider to break the references.

I removed the TODO comments as now you can instantiate an actor simultaneously with the collider without the memory leak.

kuperavv commented 3 years ago

I resolved some additional memory leaks stemming from not cleaning up animation references.

tombuMS commented 3 years ago

Thanks for the first PR!

This PR is targeting the wrong branch though. Can you please retarget it to the red branch for further review?

kuperavv commented 3 years ago

@tombuMS - Thanks for pointing that out. I believe I should have submitted another PR targeting the red branch, can you confirm that looks correct on your end? We can close this PR request if it looks correct.