fix: Fix collision prospect checking to be fully reliable

Author: luanpotter

packages/flame/lib/collisions.dart

@@ -1,4 +1,5 @@
 export 'src/collisions/broadphase/broadphase.dart';
+export 'src/collisions/broadphase/collision_prospect.dart';
 export 'src/collisions/broadphase/prospect_pool.dart';
 export 'src/collisions/broadphase/quadtree/has_quadtree_collision_detection.dart';
 export 'src/collisions/broadphase/quadtree/quad_tree_broadphase.dart';

packages/flame/lib/src/collisions/broadphase/broadphase.dart

@@ -1,5 +1,3 @@
-import 'dart:math';
-
 import 'package:flame/collisions.dart';
 
 /// The [Broadphase] class is used to make collision detection more efficient
@@ -53,43 +51,3 @@ abstract class Broadphase<T extends Hitbox<T>> {
   /// Returns the potential hitbox collisions
   Iterable<CollisionProspect<T>> query();
 }
-
-/// A [CollisionProspect] is a tuple that is used to contain two potentially
-/// colliding hitboxes.
-class CollisionProspect<T> {
-  T _a;
-  T _b;
-
-  T get a => _a;
-  T get b => _b;
-
-  int get hash => _hash;
-  int _hash;
-
-  CollisionProspect(this._a, this._b)
-    : _hash = _pairHash(_a.hashCode, _b.hashCode);
-
-  /// Computes a hash for an unordered pair of hash codes that is much less
-  /// likely to collide than a simple XOR.
-  static int _pairHash(int h1, int h2) {
-    return Object.hash(min(h1, h2), max(h1, h2));
-  }
-
-  /// Sets the prospect to contain [a] and [b] instead of what it previously
-  /// contained.
-  void set(T a, T b) {
-    _a = a;
-    _b = b;
-    _hash = _pairHash(a.hashCode, b.hashCode);
-  }
-
-  /// Sets the prospect to contain the content of [other].
-  void setFrom(CollisionProspect<T> other) {
-    _a = other._a;
-    _b = other._b;
-    _hash = other._hash;
-  }
-
-  /// Creates a new prospect object with the same content.
-  CollisionProspect<T> clone() => CollisionProspect(_a, _b);
-}

packages/flame/lib/src/collisions/broadphase/collision_prospect.dart

@@ -0,0 +1,29 @@
+import 'dart:math';
+
+import 'package:meta/meta.dart';
+
+/// A [CollisionProspect] is an immutable view of a pair of two potentially
+/// colliding hitboxes.
+///
+/// Equality is based on unordered pair identity: {A, B} == {B, A}.
+@immutable
+abstract class CollisionProspect<T> {
+  T get a;
+  T get b;
+
+  @override
+  int get hashCode {
+    final h1 = a.hashCode;
+    final h2 = b.hashCode;
+    return Object.hash(min(h1, h2), max(h1, h2));
+  }
+
+  @override
+  bool operator ==(Object other) {
+    if (other is! CollisionProspect) {
+      return false;
+    }
+    return (identical(a, other.a) && identical(b, other.b)) ||
+        (identical(a, other.b) && identical(b, other.a));
+  }
+}

packages/flame/lib/src/collisions/broadphase/prospect_pool.dart

@@ -1,25 +1,111 @@
-import 'package:flame/src/collisions/broadphase/broadphase.dart';
+import 'dart:collection';
+import 'dart:math';
+
+import 'package:flame/src/collisions/broadphase/collision_prospect.dart';
 import 'package:flame/src/collisions/hitboxes/hitbox.dart';
 
