refactor: Component rebalancing is now performed via a global queue (#2352)

This PR ensures that all component rebalancing operations are resolved from a single location, after the update stage but before the render stage (thus, components may get reordered during the update, and these changes will go into effect during the rendering step on the same game tick).

This also fixes the problem where the child changing the priorities of its parent would cause a ConcurrentModificationError.

A number of methods that were used to handle rebalancing are now marked as deprecated. From the user's perspective, the only API they should be using is the .priority setter.

doc/flame/components.md

@@ -96,9 +96,10 @@ class MyGame extends FlameGame {
 ```
 
 To update the priority of a component you have to set it to a new value, like
-`component.priority = 2`, and it will be updated in the next tick.
+`component.priority = 2`, and it will be updated in the current tick before the rendering stage.
 
-Example:
+In the following example we first initialize the component with priority 1, and then when the
+user taps the component we change its priority to 2:
 
 ```dart
 class MyComponent extends PositionComponent with Tappable {
@@ -112,9 +113,6 @@ class MyComponent extends PositionComponent with Tappable {
 }
 ```
 
-In the example above we first initialize the component with priority 1, and then when the user taps
-the component we change the priority to 2.
-
 
 ### Composability of components
 

examples/lib/stories/components/priority_example.dart

@@ -33,7 +33,7 @@ class Square extends RectangleComponent
   bool onTapDown(TapDownInfo info) {
     final topComponent = gameRef.children.last;
     if (topComponent != this) {
-      gameRef.children.changePriority(this, topComponent.priority + 1);
+      priority = topComponent.priority + 1;
     }
     return false;
   }

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

@@ -489,7 +489,6 @@ class Component {
   /// children according to their [priority] order, relative to the
   /// priority of the direct siblings, not the children or the ancestors.
   void updateTree(double dt) {
-    _children?.updateComponentList();
     update(dt);
     _children?.forEach((c) => c.updateTree(dt));
   }
@@ -728,22 +727,26 @@ class Component {
   int get priority => _priority;
   int _priority;
   set priority(int newPriority) {
-    if (parent == null) {
+    if (_priority != newPriority) {
       _priority = newPriority;
-    } else {
-      parent!.children.changePriority(this, newPriority);
+      final game = findGame();
+      if (game != null && _parent != null) {
+        (game as FlameGame).enqueueRebalance(_parent!);
+      }
     }
   }
 
   /// Usually this is not something that the user would want to call since the
   /// component list isn't re-ordered when it is called.
   /// See FlameGame.changePriority instead.
+  @Deprecated('Will be removed in 1.8.0. Use priority setter instead.')
   void changePriorityWithoutResorting(int priority) => _priority = priority;
 
   /// Call this if any of this component's children priorities have changed
   /// at runtime.
   ///
   /// This will call [ComponentSet.rebalanceAll] on the [children] ordered set.
+  @Deprecated('Will be removed in 1.8.0.')
   void reorderChildren() => _children?.rebalanceAll();
 
   //#endregion

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

@@ -22,12 +22,6 @@ class ComponentSet extends QueryableOrderedSet<Component> {
           strictMode: strictMode ?? defaultStrictMode,
         );
 
-  /// Components whose priority changed since the last update.
-  ///
-  /// When priorities change we need to re-balance the component set, but
-  /// we can only do that after each update to avoid concurrency issues.
-  final Set<Component> _changedPriorities = {};
-
   static bool defaultStrictMode = false;
 
   /// Marked as internal, because the users shouldn't be able to add elements
@@ -73,23 +67,30 @@ class ComponentSet extends QueryableOrderedSet<Component> {
   @override
   bool get isNotEmpty => !isEmpty;
 
-  /// Call this on your update method.
-  ///
-  /// This method effectuates any pending operations of insertion or removal,
-  /// and thus actually modifies the components set.
-  /// Note: do not call this while iterating the set.
-  void updateComponentList() {
-    _actuallyUpdatePriorities();
-  }
-
-  @override
-  void rebalanceAll() {
+  /// Sorts the components according to their `priority`s. This method is
+  /// invoked by the framework when it knows that the priorities of the
+  /// components in this set has changed.
+  @internal
+  void reorder() {
     final elements = toList();
     // bypass the wrapper because the components are already added
     super.clear();
     elements.forEach(super.add);
   }
 
+  /// Call this on your update method.
+  ///
+  /// This method effectuates any pending operations of insertion or removal,
+  /// and thus actually modifies the components set.
+  /// Note: do not call this while iterating the set.
+  @Deprecated('Will be removed in 1.8.0.')
+  void updateComponentList() {}
+
+  @Deprecated('Will be removed in 1.8.0.')
+  @override
+  void rebalanceAll() => reorder();
+
+  @Deprecated('Will be removed in 1.8.0.')
   @override
   void rebalanceWhere(bool Function(Component element) test) {
     // bypass the wrapper because the components are already added
@@ -104,33 +105,12 @@ class ComponentSet extends QueryableOrderedSet<Component> {
   /// either was a child of another component, if it didn't exist at all or if
   /// it was a component added directly on the game but its priority didn't
   /// change.
+  @Deprecated('Will be removed in 1.8.0.')
   bool changePriority(
     Component component,
     int priority,
   ) {
-    if (component.priority == priority) {
-      return false;
-    }
-    component.changePriorityWithoutResorting(priority);
-    _changedPriorities.add(component);
+    component.priority = priority;
     return true;
   }
-
-  void _actuallyUpdatePriorities() {
-    var hasRootComponents = false;
-    final parents = <Component>{};
-    _changedPriorities.forEach((component) {
-      final parent = component.parent;
-      if (parent != null) {
-        parents.add(parent);
-      } else {
-        hasRootComponents |= contains(component);
-      }
-    });
-    if (hasRootComponents) {
-      rebalanceAll();
-    }
-    parents.forEach((parent) => parent.reorderChildren());
-    _changedPriorities.clear();
-  }
 }

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

@@ -12,10 +12,12 @@ import 'package:vector_math/vector_math_64.dart';
 class ComponentTreeRoot extends Component {
   ComponentTreeRoot({super.children})
       : _queue = RecycledQueue(_LifecycleEvent.new),
-        _blocked = <int>{};
+        _blocked = <int>{},
+        _componentsToRebalance = <Component>{};
 
   final RecycledQueue<_LifecycleEvent> _queue;
   final Set<int> _blocked;
+  final Set<Component> _componentsToRebalance;
 
   @internal
   void enqueueAdd(Component child, Component parent) {
@@ -56,6 +58,11 @@ class ComponentTreeRoot extends Component {
       ..parent = newParent;
   }
 
+  @internal
+  void enqueueRebalance(Component parent) {
+    _componentsToRebalance.add(parent);
+  }
+
   bool get hasLifecycleEvents => _queue.isNotEmpty;
 
   void processLifecycleEvents() {
@@ -101,6 +108,13 @@ class ComponentTreeRoot extends Component {
     }
   }
 
+  void processRebalanceEvents() {
+    for (final component in _componentsToRebalance) {
+      component.children.reorder();
+    }
+    _componentsToRebalance.clear();
+  }
+
   @mustCallSuper
   @override
   @internal

packages/flame/lib/src/components/router_component.dart

@@ -273,9 +273,8 @@ class RouterComponent extends Component {
 
   void _adjustRoutesOrder() {
     for (var i = 0; i < _routeStack.length; i++) {
-      _routeStack[i].changePriorityWithoutResorting(i);
+      _routeStack[i].priority = i;
     }
-    reorderChildren();
   }
 
   void _adjustRoutesVisibility() {

packages/flame/lib/src/game/flame_game.dart

@@ -85,11 +85,11 @@ class FlameGame extends ComponentTreeRoot with Game {
   @override
   void updateTree(double dt) {
     processLifecycleEvents();
-    children.updateComponentList();
     if (parent != null) {
       update(dt);
     }
     children.forEach((c) => c.updateTree(dt));
+    processRebalanceEvents();
   }
 
   /// This passes the new size along to every component in the tree via their

packages/flame/test/components/priority_test.dart

@@ -1,27 +1,11 @@
+import 'dart:ui';
+
 import 'package:flame/components.dart';
 import 'package:flame_test/flame_test.dart';
 import 'package:test/test.dart';
 
-class _PriorityComponent extends Component {
-  _PriorityComponent(int priority) : super(priority: priority);
-}
-
-class _ParentWithReorderSpy extends Component {
-  int callCount = 0;
-
-  _ParentWithReorderSpy(int priority) : super(priority: priority);
-
-  @override
-  void reorderChildren() {
-    callCount++;
-    super.reorderChildren();
-  }
-
-  void assertCalled(int n) {
-    expect(callCount, n);
-    callCount = 0;
-  }
-}
+import '../custom_component.dart';
+// ignore_for_file: deprecated_member_use_from_same_package
 
 void main() {
   void componentsSorted(Iterable<Component> components) {
@@ -222,5 +206,68 @@ void main() {
         componentsSorted(c.children);
       },
     );
+
+    testWithFlameGame('child can update priority of its parent', (game) async {
+      final renderEvents = <String>[];
+
+      final parent = CustomComponent(
+        priority: 0,
+        onRender: (self, canvas) {
+          renderEvents.add('render:parent');
+        },
+      );
+      final child = CustomComponent(
+        onUpdate: (self, dt) {
+          self.parent!.priority = 10;
+        },
+      );
+      parent.add(child);
+      game.add(parent);
+      game.add(
+        CustomComponent(
+          priority: 1,
+          onRender: (self, canvas) {
+            renderEvents.add('render:another');
+          },
+        ),
+      );
+      await game.ready();
+
+      expect(parent.priority, 0);
+      expect(child.priority, 0);
+      game.update(0.1);
+      expect(parent.priority, 10);
+      expect(child.priority, 0);
+      expect(renderEvents, isEmpty);
+      game.render(Canvas(PictureRecorder()));
+      expect(renderEvents, ['render:another', 'render:parent']);
+    });
   });
 }
+
+class _SpyComponentSet extends ComponentSet {
+  int callCount = 0;
+
+  @override
+  void reorder() {
+    callCount++;
+    super.reorder();
+  }
+}
+
+class _PriorityComponent extends Component {
+  _PriorityComponent(int priority) : super(priority: priority);
+}
+
+class _ParentWithReorderSpy extends Component {
+  _ParentWithReorderSpy(int priority) : super(priority: priority);
+
+  @override
+  ComponentSet createComponentSet() => _SpyComponentSet();
+
+  void assertCalled(int n) {
+    final componentSet = children as _SpyComponentSet;
+    expect(componentSet.callCount, n);
+    componentSet.callCount = 0;
+  }
+}

packages/flame/test/custom_component.dart

@@ -1,4 +1,5 @@
 import 'dart:async';
+import 'dart:ui';
 
 import 'package:flame/components.dart';
 
@@ -10,15 +11,23 @@ class CustomComponent extends Component {
     FutureOr<void> Function(CustomComponent)? onLoad,
     void Function(CustomComponent)? onMount,
     void Function(CustomComponent)? onRemove,
+    void Function(CustomComponent, double)? onUpdate,
+    void Function(CustomComponent, Canvas)? onRender,
+    super.priority,
+    super.children,
   })  : _onGameResize = onGameResize,
         _onLoad = onLoad,
         _onMount = onMount,
-        _onRemove = onRemove;
+        _onRemove = onRemove,
+        _onUpdate = onUpdate,
+        _onRender = onRender;
 
   final void Function(CustomComponent, Vector2)? _onGameResize;
   final FutureOr<void> Function(CustomComponent)? _onLoad;
   final void Function(CustomComponent)? _onMount;
   final void Function(CustomComponent)? _onRemove;
+  final void Function(CustomComponent, double)? _onUpdate;
+  final void Function(CustomComponent, Canvas)? _onRender;
 
   @override
   void onGameResize(Vector2 size) {
@@ -34,4 +43,10 @@ class CustomComponent extends Component {
 
   @override
   void onRemove() => _onRemove?.call(this);
+
+  @override
+  void update(double dt) => _onUpdate?.call(this, dt);
+
+  @override
+  void render(Canvas canvas) => _onRender?.call(this, canvas);
 }