fix: Use the hitboxParent instead of the parent in the componentTypeC…

…heck (#2335)

* Fix benchmark file name to run as test

* QuadTree now uses a parentHitbox instead of parent

* Add tests

* Apply suggestions from code review

Co-authored-by: Lukas Klingsbo <lukas.klingsbo@gmail.com>

* Replace the var name

* Revert mark as final

* Change default parameter name

---------

Co-authored-by: Lukas Klingsbo <lukas.klingsbo@gmail.com>

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

@@ -1,5 +1,4 @@
 import 'package:flame/collisions.dart';
-import 'package:flame/components.dart';
 import 'package:flame/extensions.dart';
 import 'package:flame/game.dart';
 
@@ -66,28 +65,9 @@ mixin HasQuadTreeCollisionDetection on FlameGame
             (activeItemCenter.y - potentialCenter.y).abs() > minimumDistance!);
   }
 
-  bool onComponentTypeCheck(PositionComponent one, PositionComponent another) {
-    var checkParent = false;
-    if (one is GenericCollisionCallbacks) {
-      if (!(one as GenericCollisionCallbacks).onComponentTypeCheck(another)) {
-        return false;
-      }
-    } else {
-      checkParent = true;
-    }
-
-    if (another is GenericCollisionCallbacks) {
-      if (!(another as GenericCollisionCallbacks).onComponentTypeCheck(one)) {
-        return false;
-      }
-    } else {
-      checkParent = true;
-    }
-
-    if (checkParent && one is ShapeHitbox && another is ShapeHitbox) {
-      return onComponentTypeCheck(one.hitboxParent, another.hitboxParent);
-    }
-    return true;
+  bool onComponentTypeCheck(ShapeHitbox first, ShapeHitbox second) {
+    return first.onComponentTypeCheck(second) &&
+        second.onComponentTypeCheck(first);
   }
 
   @override

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

@@ -1,12 +1,11 @@
 import 'dart:collection';
 
 import 'package:flame/collisions.dart';
-import 'package:flame/components.dart';
 import 'package:flame/extensions.dart';
 
 typedef ExternalBroadphaseCheck = bool Function(
-  PositionComponent one,
-  PositionComponent another,
+  ShapeHitbox first,
+  ShapeHitbox second,
 );
 
 typedef ExternalMinDistanceCheck = bool Function(

packages/flame/lib/src/collisions/hitboxes/shape_hitbox.dart

@@ -213,16 +213,27 @@ mixin ShapeHitbox on ShapeComponent implements Hitbox<ShapeHitbox> {
     }
   }
 
+  /// Defines whether the [other] component should be able to collide with
+  /// this component.
+  ///
+  /// If the [hitboxParent] is not `CollisionCallbacks` but `PositionComponent`,
+  /// there is no [CollisionCallbacks.onComponentTypeCheck] in that component.
+  /// As a result, it will always be able to collide with all other types.
   @override
   @mustCallSuper
   bool onComponentTypeCheck(PositionComponent other) {
-    final myParent = parent;
-    final otherParent = other.parent;
-    if (myParent is CollisionCallbacks && otherParent is PositionComponent) {
-      return myParent.onComponentTypeCheck(otherParent);
-    }
+    final otherHitboxParent = (other as ShapeHitbox).hitboxParent;
+
+    final thisCanCollideWithOther = (hitboxParent is! CollisionCallbacks) ||
+        (hitboxParent as CollisionCallbacks)
+            .onComponentTypeCheck(otherHitboxParent);
+
+    final otherCanCollideWithThis =
+        (otherHitboxParent is! CollisionCallbacks) ||
+            (otherHitboxParent as CollisionCallbacks)
+                .onComponentTypeCheck(hitboxParent);
 
-    return true;
+    return thisCanCollideWithOther && otherCanCollideWithThis;
   }
 
   @override

packages/flame/test/collisions/collision_callback_test.dart

@@ -110,6 +110,60 @@ void main() {
         expect(hitboxA.endCounter, 1);
         expect(hitboxB.endCounter, 1);
       },
+      'hitboxes are in any descendants': (game) async {
+        // The hitboxParent of the `testHitbox` will be the `player`.
+        final player = TestBlock(
+          Vector2.all(0),
+          Vector2.all(10),
+          addTestHitbox: false,
+          children: [
+            Component(
+              children: [
+                Component(children: [TestHitbox()]),
+              ],
+            ),
+          ],
+        );
+        final block = TestBlock(Vector2.all(100), Vector2.all(10));
+
+        await game.ensureAddAll([player, block]);
+
+        expect(block.startCounter, 0);
+        expect(block.onCollisionCounter, 0);
+        expect(block.endCounter, 0);
+
+        // player <== collides with  ==> block
+        block.position = Vector2.all(5);
+        game.update(0);
+
+        expect(block.startCounter, 1);
+        expect(block.onCollisionCounter, 1);
+        expect(block.endCounter, 0);
+      },
+      'hitboxParent is PositionComponent but not CollisionCallbacks':
+          (game) async {
+        final player = PositionComponent(
+          position: Vector2.all(0),
+          size: Vector2.all(10),
+          children: [
+            Component(
+              children: [
+                Component(children: [TestHitbox()]),
+              ],
+            ),
+          ],
+        );
+        final block = TestBlock(Vector2.all(100), Vector2.all(10));
+
+        await game.ensureAddAll([player, block]);
+
+        block.position = Vector2.all(5);
+        game.update(0);
+
+        expect(block.startCounter, 1);
+        expect(block.onCollisionCounter, 1);
+        expect(block.endCounter, 0);
+      },
     });
   });
 
