Allow creating dependencies during load

[ekrctb]

Currently, we cannot create dependencies with access to parent dependencies. Two usage examples:

1. Override dependency with access to "base":

[Resolved] T baseProperty;
[Cached] T property => func(baseProperty);

This corresponds to override T Property => func(base.Property); of class inheritance.

2. Cache resource:

[Cached] Resource resource;
[BackgroundDependencyLoader] void load(ResourceStore store) => resource = store.Get("resource");

Current order of loading:

1. Parent creates dependencies.
2. Child creates dependencies.
3. Child resolves dependencies and load Child.

Proposed order:

1. Parent creates dependencies.
2. Child resolves dependencies and load Child.
3. Child creates dependencies.

[peppy]

Just as an aside, you can currently get around this by making your dependent dependencies in CreateChildDependencies (Get from parent and cache directly inside this method.

Obviously a bit convoluted as a solution, but works.

[Flutterish]

Hey, I would like to bump this and possibly fix it myself if possible. I recently stumbled onto this problem when I wanted to create a material manager for a 3D scene in the load method, similarly to how Game does it, however the children would not resolve it. I looked into it and as it turns out, something is wrong with the load order of [BackgroundDependencyLoader]. I ran a test to see how components are loaded and this is the result: BDL loads children before the parent, which makes no sense, not even considering that the parent should be able to create dependencies in the BDL method that are then propagated to children. You can technically work around this like peppy said, or by defering adding children to the next frame, which is terrible.

The test: https://drive.google.com/file/d/1vfE-kLrLNNOkGJA68daamxVffHlCWMxA/view?usp=sharing

[peppy]

What is the perceived issue with using CreateChildDependencies? We use this in many places without issue.

[bdach]

also not sure what is the issue that the parent's BDL runs after the children's. the children load happens first because it's in CompositeDrawable's BDL. which is consistent with everything else, because it's the base class of your LoadTesterBase. it's like a constructor, and will ideally eventually become the constructor.

[Flutterish]

also not sure what is the issue that the parent's BDL runs before the children

The childrens' BDL runs first, not the parent's! That is the issue.

[bdach]

yes, i mis-spoke.

you may be looking to use LoadAsyncComplete. i think it will give you the ordering you want.

[Flutterish]

I can resolve my issue by adding the DI at the CreateChildDependencies, however I don't think it makes sense for any DI to happen in children before it happens in the parent. After all, when we use [Cached] and [Resolved], all parent DI is cached and resolved before the childrens', I think it would only make sense for all DI steps to follow this order.

If you say that it's supposed to be a "contructor", then the parents ctor is run before the child ctor (because we only add the child in the ctor). We are talking about the drawable hierarchy, not derived classes, becuase in that case of course the base ctor is run before the derived ctor.

[bdach]

i have been through this myself already.

read https://github.com/ppy/osu-framework/issues/4442#issuecomment-833309878 for an argument why the order you propose is incompatible with being able to override dependencies in base classes, which many consumers (including the game) already rely on.

[Flutterish]

I feel like you are talking about Base-Derived order while I am talking about Parent-Child order, no? I might be not reading into it enough, but I don't see any mention about a child drawable, only derived classes.

[bdach]

as i mentioned before, the child load happens in CompositeDrawable's BDL.

https://github.com/ppy/osu-framework/blob/a8429246cd22b686db420a89c049a57792f37923/osu.Framework/Graphics/Containers/CompositeDrawable.cs#L234-L249

so the two issues boil down to being essentially the same.

[Flutterish]

Ah, I see. I thought the BDL happened somewhere where it could be ordered, rather than in another BDL. I understand how this might break things then. Thank you for the explaination, I don't think I will be able to come up with a sane idea to amend this, but I will think about it. I think the only way to have the order be the same as other loads for the time being is in the CreateChildDependencies or other load methods like peppy said.

[peppy]

I'm honestly not sure if this issue should stay open. @ekrctb have your thoughts on this changed since 2019?

[ekrctb]

I haven't fully prepared for arguing for the proposal, but I still think my proposed order is more natural. I think this issue should be focused on parent/child relations and not inheritance relations.

However, another question is whether there is a way forward to resolve this issue. This is still an open question I think.

[Flutterish]

The issue and solution I see here is that the loading of children happen in a BDL, however at least to me the BDL feels like it should be a pure DI step that performs what Cached and Resolved couldnt. In this hypothetical world, it should not do anything about its children or the hierarchy, at all, it should rather be deferred to say, LoadComplete, which has a controllable order since you can choose when to call base (if at all, which might pose an issue I guess). I dont fully understand why the CompositeDrawable BDL has to load the children, rather than load them like all other children added after that step, but thats up to you to resolve.

[frenzibyte]

BDL (aka BackgroundDependencyLoader) is more of a stage for the drawable to begin loading itself, I believe it is rather optimal for the drawable to load and add its children at that point.

Keep in mind BDL may run in an asynchronous thread if the drawable was loaded via LoadComponentAsync for example, which gives all the more reason why a drawable should load its children at that point.

And at this point, it may not even make sense to call it BackgroundDependencyLoader anymore given it's not for DI purposes only, but that's a completely separate topic that I don't think matters at all right now.

---

That aside, it is now more favourable to add children in BDL rather than in constructor anyways, dependencies can be cached during load before children are added in that case, no?