fix!: Wait for children added in onLoad before marking a component as loaded

doc/flame/components/components.md

@@ -94,7 +94,8 @@ class MyComponent extends Component {
 A component's lifecycle state can be checked by a series of getters:
 
 - `isLoaded`: Returns a bool with the current loaded state.
-- `loaded`: Returns a future that will complete once the component has finished loading.
+- `loaded`: Returns a future that will complete once the component has finished loading, including
+  the loading of any children that were added during its `onLoad`.
 - `isMounted`: Returns a bool with the current mounted state.
 - `mounted`: Returns a future that will complete once the component has finished mounting.
 - `isRemoved`: Returns a bool with the current removed state.
@@ -263,6 +264,14 @@ been, and the parent is only mounted once its `onLoad` has completed, so those f
 deadlock. The same goes for `game.lifecycleEventsProcessed`, since the parent's own pending mount is
 part of the queue it waits for.
 
+A component does not count as loaded until every child that was added during its `onLoad` has
+finished loading as well, even without awaiting their `loaded` futures explicitly. This means that
+by the time the component mounts, the subtree it created during `onLoad` is fully loaded, and those
+children mount together with it in the same lifecycle processing pass. A child that fails to load
+is the exception: it is dropped from the tree without blocking its parent. Because the parent now
+waits for its children, a child's `onLoad` must not await the parent's `loaded` future, that would
+deadlock.
+
 Note that the children added via either method are only guaranteed to be available eventually:
 after they are loaded and mounted. We can only assure that they will appear in the children list
 in the same order as they were scheduled for addition.

packages/flame/lib/src/components/core/component.dart

@@ -192,7 +192,8 @@ class Component {
   void _setLoadingBit() => _state |= _loading;
   void _clearLoadingBit() => _state &= ~_loading;
 
-  /// Whether this component has completed its [onLoad] step.
+  /// Whether this component has completed its [onLoad] step, including the
+  /// loading of every child that was added during [onLoad].
   bool get isLoaded => (_state & _loaded) != 0;
   void _setLoadedBit() => _state |= _loaded;
 
@@ -232,6 +233,11 @@ class Component {
 
   /// A future that completes when this component finishes loading.
   ///
+  /// A component only counts as finished loading once every child that was
+  /// added during its [onLoad] has finished loading as well, so awaiting
+  /// this future guarantees that the subtree created during [onLoad] is
+  /// loaded.
+  ///
   /// If the component is already loaded (see [isLoaded]), this returns an
   /// already completed future. If [onLoad] threw, this returns a future that
   /// completes with that error, every time it is read.
@@ -814,6 +820,9 @@ class Component {
     } else {
       _children?.remove(child);
       child._parent = null;
+      if (isLoading) {
+        _notifyChildrenChangedWhileLoading();
+      }
     }
   }
 
@@ -1073,7 +1082,68 @@ class Component {
     }
   }
 
+  /// Finishes the load step once every child that is still loading has
+  /// settled as well, so that a component is only marked as loaded when the
+  /// children that were added during its [onLoad] have finished loading too.
   void _finishLoading() {
+    if (_loadingChildren().isEmpty) {
+      _completeLoading();
+    } else {
+      _waitForLoadingChildren().then((_) => _completeLoading());
+    }
+  }
+
+  /// Waits until no child of this component is loading anymore.
+  ///
+  /// Children whose load has failed do not count as loading; they are
+  /// dropped when this component mounts, the same way as when they fail to
+  /// load under a parent that is already mounted.
+  Future<void> _waitForLoadingChildren() async {
+    var wake = Completer<void>();
+    void wakeUp() {
+      if (!wake.isCompleted) {
+        wake.complete();
+      }
+    }
+
+    final watchedChildren = <Component>{};
+    while (true) {
+      final loadingChildren = _loadingChildren();
+      if (loadingChildren.isEmpty) {
+        return;
+      }
+      if (wake.isCompleted) {
+        wake = Completer<void>();
+      }
+      for (final child in loadingChildren) {
+        if (watchedChildren.add(child)) {
+          child.loadSettled.then((_) => wakeUp());
+        }
+      }
+      // Sleep until a child settles its load, or until a child is removed
+      // while this component is loading, and re-evaluate.
+      await Future.any([
+        wake.future,
+        (_childrenChangedWhileLoading ??= Completer<void>()).future,
+      ]);
+    }
+  }
+
+  /// The children whose loads still have to settle before this component can
+  /// be considered loaded.
+  List<Component> _loadingChildren() {
+    final children = _children;
+    if (children == null || children.isEmpty) {
+      return const [];
+    }
+    return [
+      for (final child in children)
+        if (child.isLoading && child._loadError == null) child,
+    ];
+  }
+
+  void _completeLoading() {
+    _childrenChangedWhileLoading = null;
     _clearLoadingBit();
     _setLoadedBit();
     _loadCompleter?.complete();
@@ -1086,6 +1156,16 @@ class Component {
     _loadSettledCompleter = null;
   }
 
+  /// Completed when the children set changes while this component is
+  /// loading, so that the pending [_finishLoading] gate re-evaluates, for
+  /// example when a child that never finishes loading is removed.
+  Completer<void>? _childrenChangedWhileLoading;
+
+  void _notifyChildrenChangedWhileLoading() {
+    _childrenChangedWhileLoading?.complete();
+    _childrenChangedWhileLoading = null;
+  }
+
   /// Surfaces an error thrown by [onLoad].
   ///
   /// Since [add] is synchronous and no longer returns the loading future,
@@ -1183,7 +1263,7 @@ class Component {
   /// Used by the [FlameGame] to set the loaded state of the component, since
   /// the game isn't going through the whole normal component life cycle.
   @internal
-  void setLoaded() => _finishLoading();
+  void setLoaded() => _completeLoading();
 
   /// Used by the [FlameGame] to set the mounted state of the component, since
   /// the game isn't going through the whole normal component life cycle.

packages/flame/test/components/component_test.dart

@@ -597,6 +597,133 @@ void main() {
         });
       });
 
