feat: Components are now always added in the correct order

[st-pasha]

Description

• Components may be added into game tree out of order #1222: Main change is that the system now ensures that when components are added into the game tree, they will be always added in the order that the user requested. For example, if the user says game..add(c1)..add(c2), then components c1 and c2 will appear in that order in game's children list, regardless of how long it took for them to load.

• New mixin SingleGameInstance allows onLoad() to be always called when the component is added to a parent, even if the parent is not mounted yet.

• Improving Flame testing by allowing users to wait for game to load #1335: Added game.ready() function, which can be awaited if you need to resolve all pending changes to the game tree (mostly useful for testing).

• Remove prepareComponent #1342: Lifecycle methods prepare and prepareComponent were removed. Existing logic in prepare can be merged with onMount, and functionality of prepareComponent can always be replicated within the Component itself.

• Some of the functionality from ComponentSet was moved out into the _LifecycleManager class. This has several advantages: first, the Component is no longer forced to carry temporary lifecycle queues forever, which reduces overall memory consumption; second, implementing alternative ComponentSets now becomes easier, as it is no longer required to cater to the Components lifecycle peculiarities.

• From testing standpoint, the main difference is that now at least some game instance is required in each test which involves more than one component. The idiom await game.ready() can now be used to ensure that all components were fully mounted. In most cases it could replace the game.update(0) call, which is less reliable.

Checklist

• The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
• I have read the Contributor Guide and followed the process outlined for submitting PRs.
• I have updated/added tests for ALL new/updated/fixed functionality.
• I have updated/added relevant documentation in docs and added dartdoc comments with ///.
• (NA) I have updated/added relevant examples in examples.

Breaking Change

• No, this is not a breaking change.
  • Except if you were using prepare() / prepareComponent(), which we believe to be extremely rare. But if you did, then the recommendation is to merge that functionality into the onMount callback.

Related Issues

Closes #1222
Closes #1293
Closes #1305
Closes #1335
Closes #1338
Closes #1342

[spydon]

Since we are already having an ordering function in the OrderedSet, can't we make use of that one?
We would only have to keep track of an integer for which order the component was added in and within the same priority layer that order index would decide which order it would be in in the ComponentSet.

So the comparator would be something like this:

Comparing.join<Component>([
  Comparing.on<Component>((c) => c.priority),
  Comparing.on<Component>((c) => c.addIndex),
]);

(addIndex would probably have a different name, but you probably understand the gist at least.)

This could also be made a non-breaking change in terms of API.

Another small thing is that we probably want to keep the Future on add, in almost all cases it will work to await that one, the only case I can come up with is when you add a component to a component that doesn't have a root yet, but since subcomponents mostly are added in either onLoad or as reactions to input that await almost always work. Or do you have another common usecase in mind? If we don't remove it we do need better documentation for the case where it won't work however. Or if we can come up with another way of waiting for specific components to be loaded.

[st-pasha]

Since we are already having an ordering function in the OrderedSet, can't we make use of that one?

Can you please elaborate what benefit do you see for this approach? What it does is that it adds component unordered, but keeps an extra variable addIndex around that helps with putting all components back in order. This PR, on the other hand, makes sure that everything is put in order in the first place.

The approach that you suggest also leads to a significant increase in memory size of the component tree. This is due to the fact of how OrderedSet is implemented: it puts all components with same priorities in a single list, and components with different priorities into different lists. Thus, in the common case when there are n components with the default priority of 0, the OrderedSet will put them into a single list of length n in a tree of size 1. However, with the introduction of addIndex, the priorities will now always be different, which means n lists of size 1 living in a tree of size n.

I'm not even sure how solid the statement "we are already having an ordering function in the OrderedSet" is. I mean, surely, we do, but eventually we want to support different implementations for ComponentSets. For example, for some simple leaf-level components like sprites you might have a simpler ComponentSet based on a list. For a large-size game you may want to have a ComponentSet based on a quadtree or a similar structure. For an isometric game you could have a ComponentSet that automatically sorts the components according to their distance from the camera. And so on.

Another advantage this PR brings is that it reduces the size of the Component: it no longer needs to carry the Set _addLater, instead these queues are handled at the Game level.

Another small thing is that we probably want to keep the Future on add, in almost all cases it will work to await that one

Sure, we can. After all, it's already there: onLoadFuture, and it's just the question of whether we want to return it or not. However, I find that future very confusing. Without consulting documentation, it's easy to assume looking at the function signature Future<void> add(Component c) that when you wait on that future you wait for c to be added. It's not even true that awaiting on that future will guarantee that c is loaded, after all c's loading can be delayed if it cannot be prepared.

