feat: Add collision completed listener #2896

christian-mindfulness · 2023-12-02T19:41:20Z

Description

Add a new variable to the CollisionDetection class. Call notifyListeners at the end of the collision detection step, so that users can add a listener to their code to know when this step has finished. The variable is a basic implementation of a Listenable class, since it needs no more complexity.

Checklist

I have followed the Contributor Guide when preparing my PR.
I have updated/added tests for ALL new/updated/fixed functionality.
I have updated/added relevant documentation in docs and added dartdoc comments with ///.
I have updated/added relevant examples in examples or docs.

Breaking Change?

Yes, this PR is a breaking change.
No, this PR is not a breaking change.

Related Issues

Closes #2849

This reverts commit f4ab7b9.

This reverts commit 4b8bdd5.

This reverts commit 5d42eed.

spydon

Some comments, and missing tests

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

spydon · 2023-12-04T08:41:40Z

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

@@ -161,3 +166,24 @@ abstract class CollisionDetection<T extends Hitbox<T>,
    List<RaycastResult<T>>? out,
  });
 }
+
+/// Add a basic class which implements a Listener.
+class CollisionDetectionCompletionNotifier implements Listenable {


Suggested change

class CollisionDetectionCompletionNotifier implements Listenable {

class CollisionDetectionCompletionNotifier with ChangeNotifier {

No need to manually implement Listenable, check https://api.flutter.dev/flutter/foundation/ChangeNotifier-class.html

I think the suggestion to implement Listenable came from you, to avoid the warning about unnecessary overrides. Happy to do whichever you think best.

Ah, now I remember. 🤦‍♂️
I think it is probably better to go with with ChangeNotifier anyways, so that we don't have to do the re-implementation, and then we can just put an ignore lint lint above the overridden method.

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

christian-mindfulness · 2023-12-04T15:34:35Z

Some comments, and missing tests

I appreciate the need for tests, but the testing framework is very confusing to me. @spydon would you be able to suggest a change for this?

spydon · 2023-12-04T15:48:57Z

I appreciate the need for tests, but the testing framework is very confusing to me. @spydon would you be able to suggest a change for this?

Yeah, the collision detection tests can be a bit confusing, this one can be fairly simple though, something like this:

group('collisionsCompletedNotifier', {
  runCollisionTestRegistry({
    'collisionsCompletedNotifier calls listeners': (game) async {
      var calledTimes = 0;
      final listeners = List.generate(10, (_) => () => calledTimes++);
      for(final listener in listeners) {
        game.collisionDetection.collisionsCompletedNotifier.addListener(listener);
      }
      game.update(0);
  
      expect(calledTimes, listeners.length);
    },
  });
});

and then just adding that in the end of this file:
https://github.com/flame-engine/flame/blob/main/packages/flame/test/collisions/collision_callback_test.dart

christian-mindfulness · 2023-12-04T16:35:44Z

...
and then just adding that in the end of this file: https://github.com/flame-engine/flame/blob/main/packages/flame/test/collisions/collision_callback_test.dart

Thanks. Done that and managed to get the test to run, but fails:
Expected: <10>
Actual: <0>
I think the code might be adding the listeners, but not running collision detection and so the counter is not incremented?

spydon · 2023-12-04T16:39:40Z

@christian-mindfulness did you add the game.update(0);? I forgot to add that at first.

christian-mindfulness · 2023-12-04T17:30:06Z

@christian-mindfulness did you add the game.update(0);? I forgot to add that at first.

Thanks the one, thank you.

spydon

A few final comments

doc/flame/collision_detection.md

spydon · 2023-12-04T20:10:54Z

doc/flame/collision_detection.md

+(game as HasCollisionDetection).collisionDetection.collisionsCompletedNotifier.
+                                addListener(() {
+  resolveCollisions();
+});


I have to break the line (as it's too long otherwise). I've now done it in a way that looks nice to me, happy to hear your thoughts.

if you put it in an IDE and or https://dartpad.dev it will correctly format it for you, since it follows 80 width by default

Couldn't get VScode, dartpad or tutorialspoint to do it automatically for me. :-(

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

packages/flame/test/collisions/collision_detection_test.dart

christian-mindfulness · 2023-12-06T20:52:40Z

Thanks @spydon for the review. I think my latest commit addresses all the issues. Let me know if I've missed anything. I've also simplified the extension of ChangeNotifier to have an empty body, which avoids any warnings.

spydon · 2023-12-06T20:55:19Z

Thanks @spydon for the review. I think my latest commit addresses all the issues. Let me know if I've missed anything. I've also simplified the extension of ChangeNotifier to have an empty body, which avoids any warnings.

You have to override notifyListeners since super.notifyListeners isn't exposed, only for tests.

christian-mindfulness · 2023-12-06T21:07:36Z

Thanks @spydon for the review. I think my latest commit addresses all the issues. Let me know if I've missed anything. I've also simplified the extension of ChangeNotifier to have an empty body, which avoids any warnings.

You have to override notifyListeners since super.notifyListeners isn't exposed, only for tests.

Curious that it worked for me - I've changed back.

spydon · 2023-12-06T21:22:34Z

Curious that it worked for me - I've changed back.

It works, but you get an analyzer warning. That was why the pipeline didn't pass.

spydon · 2023-12-06T21:26:11Z

doc/flame/collision_detection.md

+(game as HasCollisionDetection).collisionDetection
+                               .collisionsCompletedNotifier
+                               .addListener(() {
+  resolveCollisions();
+});


Suggested change

(game as HasCollisionDetection).collisionDetection

.collisionsCompletedNotifier

.addListener(() {

resolveCollisions();

});

(game as HasCollisionDetection)

.collisionDetection

.collisionsCompletedNotifier

.addListener(() {

resolveCollisions();

});

This is the correct formatting

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

spydon

Thanks for your contribution! :)

christian-mindfulness and others added 15 commits November 11, 2023 10:50

First attempt at creating onCollisionsCompleted

4b8bdd5

Bug fix which gets things working.

5d42eed

First attempt at adding tests

f4ab7b9

Revert "First attempt at adding tests"

76d2970

This reverts commit f4ab7b9.

Revert "First attempt at creating onCollisionsCompleted"

1216440

This reverts commit 4b8bdd5.

Revert "Bug fix which gets things working."

e9dba44

This reverts commit 5d42eed.

Add ChangeNotifier to collision detection.

d49d3da

Add an override of the ChaneNotifier class and start documentation.

05db541

Add a description of new ChangeNotifier override.

015f9bd

Remove changes from shape hitbox.

92eb7c5

Switch from ChangeNotifier to implementing Listenable.

e6256db

Merge branch 'main' into add_collision_completed

09fec9c

Update comment for Listener class

21e8725

Fix markdown lint failure

362ad63

Merge branch 'main' into add_collision_completed

82b6745

spydon reviewed Dec 4, 2023

View reviewed changes

christian-mindfulness and others added 2 commits December 4, 2023 15:09

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

9d74231

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

Change name of notifier, as requested in review

834c950

Notify class inherits from ChangeNotifier, rather than Listenable.

2b9c946

Add first version of notifier test.

66879ca

Bug fix on flame test.

8e16121

spydon reviewed Dec 4, 2023

View reviewed changes

Fixes following @spydon's review.

6b779c2

Fix change notifier.

13b3905

spydon reviewed Dec 6, 2023

View reviewed changes

Update formatting in docs

2f9fc20

spydon approved these changes Dec 6, 2023

View reviewed changes

Merge branch 'main' into add_collision_completed

a9308c9

spydon enabled auto-merge (squash) December 6, 2023 21:44

spydon merged commit 957db3c into flame-engine:main Dec 6, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add collision completed listener #2896

feat: Add collision completed listener #2896

christian-mindfulness commented Dec 2, 2023 •

edited

spydon left a comment

spydon Dec 4, 2023

spydon Dec 4, 2023

christian-mindfulness Dec 4, 2023

spydon Dec 4, 2023

christian-mindfulness commented Dec 4, 2023

spydon commented Dec 4, 2023 •

edited

christian-mindfulness commented Dec 4, 2023

spydon commented Dec 4, 2023

christian-mindfulness commented Dec 4, 2023

spydon left a comment

spydon Dec 4, 2023

christian-mindfulness Dec 6, 2023

spydon Dec 6, 2023

christian-mindfulness Dec 6, 2023

christian-mindfulness commented Dec 6, 2023

spydon commented Dec 6, 2023

christian-mindfulness commented Dec 6, 2023

spydon commented Dec 6, 2023

spydon Dec 6, 2023

spydon left a comment

	class CollisionDetectionCompletionNotifier implements Listenable {
	class CollisionDetectionCompletionNotifier with ChangeNotifier {

feat: Add collision completed listener #2896

feat: Add collision completed listener #2896

Conversation

christian-mindfulness commented Dec 2, 2023 • edited

Description

Checklist

Breaking Change?

Related Issues

spydon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christian-mindfulness commented Dec 4, 2023

spydon commented Dec 4, 2023 • edited

christian-mindfulness commented Dec 4, 2023

spydon commented Dec 4, 2023

christian-mindfulness commented Dec 4, 2023

spydon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christian-mindfulness commented Dec 6, 2023

spydon commented Dec 6, 2023

christian-mindfulness commented Dec 6, 2023

spydon commented Dec 6, 2023

Choose a reason for hiding this comment

spydon left a comment

Choose a reason for hiding this comment

christian-mindfulness commented Dec 2, 2023 •

edited

spydon commented Dec 4, 2023 •

edited