search

saharan / OimoPhysics

A cross-platform 3D physics engine
MIT License
863 stars 68 forks source link

BvhBroadPhase.hx - Memory leak #63

Closed Snky closed 5 months ago

Snky commented 1 year ago

Proxies added to movedProxies are not being removed when calling destroyProxy.

I've written this at the bottom of destroyProxy but I feel like the array needs sorting, as well as, bvhProxy._id is counting up at a different rate to numMovedProxies I believe:

movedProxies[bvhProxy._id] = null; numMovedProxies--; _idCount--;

Snky commented 1 year ago

Alternatively I've now written: movedProxies[bvhProxy._id] = movedProxies[--numMovedProxies]; movedProxies[numMovedProxies] = null; _idCount--;

I thought I'd leave these here, and perhaps someone else can comment which is suitable, or if there's a better way.

saharan commented 5 months ago

BvhBroadPhase.movedProxies is just a temporary array and will be cleared every step like in https://github.com/saharan/OimoPhysics/blob/c0c828a336c06380e90e92bb522f6344ce0f6cfa/src/oimo/collision/broadphase/bvh/BvhBroadPhase.hx#L217 or https://github.com/saharan/OimoPhysics/blob/c0c828a336c06380e90e92bb522f6344ce0f6cfa/src/oimo/collision/broadphase/bvh/BvhBroadPhase.hx#L197.

Destroying a proxy sets BvhProxy._moved to false so that the proxy won't be checked again and will just be discarded here: https://github.com/saharan/OimoPhysics/blob/c0c828a336c06380e90e92bb522f6344ce0f6cfa/src/oimo/collision/broadphase/bvh/BvhBroadPhase.hx#L208-L217

So it won't cause a memory leak. Also, the purpose of the ID assigned to each object is to distinguish between objects for debugging, and is not to store an index of an array. So this code

movedProxies[bvhProxy._id] = movedProxies[--numMovedProxies];

can access outside the array or overwrite and destroy necessary data to detect potential collisions.

Snky commented 4 months ago

Oh, will have to get my head around why I thought that then, so cool to see you're back, I think having an easy way to change center of gravity (one of the other issues) will be a huge game changer for Oimo!