From df1acb3f10d35928de39342685f28397b2fccb89 Mon Sep 17 00:00:00 2001 From: Erik Onarheim Date: Sat, 27 Nov 2021 23:54:48 -0600 Subject: [PATCH] fix: Prevent pair generation for composite colliders (#2131) Fixes an issue where pairs were erroneously being generated for composite colliders when fps was low. image Discovered when artificially limiting to 15 fps, this happened because the low fps would trigger the continuous fast moving object detection for the collision processor. This fast object code had a bug and did not evaluate whether it should generate a pair the same way. The code has been fixed so that they are consistent. ## Changes: - Prevent pairs with the same owner id from being generated - Adds tests to the fast moving object --- CHANGELOG.md | 1 + .../DynamicTreeCollisionProcessor.ts | 39 ++------ src/engine/Collision/Detection/Pair.ts | 22 +++++ src/engine/Loader.ts | 28 +++--- src/spec/CollisionSpec.ts | 2 +- src/spec/DynamicTreeBroadphaseSpec.ts | 16 +++ src/spec/PairSpec.ts | 98 +++++++++++++++++++ 7 files changed, 162 insertions(+), 44 deletions(-) create mode 100644 src/spec/PairSpec.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 8558bd3c..77e5ffd7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). ### Fixed +- Fixed issue where fast moving `CompositeCollider`s were erroneously generating pairs for their constituent parts - Fixed Safari 13.1 crash when booting Excalibur because of they odd MediaQuery API in older Safari - Fixed issue where pointers did not work because of missing types - Fixed issue with `ArcadeSolver` where stacked/overlapped tiles would double solve the position of the collider for the same overlap diff --git a/src/engine/Collision/Detection/DynamicTreeCollisionProcessor.ts b/src/engine/Collision/Detection/DynamicTreeCollisionProcessor.ts index 65e643a6..7b90a3da 100644 --- a/src/engine/Collision/Detection/DynamicTreeCollisionProcessor.ts +++ b/src/engine/Collision/Detection/DynamicTreeCollisionProcessor.ts @@ -20,7 +20,7 @@ import { ExcaliburGraphicsContext } from '../..'; */ export class DynamicTreeCollisionProcessor implements CollisionProcessor { private _dynamicCollisionTree = new DynamicTree(); - private _collisions = new Set(); + private _pairs = new Set(); private _collisionPairCache: Pair[] = []; private _colliders: Collider[] = []; @@ -77,31 +77,10 @@ export class DynamicTreeCollisionProcessor implements CollisionProcessor { } } - private _shouldGenerateCollisionPair(colliderA: Collider, colliderB: Collider) { - // if the collision pair must be 2 separate colliders - // Also separate owners for composite colliders - if ( - (colliderA.id !== null && - colliderB.id !== null && - colliderA.id === colliderB.id) || - (colliderA.owner !== null && - colliderB.owner !== null && - colliderA.owner === colliderB.owner)) { - return false; - } - + private _pairExists(colliderA: Collider, colliderB: Collider) { // if the collision pair has been calculated already short circuit const hash = Pair.calculatePairHash(colliderA.id, colliderB.id); - if (this._collisions.has(hash)) { - return false; // pair exists easy exit return false - } - - // if the pair has a member with zero dimension - if (colliderA.localBounds.hasZeroDimensions() || colliderB.localBounds.hasZeroDimensions()) { - return false; - } - - return Pair.canCollide(colliderA, colliderB); + return this._pairs.has(hash); } /** @@ -118,7 +97,7 @@ export class DynamicTreeCollisionProcessor implements CollisionProcessor { // clear old list of collision pairs this._collisionPairCache = []; - this._collisions.clear(); + this._pairs.clear(); // check for normal collision pairs let collider: Collider; @@ -126,9 +105,9 @@ export class DynamicTreeCollisionProcessor implements CollisionProcessor { collider = potentialColliders[j]; // Query the collision tree for potential colliders this._dynamicCollisionTree.query(collider, (other: Collider) => { - if (this._shouldGenerateCollisionPair(collider, other)) { + if (!this._pairExists(collider, other) && Pair.canCollide(collider, other)) { const pair = new Pair(collider, other); - this._collisions.add(pair.id); + this._pairs.add(pair.id); this._collisionPairCache.push(pair); } // Always return false, to query whole tree. Returning true in the query method stops searching @@ -175,7 +154,7 @@ export class DynamicTreeCollisionProcessor implements CollisionProcessor { let minCollider: Collider; let minTranslate: Vector = new Vector(Infinity, Infinity); this._dynamicCollisionTree.rayCastQuery(ray, updateDistance + Physics.surfaceEpsilon * 2, (other: Collider) => { - if (collider !== other && Pair.canCollide(collider, other)) { + if (!this._pairExists(collider, other) && Pair.canCollide(collider, other)) { const hitPoint = other.rayCast(ray, updateDistance + Physics.surfaceEpsilon * 10); if (hitPoint) { const translate = hitPoint.sub(origin); @@ -190,8 +169,8 @@ export class DynamicTreeCollisionProcessor implements CollisionProcessor { if (minCollider && Vector.isValid(minTranslate)) { const pair = new Pair(collider, minCollider); - if (!this._collisions.has(pair.id)) { - this._collisions.add(pair.id); + if (!this._pairs.has(pair.id)) { + this._pairs.add(pair.id); this._collisionPairCache.push(pair); } // move the fast moving object to the other body diff --git a/src/engine/Collision/Detection/Pair.ts b/src/engine/Collision/Detection/Pair.ts index 284fe618..e8fcb3c3 100644 --- a/src/engine/Collision/Detection/Pair.ts +++ b/src/engine/Collision/Detection/Pair.ts @@ -13,10 +13,32 @@ export class Pair { this.id = Pair.calculatePairHash(colliderA.id, colliderB.id); } + /** + * Returns whether a it is allowed for 2 colliders in a Pair to collide + * @param colliderA + * @param colliderB + */ public static canCollide(colliderA: Collider, colliderB: Collider) { const bodyA = colliderA?.owner?.get(BodyComponent); const bodyB = colliderB?.owner?.get(BodyComponent); + // Prevent self collision + if (colliderA.id === colliderB.id) { + return false; + } + + // Colliders with the same owner do not collide (composite colliders) + if (colliderA.owner && + colliderB.owner && + colliderA.owner.id === colliderB.owner.id) { + return false; + } + + // if the pair has a member with zero dimension don't collide + if (colliderA.localBounds.hasZeroDimensions() || colliderB.localBounds.hasZeroDimensions()) { + return false; + } + // Body's needed for collision in the current state // TODO can we collide without a body? if (!bodyA || !bodyB) { diff --git a/src/engine/Loader.ts b/src/engine/Loader.ts index f08ef443..0fd8510b 100644 --- a/src/engine/Loader.ts +++ b/src/engine/Loader.ts @@ -354,19 +354,21 @@ export class Loader extends Class implements Loadable[]> { } private _positionPlayButton() { - const screenHeight = this._engine.screen.viewport.height; - const screenWidth = this._engine.screen.viewport.width; - if (this._playButtonRootElement) { - const left = this._engine.canvas.offsetLeft; - const top = this._engine.canvas.offsetTop; - const buttonWidth = this._playButton.clientWidth; - const buttonHeight = this._playButton.clientHeight; - if (this.playButtonPosition) { - this._playButtonRootElement.style.left = `${this.playButtonPosition.x}px`; - this._playButtonRootElement.style.top = `${this.playButtonPosition.y}px`; - } else { - this._playButtonRootElement.style.left = `${left + screenWidth / 2 - buttonWidth / 2}px`; - this._playButtonRootElement.style.top = `${top + screenHeight / 2 - buttonHeight / 2 + 100}px`; + if (this._engine) { + const screenHeight = this._engine.screen.viewport.height; + const screenWidth = this._engine.screen.viewport.width; + if (this._playButtonRootElement) { + const left = this._engine.canvas.offsetLeft; + const top = this._engine.canvas.offsetTop; + const buttonWidth = this._playButton.clientWidth; + const buttonHeight = this._playButton.clientHeight; + if (this.playButtonPosition) { + this._playButtonRootElement.style.left = `${this.playButtonPosition.x}px`; + this._playButtonRootElement.style.top = `${this.playButtonPosition.y}px`; + } else { + this._playButtonRootElement.style.left = `${left + screenWidth / 2 - buttonWidth / 2}px`; + this._playButtonRootElement.style.top = `${top + screenHeight / 2 - buttonHeight / 2 + 100}px`; + } } } } diff --git a/src/spec/CollisionSpec.ts b/src/spec/CollisionSpec.ts index 9d416aa8..707e1d9b 100644 --- a/src/spec/CollisionSpec.ts +++ b/src/spec/CollisionSpec.ts @@ -280,7 +280,7 @@ describe('A Collision', () => { fixedBlock.body.collisionType = ex.CollisionType.Fixed; engine.add(fixedBlock); - clock.run(5, 1000); + clock.run(15, 100); expect(activeBlock.vel.x).toBe(0); }); diff --git a/src/spec/DynamicTreeBroadphaseSpec.ts b/src/spec/DynamicTreeBroadphaseSpec.ts index eeb37aea..8caecfe7 100644 --- a/src/spec/DynamicTreeBroadphaseSpec.ts +++ b/src/spec/DynamicTreeBroadphaseSpec.ts @@ -58,4 +58,20 @@ describe('A DynamicTree Broadphase', () => { const pairs = dt.broadphase([circle, box], 100); expect(pairs).toEqual([]); }); + + it('should not find pairs for a composite collider when moving fast', () => { + const circle = ex.Shape.Circle(50); + const box = ex.Shape.Box(200, 10); + const compCollider = new ex.CompositeCollider([ + circle, + box + ]); + const actor = new ex.Actor({collider: compCollider, collisionType: ex.CollisionType.Active}); + actor.body.vel = ex.vec(2000, 0); // extra fast to trigger the fast object detection + const dt = new ex.DynamicTreeCollisionProcessor(); + dt.track(compCollider); + + const pairs = dt.broadphase([circle, box], 100); + expect(pairs).toEqual([]); + }); }); diff --git a/src/spec/PairSpec.ts b/src/spec/PairSpec.ts new file mode 100644 index 00000000..69ab4a8c --- /dev/null +++ b/src/spec/PairSpec.ts @@ -0,0 +1,98 @@ +import * as ex from '@excalibur'; + +describe('A Collision Pair', () => { + it('exists', () => { + expect(ex.Pair).toBeDefined(); + }); + + it('can be created with colliders', () => { + const actor1 = new ex.Actor({ + width: 10, + height: 10 + }); + + const actor2 = new ex.Actor({ + width: 20, + height: 20 + }); + + const sut = new ex.Pair(actor1.collider.get(), actor2.collider.get()); + + expect(sut.id).toBe(`#${actor1.collider.get().id.value}+${actor2.collider.get().id.value}`); + }); + + it('cannot collide without a body', () => { + const actor1 = new ex.Actor({ + width: 10, + height: 10 + }); + + const actor2 = new ex.Actor({ + width: 20, + height: 20 + }); + actor2.removeComponent('ex.body', true); + + const sut = ex.Pair.canCollide(actor1.collider.get(), actor2.collider.get()); + + expect(sut).toBe(false); + }); + + it('cannot collide with the same collider', () => { + const actor1 = new ex.Actor({ + width: 10, + height: 10 + }); + + const sut = ex.Pair.canCollide(actor1.collider.get(), actor1.collider.get()); + + expect(sut).toBe(false); + }); + + it('cannot collide with the same colliders with the same owner', () => { + const actor1 = new ex.Actor(); + const collider1 = ex.Shape.Circle(50); + const collider2 = ex.Shape.Box(100, 100); + actor1.collider.useCompositeCollider([ + collider1, + collider2 + ]); + + const sut = ex.Pair.canCollide(collider1, collider2); + + expect(sut).toBe(false); + }); + + it('cannot collide with zero dimension colliders', () => { + const actor1 = new ex.Actor({ + width: 10, + height: 10 + }); + + const actor2 = new ex.Actor(); + actor2.collider.useBoxCollider(0, 0); + + const sut = ex.Pair.canCollide(actor1.collider.get(), actor2.collider.get()); + + expect(sut).toBe(false); + }); + + it('cannot collide with same collision group', () => { + const group = new ex.CollisionGroup('group', 1, ~1); + const actor1 = new ex.Actor({ + width: 10, + height: 10 + }); + actor1.body.group = group; + + const actor2 = new ex.Actor({ + width: 100, + height: 100 + }); + actor2.body.group = group; + + const sut = ex.Pair.canCollide(actor1.collider.get(), actor2.collider.get()); + + expect(sut).toBe(false); + }); +}); \ No newline at end of file -- 2.51.2