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.

doc/flame/game.md

@@ -127,9 +127,12 @@ The `FlameGame` lifecycle callbacks, `onLoad`, `render`, etc. are called in the
 ```
 
 When a `FlameGame` is first added to a `GameWidget` the lifecycle methods `onGameResize`, `onLoad`
-and `onMount` will be called in that order. Then `update` and `render` are called in sequence for
-every game tick. If the `FlameGame` is removed from the `GameWidget` then `onRemove` is called.
-If the `FlameGame` is added to a new `GameWidget` the sequence repeats from `onGameResize`.
+and `onMount` will be called in that order. After that, the `GameWidget` waits for the whole initial
+component tree to be loaded and mounted, so the game does not start (and the `loadingBuilder`
+widget, if one is set, stays visible) until every component added during `onLoad` is ready. Then
+`update` and `render` are called in sequence for every game tick. If the `FlameGame` is removed
+from the `GameWidget` then `onRemove` is called. If the `FlameGame` is added to a new `GameWidget`
+the sequence repeats from `onGameResize`.
 
 ```{note}
 The order of `onGameResize` and `onLoad` are reversed from that of other

packages/flame/benchmark/common.dart

@@ -13,5 +13,6 @@ Future<void> mountGame(FlameGame game, {Vector2? size}) async {
   await game.load();
   // ignore: invalid_use_of_internal_member
   game.mount();
+  await game.ready();
   game.update(0);
 }

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;
 
@@ -220,6 +221,7 @@ class Component {
   void _clearRemovedBit() => _state &= ~_removed;
 
   Completer<void>? _loadCompleter;
+  Completer<void>? _loadSettledCompleter;
   Completer<void>? _mountCompleter;
   Completer<void>? _removeCompleter;
 
@@ -231,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.
@@ -244,6 +251,22 @@ class Component {
         : (_loadCompleter ??= Completer<void>()).future;
   }
 
+  /// A future that completes once the [onLoad] step has settled, regardless
+  /// of whether it succeeded or failed.
+  ///
+  /// Unlike [loaded], this future never completes with an error; a load
+  /// failure is still reported through [loaded], or through the current
+  /// [Zone] if nothing is awaiting [loaded]. This is used by
+  /// [FlameGame.ready] to wait for loading components without interfering
+  /// with how their load errors are reported.
+  @internal
+  Future<void> get loadSettled {
+    if (isLoaded || _loadError != null) {
+      return Future.value();
+    }
+    return (_loadSettledCompleter ??= Completer<void>()).future;
+  }
+
   /// A future that will complete once the component is mounted on its parent.
   ///
   /// If the component is already mounted (see [isMounted]), this returns an
@@ -797,6 +820,9 @@ class Component {
     } else {
       _children?.remove(child);
       child._parent = null;
+      if (isLoading) {
+        _notifyChildrenChangedWhileLoading();
+      }
     }
   }
 
@@ -1056,11 +1082,88 @@ 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();
     _loadCompleter = null;
+    _completeLoadSettled();
+  }
+
+  void _completeLoadSettled() {
+    _loadSettledCompleter?.complete();
+    _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].
@@ -1085,6 +1188,7 @@ class Component {
     } else {
       Zone.current.handleUncaughtError(error, stackTrace);
     }
+    _completeLoadSettled();
   }
 
   /// Mount the component that is already loaded and has a mounted parent.
@@ -1159,11 +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() {
-    _setLoadedBit();
-    _loadCompleter?.complete();
-    _loadCompleter = null;
-  }
+  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/lib/src/components/core/component_tree_root.dart

