Dispose method for `FlameGame`

[imaNNeo]

Current bug behavior

When the FlameGame gets disposed (page closes), it should remove all components of itself (call onRemove() on all the sub-components to let them clear their resources) First of all, when we close the game page (which contains a game), it does not automatically call remove on the child objects, to do that we need to write the below code manually:

@override
void onRemove() {
  removeAll(children);
  super.onRemove();
}

This onRemove() method on the game that extends FlameGame gets called when the page closes, but when we call removeAll(children);, it adds the components to the lifecycle._removals queue. But it never processes this queue. Because GameWidget is removed from the widget tree and there is no other tick to call lifecycle.processQueues(). So it leads to a memory leak in some components such as FlameBlocListenable because onRemove will never be called and subscription leaks.

Expected behavior

I expect that FlameGame call removeAll(children); under the hood and process the removal queue before the game disposes to allow all sub-components to free their resources in the onRemove() function.

Steps to reproduce

I wanted to put a Zapp link, but it seems Zapp is down at this moment, so there is a main.dart file here. You just need to add these dependencies to run the reproducible code:

flame_bloc: ^1.8.2
flutter_bloc: ^8.1.2
flame: 1.6.0

Steps:

1. Run the game, then HomePage opens
2. Click on the Play button on the HomePage, then GamePage opens.
3. Click the plus button
4. LeakingCounterComponent.onNewState(1) prints in the screen
5. press the back button
6. MyGame.onRemove() is called, but LeakingCounterComponent.onRemove() didn't call, so it leaked through it's subscription.
7. Click the Play and plus button again
8. Now you see LeakingCounterComponent.onNewState(2) printed 2 times (because there is a leak)
9. press the back button again
10. Click the Play and plus button again
11. Now you see LeakingCounterComponent.onNewState(3) printed 3 times (because there are two leaks)

As a workaround, I wrote the below code:

@override
void onRemove() {
  ...
  removeAll(children);
  processPendingLifecycleEvents();
  super.onRemove();
}

So with this code, LeakingCounterComponent.onRemove() gets called everytime we close the game page

Flutter doctor output

Doctor summary (to see all details, run flutter doctor -v):
[✓] Flutter (Channel stable, 3.7.1, on macOS 13.2.1 22D68 darwin-arm64, locale
    en-US)
[✓] Android toolchain - develop for Android devices (Android SDK version 33.0.0)
[✓] Xcode - develop for iOS and macOS (Xcode 14.1)
[✓] Chrome - develop for the web
[✓] Android Studio (version 2021.3)
[✓] IntelliJ IDEA Community Edition (version 2022.2.4)
[✓] IntelliJ IDEA Community Edition (version 2022.3.1)
[✓] IntelliJ IDEA Community Edition (version 2022.3.2)
[✓] VS Code (version 1.76.0)
[✓] Connected device (2 available)
[✓] HTTP Host Availability

• No issues found!

[spydon]

I would say that this is up to the user to handle. Because we can't know whether the game is going to be re-used later or not, and then the state shouldn't be changed. For example a game might not only contain of a FlameGame class that is always active throughout the life of a game session, there might for example be other Flutter screens in a game and then the game shouldn't remove all components when you navigate away from a screen showing the GameWidget.

[erickzanardo]

I agree with @spydon , we can't know, and it should be up to the user, just like flutter's classes like animation controller, text editing controller and other controllers, wher you need to call dispose yourself

But I wonder if we could have a dispose method on game? That would instead of going through the component lifecycle, just iterate over the components and their remove method

[spydon]

But I wonder if we could have a dispose method on game? That would instead of going through the component lifecycle, just iterate over the components and their remove method

That could be a good idea! I don't see any harm in doing it through the lifecycle though.

[erickzanardo]

The only concern Thani have doing in the life cycle is that it is an async op right? Maybe if the game is "gced" before everything is disposed we could still end up with resources not being disposed?

I am just speculating at this point, so if that is not a possibility then yeah, we could do through the lifecycle

[imaNNeo]

But I wonder if we could have a dispose method on game? That would instead of going through the component lifecycle, just iterate over the components and their remove method

It makes sense. Calling dispose() is like a convention in the Flutter world.

[imaNNeo]

The only concern Thani have doing in the life cycle is that it is an async op right? Maybe if the game is "gced" before everything is disposed we could still end up with resources not being disposed?

Why is it an async operation? As I see both removeAll(children) and processPendingLifecycleEvents() are void functions.

[erickzanardo]

You are correct, it is not async, I am confusing with the game.ready method that we using in testing.

So maybe have a dispose method just to follow flutter standards, call remove of their children and dispose all the images cached that it may contain would be enough I guess

[renancaraujo]

This would imply another lifecycle method on components since component removal is not necessarily the same as disposal. Components can be removed due to game logic, but a game could only be disposed of due to the widget lifecycle.

[munsterlander]

So based on my recent pr and discovery / charting of the life cycle, I also feel into the trap of thinking onRemove was not properly being called only to later realize that FlameGame is a component and should be treated like other components, meaning just because the game has been removed from the flutter widget tree, does not mean that the actual game state should be wiped by default. Counter to this enhancement was the bug I was fixing, in that they wanted to attach / detach the game and come back to where it was left. So implementing this enhancement would break that work flow.

I do think that for GameRenderBox, there could be a passed super parameter to dispose all children and process life cycle events. That would then solve both cases and allow the user the option of what to do. Thoughts?

[spydon]

I still think of it like this: https://github.com/flame-engine/flame/issues/2403#issuecomment-1466052287

We could create a dispose method, but there is no natural place to call it automatically from, so the user would have to trigger it manually anyways and then they can just as well create that dispose method themselves.

What we could have in such a pre-created dispose method is emptying the caches though.

[ebelevics]

Was really surprised that such functionally was not implemented, as game objects can be changed frequently. Thankfully it seems that all flame objects are components, and it was not so hard to implement dispose function recursively:

extension ComponentExt on Component {
  void dispose() {
    for (final c in children) {
      c.dispose();
      c.onRemove();
    }
  }
}