AdamsLair / duality

a 2D Game Development Framework
https://adamslair.github.io/duality
MIT License
1.41k stars 288 forks source link

Add events that fire before components are activated #671

Open Barsonax opened 6 years ago

Barsonax commented 6 years ago

Summary

In order to integrate Singularity with duality I would like the dependencies to be injected before components are activated. Currently duality does not provide a event for this.

Iam currently using Scene.ComponentAdded and Scene.Entered for this purpose except for the fact they fire after component activation.

Analysis

ilexp commented 6 years ago

Probably better to look into this after v3.0, as a lot of related code is in the process of being refactored right now, see issue #504.

Barsonax commented 6 years ago

Ill release Singularity as a v0 version then with the warning this issue is yet to addressed

Barsonax commented 6 years ago

Any timeframe on this? Maybe I can help implementing this.

ilexp commented 6 years ago

I'm not currently looking into this, but a good first step would be to draft a design to discuss - which events to add, when they're called, how they could be used and where things could go wrong, i.e. what to look out for.

Barsonax commented 6 years ago

Events to add:

These events are like the events we have now except for the fact they fire just BEFORE the activation of the components. These extra events should allow me to do the DI before the components are activated.

Obviously since components are not yet activated at this time one would have to be careful but I currently don't see how this would go wrong except for possible NullReferenceExceptions since components are not yet activated. Nothing a bit of documentation could fix. For clarity maybe we could make a timeline explaining when all the events trigger.

Also I see that the naming is a bit confusing atm. With a event named ComponentAdded you think this triggers when you add the component but it does not. It triggers when the component you add has been activated. A name like AfterComponentActivated would be much better. However this event doesnt trigger at all when the component has already been added to the gameobject before the gameobject is added to the scene. Personally I didn't expect this and only after debugging I noticed this. Renaming these events could fix this but would introduce breaking changes.

ilexp commented 6 years ago

Obviously since components are not yet activated at this time one would have to be careful but I currently don't see how this would go wrong except for possible NullReferenceExceptions since components are not yet activated. Nothing a bit of documentation could fix. For clarity maybe we could make a timeline explaining when all the events trigger.

Makes sense, as long as the docs are clear, this shouldn't be an issue.

A name like AfterComponentActivated would be much better. However this event doesnt trigger at all when the component has already been added to the gameobject before the gameobject is added to the scene. Personally I didn't expect this and only after debugging I noticed this. Renaming these events could fix this but would introduce breaking changes.

These events are intended to inform about operations affecting the current scene graph, not object activation states. When an *Added event is called, it means that a previously unknown object (and its children) is now part of the current scene - the fact that it has been activated during its addition is sort of an implementation detail, and it may not be the only operation that has been performed in order to do so. An *Activated naming scheme does not express this as closely, and it would also be a bit misleading, as none of these events are called when objects are activated or deactivated using the .Active property. They're not intended to be extensions of the usual ICmpInitializable activation events.

With a event named ComponentAdded you think this triggers when you add the component but it does not. It triggers when the component you add has been activated.

It depends on how you look at it. The past tense naming of Added suggests that the event informs about a completed operation, and activation is one part of that add operation. If I was searching for an event more at the start of the add operation - such as before activation - I'd probably look for an Adding or similar event name. You get the same naming scheme in some parts of the .NET Framework as well.

I'd probably go with something like the following names:

The grammar is not ideal though. Thoughts?

Barsonax commented 6 years ago

Ah ok makes sense. Not sure how you could word it better than what you did.

Before we start adding events all over the place let me clarify why I have a need for these events in the first place. Normally with DI constructor injection is preferred. However I do not know if this is possible in duality as duality itself calls these constructors. This is why I went for plan B and wanted to do it through method injection using those events. If you think constructor injection is still possible (maybe I can hook into the component creation logic somehow?) then I would prefer that route ofcourse.

Just FYI so I don't keep thinking inside this event solution box.

EDIT: Found the logic creating the instances in ObjectCreator and in GameObject.AddComponent. Without engine modifications it doesn't seem to be possible to put custom logic in here.

EDIT2: If we decide to change the logic here its not needed to change the current GameObject.AddComponent at all. You can make a new extension method that will be used when the type has a constructor with parameters (only works in C#7.3!):

class Program
{
    static void Main(string[] args)
    {
        var gameObject = new GameObject();
        gameObject.AddComponent<Program>();
        gameObject.AddComponent<ClassWithConstructorArguments>();
    }
}

public class ClassWithConstructorArguments
{
    public ClassWithConstructorArguments(int foo)
    {

    }
}

public static class GameObjectExtensions
{
    public static void AddComponent<T>(this GameObject gameObject) where T : class
    {
        //Will handle any types that don't fit the new() constraint
    }
}

public class GameObject
{
    public void AddComponent<T>() where T : class, new()
    {
        //The fast happy path
    }
}
ilexp commented 6 years ago

Before we start adding events all over the place let me clarify why I have a need for these events in the first place. Normally with DI constructor injection is preferred.

Can you elaborate a bit on how constructor injection normally works? Not sure I have a clear picture on the basic procedure, and how Duality blocks it.

Barsonax commented 6 years ago

Basically its this:

Duality has its own logic for this and without engine modifications I don't see a way to put my own logic in there currently.

What I do in Singularity is to create a expression tree that does all the heavy lifting and compile/cache that. This is why its so fast. This is a similar approach to what currently happens in ObjectCreator but the biggest difference is I inject real instances instead of default dummy instances.

Constructor injection has some benefits over method injection:

ilexp commented 6 years ago

Duality has its own logic for this and without engine modifications I don't see a way to put my own logic in there currently.

What would your own logic do, and when would it need to do it? Right after the objects regular constructor has been called / the instance has been created regularly?

Barsonax commented 6 years ago

Basically it would return a (more complex) delegate that could be used to create a instance of that type. This would then be used by duality to create that type.

In order to do so there are still some details to be figured out:

Barsonax commented 6 years ago

So getting the feeling that while constructor injection might be very nice as a feature it would require some more modifications at the duality side. Its probably also more complex to implement because there are more edge cases that have to be covered.

Basically it could work if:

So this seems to become much more complex than just using some events before activation.

ilexp commented 5 years ago

Hmm, yeah, the constructor injection approach might actually cause some headache in the context of Duality. It gets worse when we consider potential future optimizations - for example, it could at some point make sense to internally allocate a large amount of component instances continuously in memory and then use them as needed. Or maybe not, but the point is, there may be good reason to keep Duality in charge of its main allocations.

The good thing about an init event approach for a DI plugin would be that you get more context and state assumptions to work with, for example the entire scene already existing.

Barsonax commented 5 years ago

If DI turns out to work really well in duality we could replace the current ICmpInitializable interface to provide DI directly in the OnInit method. Though this is a major breaking change and also means you cannot use a interface like ICmpInitializable anymore :P.

So for now the safest and most simple option while still providing alot of functionality seems to be adding the following events to duality:

Sounds reasonably simple to do. Mostly a matter of finding where to invoke those events in the code. Afterwards some documentation about those 6 events might be a good idea.