-/// This pool is used to not create unnecessary [CollisionProspect] objects
-/// during collision detection, but to re-use the ones that have already been
-/// created.
-class ProspectPool<T extends Hitbox<T>> {
+/// A pool of [CollisionProspect] objects that are reused across frames to avoid
+/// per-frame allocations.
+///
+/// Internally uses a private mutable subclass but only exposes the immutable
+/// [CollisionProspect] interface. Implements [Iterable] over acquired entries.
+class ProspectPool<T extends Hitbox<T>>
+    extends IterableBase<CollisionProspect<T>> {
   ProspectPool({this.incrementSize = 1000});
 
   /// How much the pool should increase in size every time it needs to be made
   /// larger.
   final int incrementSize;
-  final _storage = <CollisionProspect<T>>[];
-  int get length => _storage.length;
-
-  /// The size of the pool will expand with [incrementSize] amount of
-  /// [CollisionProspect]s that are initially populated with two [dummyItem]s.
-  void expand(T dummyItem) {
-    for (var i = 0; i < incrementSize; i++) {
-      _storage.add(CollisionProspect<T>(dummyItem, dummyItem));
+  final _storage = <_MutableCollisionProspect<T>>[];
+
+  /// The number of prospects currently acquired this frame.
+  @override
+  int get length => _count;
+  int _count = 0;
+
+  @override
+  Iterator<CollisionProspect<T>> get iterator =>
+      _ProspectPoolIterator<T>(_storage, _count);
+
+  /// Returns a [CollisionProspect] populated with [a] and [b], reusing a
+  /// pooled object or expanding the pool as needed.
+  CollisionProspect<T> acquire(T a, T b) {
+    if (_storage.length <= _count) {
+      _expand(a);
+    }
+    final prospect = _storage[_count]..set(a, b);
+    _count++;
+    return prospect;
+  }
+
+  /// Copies all prospects from [source] into the pool, expanding capacity at
+  /// most once.
+  void acquireAll(Iterable<CollisionProspect<T>> source) {
+    final needed = source is List ? source.length : null;
+    if (needed != null && _storage.length < _count + needed) {
+      _expand(source.first.a, needed: needed);
+    }
+    for (final prospect in source) {
+      acquire(prospect.a, prospect.b);
     }
   }
 
+  /// Resets the pool for the next frame. Previously acquired prospects should
+  /// not be accessed after this call.
+  void reset() {
+    _count = 0;
+  }
+
+  /// Returns the [CollisionProspect] at [index] (must be < [length]).
   CollisionProspect<T> operator [](int index) => _storage[index];
+
+  void _expand(T dummyItem, {int? needed}) {
+    final actualIncrement = needed == null
+        ? incrementSize
+        : max(needed, incrementSize);
+    final target = _storage.length + actualIncrement;
+    while (_storage.length < target) {
+      _storage.add(_MutableCollisionProspect<T>(dummyItem, dummyItem));
+    }
+  }
+}
+
+class _ProspectPoolIterator<T extends Hitbox<T>>
+    implements Iterator<CollisionProspect<T>> {
+  _ProspectPoolIterator(this._storage, this._length);
+
+  final List<_MutableCollisionProspect<T>> _storage;
+  final int _length;
+  int _index = -1;
+
+  @override
+  CollisionProspect<T> get current => _storage[_index];
+
+  @override
+  bool moveNext() => ++_index < _length;
+}
+
+/// Private mutable implementation of [CollisionProspect] used exclusively by
+/// [ProspectPool] to reuse objects across frames.
+///
+/// Safety: mutation only happens inside [ProspectPool.acquire] and
+/// [ProspectPool.reset], which are never called while prospects are stored in
+/// hash-based collections. All access outside this file is through the
+/// immutable [CollisionProspect] interface.
+// ignore: must_be_immutable
+class _MutableCollisionProspect<T> extends CollisionProspect<T> {
+  _MutableCollisionProspect(this._a, this._b);
+
+  T _a;
+  T _b;
+
+  @override
+  T get a => _a;
+  @override
+  T get b => _b;
+
+  void set(T a, T b) {
+    _a = a;
+    _b = b;
+  }
 }

packages/flame/lib/src/collisions/broadphase/quadtree/quad_tree_broadphase.dart

@@ -42,7 +42,7 @@ class QuadTreeBroadphase extends Broadphase<ShapeHitbox> {
 
   final _cachedCenters = <ShapeHitbox, Vector2>{};
 
-  final _potentials = <int, CollisionProspect<ShapeHitbox>>{};
+  final _potentials = <CollisionProspect<ShapeHitbox>>{};
   final _potentialsTmp = <ShapeHitbox>[];
   final _prospectPool = ProspectPool<ShapeHitbox>();
 
@@ -53,6 +53,7 @@ class QuadTreeBroadphase extends Broadphase<ShapeHitbox> {
   Iterable<CollisionProspect<ShapeHitbox>> query() {
     _potentials.clear();
     _potentialsTmp.clear();
+    _prospectPool.reset();
 
     for (final activeItem in activeHitboxes) {
       if (activeItem.isRemoving || !activeItem.isMounted) {
@@ -96,12 +97,8 @@ class QuadTreeBroadphase extends Broadphase<ShapeHitbox> {
         final item0 = _potentialsTmp[i];
         final item1 = _potentialsTmp[i + 1];
         if (broadphaseCheck(item0, item1)) {
-          final CollisionProspect<ShapeHitbox> prospect;
-          if (_prospectPool.length <= i) {
-            _prospectPool.expand(item0);
-          }
-          prospect = _prospectPool[i]..set(item0, item1);
-          _potentials[prospect.hash] = prospect;
+          final prospect = _prospectPool.acquire(item0, item1);
+          _potentials.add(prospect);
         } else {
           if (_broadphaseCheckCache[item0] == null) {
             _broadphaseCheckCache[item0] = {};
@@ -110,7 +107,7 @@ class QuadTreeBroadphase extends Broadphase<ShapeHitbox> {
         }
       }
     }
-    return _potentials.values;
+    return _potentials;
   }
 
   void updateTransform(ShapeHitbox item) {

packages/flame/lib/src/collisions/broadphase/sweep/sweep.dart

@@ -7,7 +7,6 @@ class Sweep<T extends Hitbox<T>> extends Broadphase<T> {
   final List<T> items;
 
   final _active = <T>[];
-  final _potentials = <int, CollisionProspect<T>>{};
   final _prospectPool = ProspectPool<T>();
 
   @override
@@ -24,7 +23,7 @@ class Sweep<T extends Hitbox<T>> extends Broadphase<T> {
   @override
   Iterable<CollisionProspect<T>> query() {
     _active.clear();
-    _potentials.clear();
+    _prospectPool.reset();
 
     for (final item in items) {
       if (item.collisionType == CollisionType.inactive) {
@@ -42,19 +41,14 @@ class Sweep<T extends Hitbox<T>> extends Broadphase<T> {
         if (activeBox.max.x >= currentMin) {
           if (item.collisionType == CollisionType.active ||
               activeItem.collisionType == CollisionType.active) {
-            if (_prospectPool.length <= _potentials.length) {
-              _prospectPool.expand(item);
-            }
-            final prospect = _prospectPool[_potentials.length]
-              ..set(item, activeItem);
-            _potentials[prospect.hash] = prospect;
+            _prospectPool.acquire(item, activeItem);
           }
         } else {
           _active.remove(activeItem);
         }
       }
       _active.add(item);
     }
-    return _potentials.values;
+    return _prospectPool;
   }
 }

packages/flame/lib/src/collisions/collision_detection.dart

@@ -15,7 +15,8 @@ abstract class CollisionDetection<
   final B broadphase;
 
   List<T> get items => broadphase.items;
-  final _lastPotentials = <CollisionProspect<T>>[];
+  final _lastProspectPool = ProspectPool<T>();
+  final _currentPotentials = <CollisionProspect<T>>{};
   final collisionsCompletedNotifier = CollisionDetectionCompletionNotifier();
 
   CollisionDetection({required this.broadphase});
@@ -36,9 +37,10 @@ abstract class CollisionDetection<
   void run() {
     broadphase.update();
     final potentials = broadphase.query();
-    final hashes = Set.unmodifiable(potentials.map((p) => p.hash));
+    _currentPotentials.clear();
 
     for (final potential in potentials) {
+      _currentPotentials.add(potential);
       final itemA = potential.a;
       final itemB = potential.b;
 
@@ -59,8 +61,8 @@ abstract class CollisionDetection<
 
     // Handles callbacks for an ended collision that the broadphase didn't
     // report as a potential collision anymore.
-    for (final prospect in _lastPotentials) {
-      if (!hashes.contains(prospect.hash) &&
+    for (final prospect in _lastProspectPool) {
+      if (!_currentPotentials.contains(prospect) &&
           prospect.a.collidingWith(prospect.b)) {
         handleCollisionEnd(prospect.a, prospect.b);
       }
@@ -71,20 +73,9 @@ abstract class CollisionDetection<
     collisionsCompletedNotifier.notifyListeners();
   }
 
-  final _lastPotentialsPool = <CollisionProspect<T>>[];
   void _updateLastPotentials(Iterable<CollisionProspect<T>> potentials) {
-    _lastPotentials.clear();
-    for (final potential in potentials) {
-      final CollisionProspect<T> lastPotential;
-      if (_lastPotentialsPool.length > _lastPotentials.length) {
-        lastPotential = _lastPotentialsPool[_lastPotentials.length]
-          ..setFrom(potential);
-      } else {
-        lastPotential = potential.clone();
-        _lastPotentialsPool.add(lastPotential);
-      }
-      _lastPotentials.add(lastPotential);
-    }
+    _lastProspectPool.reset();
+    _lastProspectPool.acquireAll(potentials);
   }
 
   /// Check what the intersection points of two items are,