So, having a future there that sometimes works and sometimes doesn't, just doesn't sound like a solid API to me. Though I do agree with you that scenarios where that future is meaningful are quite common. Still, they aren't overwhelmingly common.

On the other hand, what if we removed the restriction that the parent needs to be mounted before we can proceed with loading? That would remove the "add" queue altogether and make the future far better defined.

[spydon]

Can you please elaborate what benefit do you see for this approach? What it does is that it adds component unordered, but keeps an extra variable addIndex around that helps with putting all components back in order. This PR, on the other hand, makes sure that everything is put in order in the first place.

It makes the code here a lot simpler, there is no extra cost to add them in their correct place in the OrderedSet even if they aren't added in the add-order.

The approach that you suggest also leads to a significant increase in memory size of the component tree. This is due to the fact of how OrderedSet is implemented: it puts all components with same priorities in a single list, and components with different priorities into different lists. Thus, in the common case when there are n components with the default priority of 0, the OrderedSet will put them into a single list of length n in a tree of size 1. However, with the introduction of addIndex, the priorities will now always be different, which means n lists of size 1 living in a tree of size n.

That's true, that's a good argument.

Another advantage this PR brings is that it reduces the size of the Component: it no longer needs to carry the Set _addLater, instead these queues are handled at the Game level.

It does remove _addLater, but the queue solution comes with quite some complexity that wasn't needed here before.

On the other hand, what if we removed the restriction that the parent needs to be mounted before we can proceed with loading?

Many games depend on the game size and the game caches in their onLoad methods, and we'd need to be mounted to have access to those. But it is an intriguing idea.

That would remove the "add" queue altogether and make the future far better defined.

How could it remove the queue though, since there would still be different loading times between components?

[st-pasha]

Many games depend on the game size and the game caches in their onLoad methods, and we'd need to be mounted to have access to those. But it is an intriguing idea.

What if we handle this at Game's lifecycle? That is, first GameWidget waits for the size to be available (and non-zero), and only then it launches Game.onLoad. This way any component that is added within onLoad is guaranteed to be able to know the size of the canvas. If a component needs to have some additional game resources, like images, it can await those inside the onLoad, exactly as it is done right now.

It does remove _addLater, but the queue solution comes with quite some complexity that wasn't needed here before.

I agree that it does look somewhat more complex, but I wonder if maybe that's just a matter of perception? Because what I did is I gathered code that was spread over prepare and prepareComponent and put it all into add(), then added several asserts -- so now the code looks much bigger, but it's actually doing all the same things. If anything, it's easier to understand now that it's all in one place.

As for the queue, it replaces the _addLater set. So, likewise, there was some code in the ComponentSet that was handling that set, and now that code transformed into the queue-processing code. It's probably a bit more code, but only because we're taking care of removing queues as soon as they empty up, which reduces memory footprint.

So, what I am saying is that the overall complexity remains approximately the same, only now it is concentrated in a single place, which makes it appear like it got more complex.

How could it remove the queue though, since there would still be different loading times between components?

We could remove the "add" queue only, the "children" queue will remain.

[spydon]

Some final small comments, other than those I think this is ready to get merged!

[luanpotter]

pretty message for this assert? or this one that "should never happen AFAWK"

[st-pasha]

This is just a sanity check on the internal state: I do not think it is possible to violate this assert in the current code. But, if in the future we need to refactor this again (say, adding more states), this assert may trigger.

[luanpotter]

we could add some checks like: if staticGameInstance != null && != this then throw pretty error message saying you cannot use SingleGameInstance like that

[st-pasha]

There is an assert like that in FlameGame class:

assert(
      Component.staticGameInstance == null,
      '$this instantiated, while another game ${Component.staticGameInstance} '
      'declares itself to be a singleton',
    );

I think it's better to have it in FlameGame because then not only we protect against two SingleGameInstance objects, but we also prevent a SingleGameInstance to coexist with a regular FlameGame.

[luanpotter]

wow what class requires this? never seem it before

[st-pasha]

This is for logging the stack-trace in line 337. It won't be needed after we upgrade to Dart 2.16 (which is in Flutter 2.10.0, released on 2022-02-03).

[luanpotter]

can we just add dart>=2.16 to the pubspecs? otherwise I would just put out an issue with this code to tackle later, rather than commented code

[st-pasha]

I'd love to! We have issue #1346 so that we won't forget to do that.

[luanpotter]

This is awesome!

[erickzanardo]

One small additional nit, but LGTM!