+      group('loading with children', () {
+        testWithFlameGame(
+          'a component is not loaded until a child added in onLoad is loaded',
+          (game) async {
+            final childLoadGate = Completer<void>();
+            final parent = _ParentWithGatedChild(childLoadGate);
+            game.world.add(parent);
+            game.update(0);
+
+            expect(parent.isLoading, isTrue);
+            expect(parent.isLoaded, isFalse);
+            expect(parent.isMounted, isFalse);
+
+            childLoadGate.complete();
+            await parent.loaded;
+
+            expect(parent.child.isLoaded, isTrue);
+            game.update(0);
+            expect(parent.isMounted, isTrue);
+            expect(parent.child.isMounted, isTrue);
+          },
+        );
+
+        testWithFlameGame(
+          'a component is not loaded until its whole subtree is loaded',
+          (game) async {
+            final grandChildLoadGate = Completer<void>();
+            final grandParent = _GrandParentWithGatedGrandChild(
+              grandChildLoadGate,
+            );
+            game.world.add(grandParent);
+            game.update(0);
+
+            expect(grandParent.isLoading, isTrue);
+            expect(grandParent.child.isLoading, isTrue);
+            expect(grandParent.isLoaded, isFalse);
+
+            grandChildLoadGate.complete();
+            await grandParent.loaded;
+
+            expect(grandParent.child.isLoaded, isTrue);
+            expect(grandParent.child.child.isLoaded, isTrue);
+
+            await game.ready();
+            expect(grandParent.isMounted, isTrue);
+            expect(grandParent.child.isMounted, isTrue);
+            expect(grandParent.child.child.isMounted, isTrue);
+          },
+        );
+
+        testWithFlameGame(
+          'children added before the parent starts loading do not gate it',
+          (game) async {
+            // Children that are not loading yet when the parent finishes its
+            // own onLoad, such as children given to the constructor of a
+            // detached component, keep loading when the parent mounts.
+            final childLoadGate = Completer<void>();
+            final child = _GatedLoadComponent(childLoadGate);
+            final parent = Component(children: [child]);
+            game.world.add(parent);
+            game.update(0);
+
+            expect(parent.isMounted, isTrue);
+            expect(child.isLoading, isTrue);
+
+            childLoadGate.complete();
+            await game.ready();
+            expect(child.isMounted, isTrue);
+          },
+        );
+
+        testWithFlameGame(
+          'when the parent mounts the whole subtree is mounted',
+          (game) async {
+            final childLoadGate = Completer<void>();
+            final parent = _ParentWithGatedChild(childLoadGate);
+            game.world.add(parent);
+
+            childLoadGate.complete();
+            final readyFuture = game.ready();
+            await parent.mounted;
+
+            // The child mounts in the same lifecycle processing pass as the
+            // parent, so once the parent is mounted the whole subtree is.
+            expect(parent.child.isMounted, isTrue);
+            await readyFuture;
+          },
+        );
+
+        testWithFlameGame(
+          'a child that fails loading does not block its parent',
+          (game) async {
+            final parent = _ParentWithFailingChild();
+            game.world.add(parent);
+            final loaded = parent.child.loaded;
+
+            await expectLater(loaded, throwsA(isA<_LoadException>()));
+            await parent.loaded;
+            await game.ready();
+
+            expect(parent.isMounted, isTrue);
+            expect(parent.children, isEmpty);
+            expect(parent.child.isMounted, isFalse);
+            expect(parent.child.parent, isNull);
+          },
+        );
+
+        testWithFlameGame(
+          'removing a child that never loads unblocks the parent',
+          (game) async {
+            final neverCompletingGate = Completer<void>();
+            final parent = _ParentWithGatedChild(neverCompletingGate);
+            game.world.add(parent);
+            game.update(0);
+            expect(parent.isLoaded, isFalse);
+
+            parent.remove(parent.child);
+            await parent.loaded;
+            await game.ready();
+
+            expect(parent.isMounted, isTrue);
+            expect(parent.child.parent, isNull);
+            expect(parent.child.isMounted, isFalse);
+          },
+        );
+      });
+
       testWithFlameGame('Can wait for lifecycleEventsProcessed', (game) async {
         await game.ready();
         final component = Component();
@@ -2159,6 +2286,51 @@ class _SlowComponent extends Component {
   String toString() => 'SlowComponent($name, loadTime=$loadTime)';
 }
 
+class _GatedLoadComponent extends Component {
+  _GatedLoadComponent(this.loadGate);
+
+  final Completer<void> loadGate;
+
+  @override
+  Future<void> onLoad() => loadGate.future;
+}
+
+class _ParentWithGatedChild extends Component {
+  _ParentWithGatedChild(this.childLoadGate);
+
+  final Completer<void> childLoadGate;
+  late final _GatedLoadComponent child;
+
+  @override
+  void onLoad() {
+    child = _GatedLoadComponent(childLoadGate);
+    add(child);
+  }
+}
+
+class _ParentWithFailingChild extends Component {
+  late final _FailingLoadComponent child;
+
+  @override
+  void onLoad() {
+    child = _FailingLoadComponent();
+    add(child);
+  }
+}
+
+class _GrandParentWithGatedGrandChild extends Component {
+  _GrandParentWithGatedGrandChild(this.grandChildLoadGate);
+
+  final Completer<void> grandChildLoadGate;
+  late final _ParentWithGatedChild child;
+
+  @override
+  void onLoad() {
+    child = _ParentWithGatedChild(grandChildLoadGate);
+    add(child);
+  }
+}
+
 class _SelfRemovingOnLoadComponent extends Component {
   @override
   Future<void>? onLoad() {