@@ -459,4 +513,65 @@ void main() {
       expect(outerBlock.endCounter, 1);
     },
   });
+
+  group('ComponentTypeCheck(only supported in the QuadTree)', () {
+    testQuadTreeCollisionDetectionGame(
+      'makes the correct Component type start to collide',
+      (game) async {
+        final water = Water(
+          position: Vector2.all(0),
+          size: Vector2.all(10),
+          children: [
+            Component(
+              children: [
+                Component(children: [TestHitbox()]),
+              ],
+            ),
+          ],
+        );
+        final brick = Brick(
+          position: Vector2.all(50),
+          size: Vector2.all(10),
+          children: [
+            Component(
+              children: [
+                Component(children: [TestHitbox()]),
+              ],
+            ),
+          ],
+        );
+
+        final block = TestBlock(
+          Vector2.all(100),
+          Vector2.all(10),
+          onComponentTypeCheck: (other) {
+            if (other is Water) {
+              return false;
+            }
+            return true;
+          },
+        );
+
+        await game.ensureAddAll([water, brick, block]);
+
+        // block <== collides with  ==> water
+        // But as [TestBlock.onComponentTypeCheck] returns false with Water,
+        // they do not actually start to collide.
+        block.position = Vector2.all(5);
+        game.update(0);
+
+        expect(block.startCounter, 0);
+        expect(block.onCollisionCounter, 0);
+        expect(block.endCounter, 0);
+
+        // block <== collides with  ==> brick
+        block.position = Vector2.all(55);
+        game.update(0);
+
+        expect(block.startCounter, 1);
+        expect(block.onCollisionCounter, 1);
+        expect(block.endCounter, 0);
+      },
+    );
+  });
 }

packages/flame/test/collisions/collision_test_helpers.dart

@@ -90,14 +90,18 @@ class TestBlock extends PositionComponent with CollisionCallbacks {
   int onCollisionCounter = 0;
   int endCounter = 0;
 
+  final bool Function(PositionComponent other)? _onComponentTypeCheck;
+
   TestBlock(
     Vector2 position,
     Vector2 size, {
     CollisionType type = CollisionType.active,
     bool addTestHitbox = true,
     super.children,
     this.name,
-  }) : super(
+    bool Function(PositionComponent other)? onComponentTypeCheck,
+  })  : _onComponentTypeCheck = onComponentTypeCheck,
+        super(
           position: position,
           size: size,
         ) {
@@ -155,4 +159,18 @@ class TestBlock extends PositionComponent with CollisionCallbacks {
     super.onCollisionEnd(other);
     endCounter++;
   }
+
+  @override
+  bool onComponentTypeCheck(PositionComponent other) {
+    return (_onComponentTypeCheck?.call(other) ?? true) &&
+        super.onComponentTypeCheck(other);
+  }
+}
+
+class Water extends PositionComponent {
+  Water({super.position, super.size, super.children});
+}
+
+class Brick extends PositionComponent {
+  Brick({super.position, super.size, super.children});
 }