Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add events that fire before components are activated #671

Open
Barsonax opened this issue Jun 20, 2018 · 14 comments
Open

Add events that fire before components are activated #671

Barsonax opened this issue Jun 20, 2018 · 14 comments
Labels
Core Area: Duality runtime or launcher Feature It doesn't exist yet, but I want it
Milestone

Comments

@Barsonax
Copy link
Member

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

  • Can be used to do quite neat stuff such as IOC
  • Like all things can also be abused.
@ilexp ilexp added this to the General milestone Jul 1, 2018
@ilexp ilexp added Feature It doesn't exist yet, but I want it Core Area: Duality runtime or launcher labels Jul 1, 2018
@ilexp
Copy link
Member

ilexp commented Jul 1, 2018

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
Copy link
Member Author

Barsonax commented Jul 3, 2018

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

@Barsonax
Copy link
Member Author

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

@ilexp
Copy link
Member

ilexp commented Oct 18, 2018

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
Copy link
Member Author

Barsonax commented Oct 29, 2018

Events to add:

  • BeforeComponentActivated like ComponentAdded but triggers before component activation
  • BeforeGameObjectActivated like GameObjectAdded but triggers before component activation
  • BeforeSceneActivated like SceneEntered but triggers after all objects are loaded in the scene but before component activation.

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
Copy link
Member

ilexp commented Oct 29, 2018

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:

  • Entering / Entered
  • GameObjectsAdding / GameObjectsAdded
  • ComponentAdding / ComponentAdded

The grammar is not ideal though. Thoughts?

@Barsonax
Copy link
Member Author

Barsonax commented Oct 29, 2018

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
Copy link
Member

ilexp commented Oct 30, 2018

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
Copy link
Member Author

Barsonax commented Oct 30, 2018

Basically its this:

  • A instance of a certain type is requested
  • Find the appropriate constructor to call, usually this means that the type must only have a single constructor to call.
  • Create instances of every constructor parameter <- this part can be recursive because those types can have their own constructors etc.
  • Use these instances to call the constructor.

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:

  • No need for some custom attribute marking some method as being magical
  • Provides more guarantees compared to method injection. You know 100% sure that the constructor has to be called to create that type for instance.
  • Enables the use of readonly and get only fields and properties
  • In unit tests its clear what to do to create a certain type, just call the constructor.

@ilexp
Copy link
Member

ilexp commented Oct 30, 2018

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
Copy link
Member Author

Barsonax commented Oct 30, 2018

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:

  • How to register dependencies now? Currently I have a component inside the scene that does this but now we could get into the situation that the dependency component has not been created yet but we do need a dependency from that component. Method injection wouldn't have this problem as it happens at a later stage when all components have already been created

@Barsonax
Copy link
Member Author

Barsonax commented Oct 30, 2018

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:

  • I have control over how duality creates a new component
  • I have control over what order components are created when a scene loads (so I can fetch the dependencies, components that implement IModule, first for instance)
  • Even with the above how to handle IModule components that register another component in the scene as dependency? Would have to take this into account as well for the order of creation..

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

@ilexp
Copy link
Member

ilexp commented Nov 3, 2018

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
Copy link
Member Author

Barsonax commented Nov 3, 2018

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:

  • Entering / Entered
  • GameObjectsAdding / GameObjectsAdded
  • ComponentAdding / ComponentAdded

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Area: Duality runtime or launcher Feature It doesn't exist yet, but I want it
Development

No branches or pull requests

2 participants