search

c-frame / aframe-physics-system

community-maintained fork of n5ro's aframe-physics-system
https://c-frame.github.io/aframe-physics-system/
MIT License
42 stars 11 forks source link

A new fix for #47 #57

Closed Elettrotecnica closed 10 months ago

Elettrotecnica commented 10 months ago

Dear maintainers,

I was struggling with some misbehavior in my application, without realizing this may very well be related with issues #47 #52 and #54

Similar to the use cases reported originally by @smeybi, my application also may remove ammo bodies during its lifecycle, as they represent assets that are spawned/despawned on the scene.

After some long debugging I have realized there existed a problem with the way ammo bodies were deleted. If we look at

https://github.com/c-frame/aframe-physics-system/blob/98cf0b39465c76006cc86d4a21e227cfcdc12f06/src/drivers/ammo-driver.js#L67-L76

At line 74 we see that when an ammo body is deleted, its supposed entry in the collisionKeys array is removed. However, this entry will be there only if the body ever participated in a collision in the role of body0

https://github.com/c-frame/aframe-physics-system/blob/98cf0b39465c76006cc86d4a21e227cfcdc12f06/src/drivers/ammo-driver.js#L106-L109

If this never happened at the time the body is removed, the resulting index -1 will de-facto make so that splice will delete an arbitrary entry in collisionKeys. If this pattern repeats often enough, it will de-facto disable event generation for any body on the scene, as only in the collisionKeys loop we do cleanup the collisions array, which is checked before we trigger the event

https://github.com/c-frame/aframe-physics-system/blob/98cf0b39465c76006cc86d4a21e227cfcdc12f06/src/drivers/ammo-driver.js#L110-L114

https://github.com/c-frame/aframe-physics-system/blob/98cf0b39465c76006cc86d4a21e227cfcdc12f06/src/drivers/ammo-driver.js#L126-L140

In my proposal I revert the previous fix, which came at the cost of a memory leak, and refactor the code so that we do not instantiate the data structures relevant for collisions on the fly, but at body creation. This ensures that a collisionKeys entry is there for every body and also makes the code more orderly IMO.

Please have a look if this also solves the issue reported originally.

All the best

diarmidmackenzie commented 10 months ago

Fantastic work. Thank you so much.