fix: Fire collisionend when collider deleted + Composite collision st…

…art/end strategy (#2931)

One stealth feature 
* onCollisionEnd events now include the last side and contact which is useful for collision logic

This PR fixes 2 issues:

1. Fixes an issue where a collider that was part of a contact that was deleted did not fire a collision end event, this was unexpected
2. Fixes an issue where you may want to have composite colliders behave as constituent colliders for the purposes of start/end collision events. A new property is added to physics config, the current behavior is the default which is `'together'`, this means the whole composite collider is treated as 1 collider for onCollisionStart/onCollisionEnd. Now you can configure a `separate` which will fire onCollisionStart/onCollisionEnd for every separate collider included in the composite (useful if you are building levels or things with gaps that you need to disambiguate). You can also configure this on a per composite level to mix and match `CompositeCollider.compositeStrategy`
![image](https://github.com/excaliburjs/Excalibur/assets/612071/e1a9c954-8a39-4402-bee6-dad9b42f9c08)

CHANGELOG.md

@@ -40,6 +40,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Added
 
+- Added additional data `side` and `lastContact` to `onCollisionEnd` and `collisionend` events
+- Added configuration option to `ex.PhysicsConfig` to configure composite collider onCollisionStart/End behavior
 - Added configuration option to `ex.TileMap({ meshingLookBehind: Infinity })` which allows users to configure how far the TileMap looks behind for matching colliders (default is 10).
 - Added Arcade Collision Solver bias to help mitigate seams in geometry that can cause problems for certain games.
   - `ex.ContactSolveBias.None` No bias, current default behavior collisions are solved in the default distance order
@@ -203,6 +205,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Fixed
 
+-  Fixes an issue where a collider that was part of a contact that was deleted did not fire a collision end event, this was unexpected
+- Fixes an issue where you may want to have composite colliders behave as constituent colliders for the purposes of start/end collision events. A new property is added to physics config, the current behavior is the default which is `'together'`, this means the whole composite collider is treated as 1 collider for onCollisionStart/onCollisionEnd. Now you can configure a `separate` which will fire onCollisionStart/onCollisionEnd for every separate collider included in the composite (useful if you are building levels or things with gaps that you need to disambiguate). You can also configure this on a per composite level to mix and match `CompositeCollider.compositeStrategy`
 - Fixed issue where particles would have an errant draw if using a particle sprite
 - Fixed issue where a null/undefined graphics group member graphic would cause a crash, now logs a warning.
 - Fixed issue where Actor built in components could not be extended because of the way the Actor based type was built.

sandbox/src/game.ts

@@ -53,12 +53,15 @@ var game = new ex.Engine({
   pointerScope: ex.PointerScope.Canvas,
   displayMode: ex.DisplayMode.FitScreenAndFill,
   snapToPixel: false,
-  fixedUpdateFps: 30,
+  fixedUpdateFps: 60,
   maxFps: 60,
   antialiasing: false,
   uvPadding: 0,
   physics: {
-    solver: ex.SolverStrategy.Realistic,
+    colliders: {
+      compositeStrategy: 'together'
+    },
+    solver: ex.SolverStrategy.Arcade,
     gravity: ex.vec(0, 20),
     arcade: {
       contactSolveBias: ex.ContactSolveBias.VerticalFirst
@@ -578,8 +581,8 @@ player.on('collisionstart', () => {
   console.log('collision start');
 });
 
-player.on('collisionend', () => {
-  console.log('collision end');
+player.on('collisionend', (e) => {
+  console.log('collision end', e.other.collider);
 });
 
 // Health bar example

src/engine/Actor.ts

@@ -973,8 +973,10 @@ export class Actor extends Entity implements Eventable, PointerEvents, CanInitia
    * Fires once when 2 entities with a ColliderComponent separate after having been in contact.
    * @param self
    * @param other
+   * @param side
+   * @param lastContact
    */
-  public onCollisionEnd(self: Collider, other: Collider) {
+  public onCollisionEnd(self: Collider, other: Collider, side: Side, lastContact: CollisionContact) {
     // Override me
   }
 

src/engine/Collision/ColliderComponent.ts

@@ -58,18 +58,25 @@ export class ColliderComponent extends Component {
     return collider;
   }
 
+  private _collidersToRemove: Collider[] = [];
   /**
    * Remove collider geometry from collider component
    */
   public clear() {
     if (this._collider) {
-      this._collider.events.unpipe(this.events);
-      this.$colliderRemoved.notifyAll(this._collider);
-      this._collider.owner = null;
+      this._collidersToRemove.push(this._collider);
       this._collider = null;
     }
   }
 
+  public processColliderRemoval() {
+    for (const collider of this._collidersToRemove) {
+      collider.events.unpipe(this.events);
+      this.$colliderRemoved.notifyAll(collider);
+      collider.owner = null;
+    }
+  }
+
   public clone(): ColliderComponent {
     const clone = new ColliderComponent(this._collider.clone());
 
@@ -187,9 +194,9 @@ export class ColliderComponent extends Component {
     });
     this.events.on('collisionend', (evt: any) => {
       const end = evt as CollisionEndEvent<Collider>;
-      entity.events.emit('collisionend', new CollisionEndEvent(end.target.owner, end.other.owner));
+      entity.events.emit('collisionend', new CollisionEndEvent(end.target.owner, end.other.owner, end.side, end.lastContact));
       if (entity instanceof Actor) {
-        entity.onCollisionEnd(end.target, end.other);
+        entity.onCollisionEnd(end.target, end.other, end.side, end.lastContact);
       }
     });
   }

src/engine/Collision/Colliders/Collider.ts

@@ -12,6 +12,7 @@ import { ExcaliburGraphicsContext } from '../../Graphics/Context/ExcaliburGraphi
 import { Transform } from '../../Math/transform';
 import { EventEmitter } from '../../EventEmitter';
 import { RayCastHit } from '../Detection/RayCastHit';
+import { CompositeCollider } from './CompositeCollider';
 
 /**
  * A collision collider specifies the geometry that can detect when other collision colliders intersect
@@ -21,11 +22,11 @@ export abstract class Collider implements Clonable<Collider> {
   private static _ID = 0;
   public readonly id: Id<'collider'> = createId('collider', Collider._ID++);
   /**
-   * Excalibur uses this to signal to the [[CollisionSystem]] this is part of a composite collider
-   * @internal
-   * @hidden
+   * Composite collider if any this collider is attached to
+   *
+   * **WARNING** do not tamper with this property
    */
-  public __compositeColliderId: Id<'collider'> | null = null;
+  public composite: CompositeCollider | null = null;
   public events = new EventEmitter();
 
   /**

src/engine/Collision/Colliders/CompositeCollider.ts

@@ -21,6 +21,26 @@ export class CompositeCollider extends Collider {
   private _dynamicAABBTree = new DynamicTree(DefaultPhysicsConfig.dynamicTree);
   private _colliders: Collider[] = [];
 
+  private _compositeStrategy?: 'separate' | 'together';
+  /**
+   * Treat composite collider's member colliders as either separate colliders for the purposes of onCollisionStart/onCollision
+   * or as a single collider together.
+   *
+   * This property can be overridden on individual [[CompositeColliders]].
+   *
+   * For composites without gaps or small groups of colliders, you probably want 'together'
+   *
+   * For composites with deliberate gaps, like a platforming level layout, you probably want 'separate'
+   *
+   * Default is 'together' if unset
+   */
+  public set compositeStrategy(value: 'separate' | 'together') {
+    this._compositeStrategy = value;
+  }
+  public get compositeStrategy() {
+    return this._compositeStrategy;
+  }
+
   constructor(colliders: Collider[]) {
     super();
     for (const c of colliders) {
@@ -43,7 +63,7 @@ export class CompositeCollider extends Collider {
     // Flatten composites
     for (const c of colliders) {
       c.events.pipe(this.events);
-      c.__compositeColliderId = this.id;
+      c.composite = this;
       this._colliders.push(c);
       this._collisionProcessor.track(c);
       this._dynamicAABBTree.trackCollider(c);
@@ -52,7 +72,7 @@ export class CompositeCollider extends Collider {
 
   removeCollider(collider: Collider) {
     collider.events.pipe(this.events);
-    collider.__compositeColliderId = null;
+    collider.composite = null;
     Util.removeItemFromArray(collider, this._colliders);
     this._collisionProcessor.untrack(collider);
     this._dynamicAABBTree.untrackCollider(collider);

src/engine/Collision/Colliders/PolygonCollider.ts

@@ -43,16 +43,21 @@ export class PolygonCollider extends Collider {
    */
   public offset: Vector;
 
+  public flagDirty() {
+    this._localBoundsDirty = true;
+    this._localSidesDirty = true;
+    this._sidesDirty = true;
+  }
+
   private _points: Vector[];
+
   /**
    * Points in the polygon in order around the perimeter in local coordinates. These are relative from the body transform position.
    * Excalibur stores these in counter-clockwise order
    */
   public set points(points: Vector[]) {
-    this._localBoundsDirty = true;
-    this._localSidesDirty = true;
-    this._sidesDirty = true;
     this._points = points;
+    this.flagDirty();
   }
 
   /**

src/engine/Collision/CollisionSystem.ts

@@ -79,6 +79,9 @@ export class CollisionSystem extends System {
         colliderComp.update();
         if (collider instanceof CompositeCollider) {
           const compositeColliders = collider.getColliders();
+          if (!collider.compositeStrategy) {
+            collider.compositeStrategy = this._physics.config.colliders.compositeStrategy;
+          }
           colliders = colliders.concat(compositeColliders);
         } else {
           colliders.push(collider);
@@ -127,6 +130,14 @@ export class CollisionSystem extends System {
 
     // Keep track of collisions contacts that have started or ended
     this._lastFrameContacts = new Map(this._currentFrameContacts);
+
+    // Process deferred collider removals
+    for (const entity of this.query.entities) {
+      const collider = entity.get(ColliderComponent);
+      if (collider) {
+        collider.processColliderRemoval();
+      }
+    }
   }
 
   getSolver(): CollisionSolver {
@@ -143,7 +154,7 @@ export class CollisionSystem extends System {
   }
 
   public runContactStartEnd() {
-    // Composite collider collisions may have a duplicate id because we want to treat those as a singular start/end
+    // If composite colliders are 'together' collisions may have a duplicate id because we want to treat those as a singular start/end
     for (const [id, c] of this._currentFrameContacts) {
       // find all new contacts
       if (!this._lastFrameContacts.has(id)) {
@@ -163,10 +174,12 @@ export class CollisionSystem extends System {
       if (!this._currentFrameContacts.has(id)) {
         const colliderA = c.colliderA;
         const colliderB = c.colliderB;
-        colliderA.events.emit('collisionend', new CollisionEndEvent(colliderA, colliderB));
-        colliderA.events.emit('contactend', new ContactEndEvent(colliderA, colliderB) as any);
-        colliderB.events.emit('collisionend', new CollisionEndEvent(colliderB, colliderA));
-        colliderB.events.emit('contactend', new ContactEndEvent(colliderB, colliderA) as any);
+        const side = Side.fromDirection(c.mtv);
+        const opposite = Side.getOpposite(side);
+        colliderA.events.emit('collisionend', new CollisionEndEvent(colliderA, colliderB, side, c));
+        colliderA.events.emit('contactend', new ContactEndEvent(colliderA, colliderB, side, c) as any);
+        colliderB.events.emit('collisionend', new CollisionEndEvent(colliderB, colliderA, opposite, c));
+        colliderB.events.emit('contactend', new ContactEndEvent(colliderB, colliderA, opposite, c) as any);
       }
     }
   }

src/engine/Collision/Detection/CollisionContact.ts

@@ -76,11 +76,11 @@ export class CollisionContact {
     this.localPoints = localPoints;
     this.info = info;
     this.id = Pair.calculatePairHash(colliderA.id, colliderB.id);
-    if (colliderA.__compositeColliderId || colliderB.__compositeColliderId) {
-      // Add on the parent composite pair for start/end contact
-      this.id += '|' + Pair.calculatePairHash(
-        colliderA.__compositeColliderId ?? colliderA.id,
-        colliderB.__compositeColliderId ?? colliderB.id);
+    if (colliderA.composite || colliderB.composite) {
+      // Add on the parent composite pair for start/end contact if 'together
+      const colliderAId = colliderA.composite?.compositeStrategy === 'separate' ? colliderA.id : colliderA.composite?.id ?? colliderA.id;
+      const colliderBId = colliderB.composite?.compositeStrategy === 'separate' ? colliderB.id : colliderB.composite?.id ?? colliderB.id;
+      this.id += '|' + Pair.calculatePairHash(colliderAId, colliderBId);
     }
   }
 

src/engine/Collision/PhysicsConfig.ts

@@ -30,6 +30,25 @@ export interface PhysicsConfig {
    */
   solver?: SolverStrategy;
 
+  /**
+   * Configure colliders
+   */
+  colliders?: {
+    /**
+     * Treat composite collider's member colliders as either separate colliders for the purposes of onCollisionStart/onCollision
+     * or as a single collider together.
+     *
+     * This property can be overridden on individual [[CompositeColliders]].
+     *
+     * For composites without gaps or small groups of colliders, you probably want 'together'
+     *
+     * For composites with deliberate gaps, like a platforming level layout, you probably want 'separate'
+     *
+     * Default is 'together' if unset
+     */
+    compositeStrategy?: 'separate' | 'together'
+  }
+
   /**
    * Configure excalibur continuous collision (WIP)
    */
@@ -183,6 +202,9 @@ export const DefaultPhysicsConfig: DeepRequired<PhysicsConfig> = {
   enabled: true,
   gravity: vec(0, 0),
   solver: SolverStrategy.Arcade,
+  colliders: {
+    compositeStrategy: 'together'
+  },
   continuous: {
     checkForFastBodies: true,
     disableMinimumSpeedForFastBody: false,
@@ -224,6 +246,9 @@ export function DeprecatedStaticToConfig(): DeepRequired<PhysicsConfig> {
       disableMinimumSpeedForFastBody: Physics.disableMinimumSpeedForFastBody,
       surfaceEpsilon: Physics.surfaceEpsilon
     },
+    colliders: {
+      compositeStrategy: 'together'
+    },
     bodies: {
       canSleepByDefault: Physics.bodiesCanSleepByDefault,
       sleepEpsilon: Physics.sleepEpsilon,

src/engine/Events.ts

@@ -451,7 +451,7 @@ export class ContactStartEvent<T> {
 }
 
 export class ContactEndEvent<T> {
-  constructor(public target: T, public other: T) {}
+  constructor(public target: T, public other: T, public side: Side, public lastContact: CollisionContact) {}
 }
 
 export class CollisionPreSolveEvent<T> {
@@ -494,7 +494,7 @@ export class CollisionEndEvent<T extends BodyComponent | Collider | Entity = Act
   /**
    *
    */
-  constructor(actor: T, public other: T) {
+  constructor(actor: T, public other: T, public side: Side, public lastContact: CollisionContact) {
     super();
     this.target = actor;
   }

src/spec/ColliderComponentSpec.ts

@@ -97,6 +97,7 @@ describe('A ColliderComponent', () => {
     expect(collider.owner).not.toBeNull();
 
     comp.clear();
+    comp.processColliderRemoval();
 
     expect(comp.get()).toBeNull();
     expect(collider.events.unpipe).toHaveBeenCalled();

src/spec/CollisionSpec.ts

@@ -25,6 +25,7 @@ describe('A Collision', () => {
   afterEach(() => {
     ex.Physics.collisionResolutionStrategy = ex.SolverStrategy.Arcade;
     engine.stop();
+    engine.dispose();
     engine = null;
     actor1 = null;
     actor2 = null;
@@ -378,4 +379,50 @@ describe('A Collision', () => {
     expect(block2.onPreCollisionResolve).toHaveBeenCalled();
     expect(block1.onCollisionStart).not.toHaveBeenCalled();
   });
+
+  it('should collisionend for a deleted collider', async () => {
+    engine.stop();
+    engine.dispose();
+    engine = TestUtils.engine({ width: 600, height: 400, physics: { enabled: true, solver: ex.SolverStrategy.Arcade }});
+    clock = engine.clock = engine.clock.toTestClock();
+    await TestUtils.runToReady(engine);
+    const activeBlock = new ex.Actor({
+      x: 200,
+      y: 200,
+      width: 50,
+      height: 50,
+      color: ex.Color.Red.clone(),
+      collisionType: ex.CollisionType.Active
+    });
+    activeBlock.acc.x = 100;
+    engine.add(activeBlock);
+
+    const fixedBlock = new ex.Actor({
+      x: 400,
+      y: 200,
+      width: 50,
+      height: 50,
+      color: ex.Color.DarkGray.clone(),
+      collisionType: ex.CollisionType.Fixed
+    });
+    engine.add(fixedBlock);
+
+    const collisionStart = jasmine.createSpy('collisionstart');
+    const collisionEnd = jasmine.createSpy('collisionend');
+
+    activeBlock.on('collisionstart', collisionStart);
+    activeBlock.on('collisionend', collisionEnd);
+
+    clock.run(20, 100);
+
+    expect(collisionStart).toHaveBeenCalled();
+    expect(collisionEnd).not.toHaveBeenCalled();
+
+    activeBlock.collider.set(ex.Shape.Circle(5));
+
+    clock.run(10, 100);
+
+    expect(collisionEnd).toHaveBeenCalledTimes(1);
+
+  });
 });