A new fix for #47

[Elettrotecnica]

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

aframe-physics-system/src/drivers/ammo-driver.js

Lines 67 to 76 in 98cf0b3

	/* @param {Ammo.btCollisionObject} body */
	AmmoDriver.prototype.removeBody = function(body) {
	this.physicsWorld.removeRigidBody(body);
	this.removeEventListener(body);
	const bodyptr = Ammo.getPointer(body);
	this.els.delete(bodyptr);
	this.collisions.delete(bodyptr);
	this.collisionKeys.splice(this.collisionKeys.indexOf(bodyptr), 1);
	this.currentCollisions.delete(bodyptr);
	};

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

aframe-physics-system/src/drivers/ammo-driver.js

Lines 106 to 109 in 98cf0b3

	if (!this.collisions.has(body0ptr)) {
	this.collisions.set(body0ptr, []);
	this.collisionKeys.push(body0ptr);
	}

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

aframe-physics-system/src/drivers/ammo-driver.js

Lines 110 to 114 in 98cf0b3

	if (this.collisions.get(body0ptr).indexOf(body1ptr) === -1) {
	this.collisions.get(body0ptr).push(body1ptr);
	if (this.eventListeners.indexOf(body0ptr) !== -1) {
	this.els.get(body0ptr).emit("collidestart", { targetEl: this.els.get(body1ptr) });
	}

aframe-physics-system/src/drivers/ammo-driver.js

Lines 126 to 140 in 98cf0b3

	for (let i = 0; i < this.collisionKeys.length; i++) {
	const body0ptr = this.collisionKeys[i];
	const body1ptrs = this.collisions.get(body0ptr);
	for (let j = body1ptrs.length - 1; j >= 0; j--) {
	const body1ptr = body1ptrs[j];
	if (this.currentCollisions.get(body0ptr).has(body1ptr)) {
	continue;
	}
	if (this.eventListeners.indexOf(body0ptr) !== -1) {
	this.els.get(body0ptr).emit("collideend", { targetEl: this.els.get(body1ptr) });
	}
	if (this.eventListeners.indexOf(body1ptr) !== -1) {
	this.els.get(body1ptr).emit("collideend", { targetEl: this.els.get(body0ptr) });
	}
	body1ptrs.splice(j, 1);

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]

Fantastic work. Thank you so much.