@@ -26,28 +26,51 @@ class ComponentTreeRoot extends Component {
   final Set<Component> _blocked;
   late final Map<ComponentKey, Component> _index = {};
   Completer<void>? _lifecycleEventsCompleter;
+  Completer<void>? _lifecycleEventMutationCompleter;
+
+  /// A future that completes the next time the lifecycle event queue is
+  /// mutated: when a new event is enqueued or an existing event is cancelled.
+  ///
+  /// This is used by `FlameGame.ready` to re-evaluate the queue when it is
+  /// changed by something other than a component finishing its load, for
+  /// example when a component is removed while it is still loading.
+  @internal
+  Future<void> get nextLifecycleEventMutation =>
+      (_lifecycleEventMutationCompleter ??= Completer<void>()).future;
+
+  void _notifyLifecycleEventMutation() {
+    _lifecycleEventMutationCompleter?.complete();
+    _lifecycleEventMutationCompleter = null;
+  }
 
   @internal
   void enqueueAdd(Component child, Component parent) {
     queue.addLast()
       ..kind = LifecycleEventKind.add
       ..child = child
       ..parent = parent;
+    _notifyLifecycleEventMutation();
   }
 
   @internal
   void dequeueAdd(Component child, Component parent) {
-    for (final event in queue) {
-      if (event.kind == LifecycleEventKind.add &&
+    // This uses [RecycledQueue.firstWhereOrNull] instead of iterating over
+    // the queue, since it can be called from user code that runs while
+    // [processLifecycleEvents] is iterating over the queue, and the queue
+    // only supports one iteration at a time.
+    final event = queue.firstWhereOrNull(
+      (event) =>
+          event.kind == LifecycleEventKind.add &&
           event.child == child &&
-          event.parent == parent) {
-        event.kind = LifecycleEventKind.unknown;
-        return;
-      }
-    }
-    throw AssertionError(
-      'Cannot find a lifecycle event Add(child=$child, parent=$parent)',
+          event.parent == parent,
     );
+    if (event == null) {
+      throw AssertionError(
+        'Cannot find a lifecycle event Add(child=$child, parent=$parent)',
+      );
+    }
+    event.kind = LifecycleEventKind.unknown;
+    _notifyLifecycleEventMutation();
   }
 
   @internal
@@ -56,14 +79,23 @@ class ComponentTreeRoot extends Component {
       ..kind = LifecycleEventKind.remove
       ..child = child
       ..parent = parent;
+    _notifyLifecycleEventMutation();
   }
 
   @internal
   void dequeueRemove(Component child) {
-    for (final event in queue) {
-      if (event.kind == LifecycleEventKind.remove && event.child == child) {
+    // See [dequeueAdd] for why this doesn't iterate over the queue directly.
+    var dequeuedAny = false;
+    queue.forEachWhere(
+      (event) =>
+          event.kind == LifecycleEventKind.remove && event.child == child,
+      (event) {
         event.kind = LifecycleEventKind.unknown;
-      }
+        dequeuedAny = true;
+      },
+    );
+    if (dequeuedAny) {
+      _notifyLifecycleEventMutation();
     }
   }
 
@@ -86,6 +118,9 @@ class ComponentTreeRoot extends Component {
         event.kind = LifecycleEventKind.unknown;
       },
     );
+    if (result.isNotEmpty) {
+      _notifyLifecycleEventMutation();
+    }
   }
 
   @internal
@@ -94,6 +129,7 @@ class ComponentTreeRoot extends Component {
       ..kind = LifecycleEventKind.move
       ..child = child
       ..parent = newParent;
+    _notifyLifecycleEventMutation();
   }
 
   @internal
@@ -105,6 +141,7 @@ class ComponentTreeRoot extends Component {
       ..kind = LifecycleEventKind.rebalance
       ..child = child
       ..parent = parent;
+    _notifyLifecycleEventMutation();
   }
 
   bool get hasLifecycleEvents => queue.isNotEmpty;
@@ -142,7 +179,30 @@ class ComponentTreeRoot extends Component {
         : (_lifecycleEventsCompleter ??= Completer<void>()).future;
   }
 
+  /// Whether [processLifecycleEvents] is currently running.
+  ///
+  /// Used by `FlameGame.ready` to defer its own queue processing when it is
+  /// called from inside a lifecycle callback, since the queue only supports
+  /// one iteration at a time.
+  @internal
+  bool get isProcessingLifecycleEvents => _processingLifecycleEvents;
+  bool _processingLifecycleEvents = false;
+
   void processLifecycleEvents() {
+    assert(
+      !_processingLifecycleEvents,
+      'processLifecycleEvents cannot be called while it is already running, '
+      'for example from inside a lifecycle callback',
+    );
+    _processingLifecycleEvents = true;
+    try {
+      _processLifecycleEvents();
+    } finally {
+      _processingLifecycleEvents = false;
+    }
+  }
+
+  void _processLifecycleEvents() {
     // reorder events to process later grouped by parent
     final reorderParents = <Component>{};
     LifecycleEventStatus handleReorderEvent(Component parent) {

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

@@ -209,6 +209,28 @@ class RecycledQueue<T extends Disposable> extends Iterable<T>
     }
   }
 
+  /// Returns the first element matching [test], or null if there is none,
+  /// by directly traversing the internal storage. Unlike iteration, this can
+  /// be safely called while another iteration is in progress.
+  T? firstWhereOrNull(bool Function(T) test) {
+    if (isEmpty) {
+      return null;
+    }
+    var i = _startIndex;
+    while (true) {
+      if (!_indicesToRemove.contains(i) && test(_elements[i])) {
+        return _elements[i];
+      }
+      if (i == _endIndex) {
+        return null;
+      }
+      i += 1;
+      if (i == _elements.length) {
+        i = 0;
+      }
+    }
+  }
+
   @override
   Iterator<T> get iterator {
     _garbageCollect();