Skip to content

Commit

Permalink
fix: Prevent onRemove/onDetach being called for initial Gesture D…
Browse files Browse the repository at this point in the history
…etector addition (#2653)

This fixes: #2647
Reverts: #2602
OBE: #2650

To be clear, the memory leak issue is not addressed because it is not a
Flame issue. Flame properly calls the `onRemove` event and if it is
needed to pass that on to children, users should do as documented:
https://docs.flame-engine.org/latest/flame/game.html#lifecycle.

So if you are using `GameWidget.controlled`, add a tappable component
and then remove the game, the lifecycle looks like:

```
flutter: onLoad
flutter: mount
flutter: onMount
flutter: attach
flutter: add has_tappable_components
flutter: refreshWidget 
// Remove Game
flutter: detach
flutter: removeGameStateListener
flutter: onRemove
flutter: remove
```

If you do not use `GameWidget.controlled`, add a tappable component and
then remove the game, the lifecycle looks like:

```
flutter: onLoad
flutter: mount
flutter: onMount
flutter: attach
flutter: add has_tappable_components
flutter: refreshWidget
// Remove Game
flutter: detach
flutter: removeGameStateListener
flutter: onRemove
flutter: remove
```

If you do not use `GameWidget.controlled`, do not add a tappable
component and then remove the game, the lifecycle looks like:

```
flutter: onLoadFuture
flutter: mount
flutter: onMount
flutter: attach
// Remove Game
flutter: detach
flutter: removeGameStateListener
flutter: onRemove
flutter: remove
```

Previously, the lifecycle would look like:

```
flutter: onLoad
flutter: mount
flutter: onMount
flutter: attach
flutter: add has_tappable_components
flutter: refreshWidget 
flutter: detach // These unnecessary calls have been eliminated
flutter: removeGameStateListener // These unnecessary calls have been eliminated
flutter: onRemove // These unnecessary calls have been eliminated
flutter: attach
// Remove Game
flutter: detach
flutter: removeGameStateListener
flutter: onRemove
flutter: remove
```

I have updated the below diagram for those who may need it in the
future:

![Flame and Flutter Events
Fixed](https://github.com/flame-engine/flame/assets/17042581/1d98bdaf-db13-4e2c-b0a6-74da2ada89f3)
  • Loading branch information
munsterlander authored Aug 20, 2023
1 parent 2f44e48 commit d172146
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 11 deletions.
3 changes: 2 additions & 1 deletion doc/flame/game.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,13 @@ Once the `GameWidget` is removed from the tree, `onRemove` is called, just like
component is removed from the component tree.

```{note}
The `onRemove` can be used to clean up potential memory leaks by doing the following:
The `onRemove` can be used to clean up potential memory leaks such as the following:
```

```dart
@override
void onRemove() {
// Optional based on your game needs.
removeAll(children);
processLifecycleEvents();
Flame.images.clearCache();
Expand Down
19 changes: 14 additions & 5 deletions packages/flame/lib/src/game/game.dart
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,10 @@ abstract mixin class Game {
);
}
_gameRenderBox = gameRenderBox;
onAttach();
if (!_isInternalRefresh) {
onAttach();
}
_isInternalRefresh = false;
}

/// Called when the game has been attached. This can be overridden
Expand All @@ -202,10 +205,10 @@ abstract mixin class Game {
///
/// Should not be called manually.
void detach() {
onRemove();
if (!_isInternalRefresh) {
onDetach();
}
_gameRenderBox = null;

onDetach();
}

/// Called when the game is about to be removed from the Flutter widget tree,
Expand Down Expand Up @@ -358,11 +361,17 @@ abstract mixin class Game {
gameStateListeners.remove(callback);
}

bool _isInternalRefresh = false;

/// When a Game is attached to a `GameWidget`, this method will force that
/// widget to be rebuilt. This can be used when updating any property which is
/// implemented within the Flutter tree.
///
/// When [isInternalRefresh] is passed as false it will trigger the `onAttach`
/// and `onDetach` events; otherwise, those events will not be called.
@internal
void refreshWidget() {
void refreshWidget({bool isInternalRefresh = true}) {
_isInternalRefresh = isInternalRefresh;
gameStateListeners.forEach((callback) => callback());
}
}
1 change: 1 addition & 0 deletions packages/flame/lib/src/game/game_widget/game_widget.dart
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ class GameWidgetState<T extends Game> extends State<GameWidget<T>> {
/// `currentGame`'s `onDispose` method will be called; otherwise, it will not.
void disposeCurrentGame({bool callGameOnDispose = false}) {
currentGame.removeGameStateListener(_onGameStateChange);
currentGame.onRemove();
if (callGameOnDispose) {
currentGame.onDispose();
}
Expand Down
10 changes: 5 additions & 5 deletions packages/flame/lib/src/game/overlay_manager.dart
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ class OverlayManager {
/// Clears all active overlays.
void clear() {
_activeOverlays.clear();
_game.refreshWidget();
_game.refreshWidget(isInternalRefresh: false);
}

/// Marks the [overlayName] to be rendered.
bool add(String overlayName) {
final setChanged = _addImpl(overlayName);
if (setChanged) {
_game.refreshWidget();
_game.refreshWidget(isInternalRefresh: false);
}
return setChanged;
}
Expand All @@ -42,7 +42,7 @@ class OverlayManager {
final initialCount = _activeOverlays.length;
overlayNames.forEach(_addImpl);
if (initialCount != _activeOverlays.length) {
_game.refreshWidget();
_game.refreshWidget(isInternalRefresh: false);
}
}

Expand All @@ -67,7 +67,7 @@ class OverlayManager {
bool remove(String overlayName) {
final hasRemoved = _activeOverlays.remove(overlayName);
if (hasRemoved) {
_game.refreshWidget();
_game.refreshWidget(isInternalRefresh: false);
}
return hasRemoved;
}
Expand All @@ -77,7 +77,7 @@ class OverlayManager {
final initialCount = _activeOverlays.length;
overlayNames.forEach(_activeOverlays.remove);
if (_activeOverlays.length != initialCount) {
_game.refreshWidget();
_game.refreshWidget(isInternalRefresh: false);
}
}

Expand Down

0 comments on commit d172146

Please sign in to comment.