duality icon indicating copy to clipboard operation
duality copied to clipboard

Auto-Establish Component References in the Same GameObject

Open ChristianGreiner opened this issue 9 years ago • 11 comments

Summary

With a special (optional) RequiredComponent-Attribute for fields you needn't assign the required component in the OnInitialize-Method manually.

Example:

[RequiredComponent]
private Spaceship ship;

public void OnUpdate()
{
    ship.DoStuff();
}

Analysis

  • Type less code
  • Do we need an new RequiredComponentAttribute-Class just for fields?

ChristianGreiner avatar Aug 14 '16 10:08 ChristianGreiner

I was thinking of trying to tackle this issue, and I have a couple of doubts regarding the possible implementation:

How should the "required class" be instanced?

  • default no-parameter constructor (easiest to do, might be useless if the object needs some specific initialization code based on the GameObject/Component it was instanced in).
  • constructor with a GameObject parameter (probably the most complete one, allows access to all GameObject Components for initializazion values).
  • constructor with a Component parameter (prone to errors, need explicit casting and won't offer any significant advantage over the GameObject variant).

When should the "required class" be instanced?

  • maybe the OnInit phase is not the best choice, as it is called multiple times.
  • unless the [RequiredComponent] attribute also includes an InitContext variable to identify which context is the correct one to instantiate the object
  • first OnUpdate phase? it means introducing an if(firstPhase) condition in all Components?
  • after the constructor? the GameObject could not be fully initialized

What should be the behavior if the object is already instanced?

  • overwrite
  • leave the current one

Not to bash the idea, just a few concerns :)

SirePi avatar Oct 03 '16 08:10 SirePi

I agree with your concerns regarding instantiating a new / required Component, which is why I'd like to propose something different:

The AutoRef Attribute

(Name is subject to change - suggestions appreciated)

Looking at this from the user side, what this attribute does is sparing you from writing the specific boilerplate code of establishing references to local (same GameObject) Components. It doesn't instantiate anything, it just retrieves the reference and stores it in the field, so you won't have to.

Example

public class Foo : Component
{
    [AutoRef(typeof(Bar))]
    private Bar otherComponent;
}

which is equivalent to:

public class Foo : Component, ICmpInitializable
{
    private Bar otherComponent;

    void ICmpInitializable.OnInit(InitContext context)
    {
        if (context == InitContext.Activate)
        {
            this.otherComponent = this.GameObj.GetComponent<Bar>();
        }
    }
    void ICmpInitializable.OnShutdown(ShutdownContext context) {}
}

and could be written even shorter, if the retrieved type matches the field type:

public class Foo : Component
{
    [AutoRef] private Bar otherComponent;
}

Behavior

  • An AutoRef(typeof(T)) field should behave the same as if invoking GetComponent<T>() and storing the result.
  • T is allowed to be an abstract base class or interface.
  • The assignment / lookup will be executed in the OnInit phase using execution context Activate.
  • It will happen behind the scenes and will be executed before actually invoking OnInit user code.
  • If there are multiple local Components that are assignable to the specified type, the field value will use the first one, just like GetComponent<T>() would do.
  • If there is no local Component that is assignable to the specified type, the field value will be null without producing an error. This should allow AutoRef-ing optional Components that may or may not be there.

Implementation Notes

  • This feature has the potential of a severe performance impact, because there is a non-trivial block of code to be executed for every Component on every OnInit(Activate) call. It will be vital to implement this in the most efficient way possible, and should be measured to evaluate its impact. How long does it take overall with, say ~10000 Components? What is the per-Component impact when using the feature vs. not using it?
  • Naive implementation impact per Component:
    • GetType call
    • GetFields call
    • Iterate over all fields
    • For each field, retrieve cached attributes
      • Dictionary lookup
      • Filter-iterate over list of attributes
      • GetComponent call
      • Assignment call
  • A potential improvement would be to introduce a per-Type metadata storage for Component types.
    • There already is one for RequiredComponent attribute data, could be generalized to house both this existing info, plus the new AutoRef data.
    • The type metadata would be initialized once and only be retrieved later on.
    • AutoRef info to store: An array of { FieldInfo, Type } pairs that represents all AutoRef attributes.
  • Metadata implementation impact per Component:
    • GetType call
    • Dictionary lookup for metadata
    • Iterate over AutoRef fields
    • GetComponent call
    • Assignment call
  • The performance of the optimized case will still be worse than implementing the boilerplate code manually, but it remains to be evaluated whether the impact is significant.
  • A knock-out criterion for this feature would be when the overhead would be significant even when not using it. Same when the overhead would be too bad when using it.
  • It could potentially be addressed by not executing the code in the Activate context of OnInit, but whenever adding or removing Components from a GameObject, and serializing the reference. This would reduce the runtime overhead to zero in most cases, making it even more efficient than manual OnInit retrieval.
    • This option needs some concept work and evaluation, look out for corner cases where the results would be unexpected.
    • Serialization behavior? Especially when there are missing type errors during editing, or similar.
    • Cloning behavior? Will references be re-established?

More Ideas / Questions

Should there be functionality to AutoRef a list of Components, for example "Put all Components implementing XY into that list"?

No. This is getting too complex. The nice thing about an AutoRef feature is its simplicity and single concern. It isn't intended to be that clever, or do specialized implementation work.

Should there be functionality to AutoRef Components in child GameObjects?

No. AutoRef exists in the context of the GameObject as a single composed instance, not in the context of inter-Component communication in general.

Should there be functionality to AutoRef child GameObjects?

No, same reason as above.

What happens when the user re-assigns the reference manually?

The user shouldn't be able to assign this, but there is no way to prevent it. This might be a bad, as it has the potential to spawn problems for new users who might accidentally do stuff they shouldn't.

It also brings up the question when to use AutoRef and when to use GetComponent. As long as there is not a clear answer to this question, this feature shouldn't be implemented.


Feedback appreciated. Thoughts?

ilexp avatar Oct 03 '16 10:10 ilexp

Personally, I would only want this feature if it worked in the last way you mentioned (runs when adding or removing a component). Even the metadata version seems like a lot of code to be ran at runtime behind the scenes with no obvious indication to users of its impact. I could see people assuming it would have far less overhead - expecting Duality to work more magic behind the scenes. Seems risky especially with this feature being so convenient that users could end up using it very often.

Couldn't the procedure you mentioned for the metadata implementation be simplified? If the RequiredComponent attribute already necessitates a similar process, it seems like you could re-use the GetType and maybe the Dictionary lookup if the data for the RequiredComponent and AutoRef attributes were stored together. If the AutoRef is allowed to be null this does have the design flaw of mixing hard requirements and optional setup. Maybe name the attribute RequiredRef rather than RequiredComponent? Or changes the semantics of the implementation to be "component initialization steps" rather "component requirements". Anyways, if the additional GetType and Dictionary lookup can be shared with RequiredComponent, the additional overhead of this attribute would start to get quite close to the manual implementation and less of an issue. This would make this issue a much more substantial change since it'd be touching the RequiredComponent stuff.

Problem with the RequiredRef idea: what would be the meaning/expectation if RequiredRef was used without RequiredComponent? We can't prevent this at build time and so it would have to be allowed.

As long as AutoRef is just syntactic sugar for GetComponent inside OnInit I can't think of a compelling reason to use it over GetComponent. And if it was syntactic sugar, I can see many people preferring the manual way just to be explicit about their initialization steps. The AutoRef attribute would be moving the code relevant to initialization to multiple places.

Another less appealing but far easier to implement implementation would be to have the attribute be an editor only feature - when a component is added to an object in the editor that has the AutoRef property, it automatically fetches a component of the requested type and serializes the reference. At runtime the AutoRef attribute would do nothing.

deanljohnson avatar Jan 16 '18 07:01 deanljohnson

Couldn't the procedure you mentioned for the metadata implementation be simplified? If the RequiredComponent attribute already necessitates a similar process, it seems like you could re-use the GetType and maybe the Dictionary lookup if the data for the RequiredComponent and AutoRef attributes were stored together.

Yes, that would be an option, though as you mentioned not the cleanest one. Since requirements and autorefs might be evaluated in completely different situations, it might not avoid any GetType or dictionary lookup operations either.

Another less appealing but far easier to implement implementation would be to have the attribute be an editor only feature - when a component is added to an object in the editor that has the AutoRef property, it automatically fetches a component of the requested type and serializes the reference. At runtime the AutoRef attribute would do nothing.

Interesting option, but could be hard to maintain. I'm sensing a longer term "find all the loopholes to plug" endeavor there.

Even the metadata version seems like a lot of code to be ran at runtime behind the scenes with no obvious indication to users of its impact. I could see people assuming it would have far less overhead - expecting Duality to work more magic behind the scenes.

Yeah, that's a good argument for not implementing this feature, and I kind of agree. Besides potential perf implications, it would remove a bit of clarity too. Doing it manually also means people know how exactly their components are initialized, can debug and step through it, reason about it and alternatives.

Not sure where to go with this issue overall. For now, I'll put a discussion tag on it, since it's not without downsides.

ilexp avatar Jan 16 '18 16:01 ilexp

I don't really think this is possible in an efficient manner without generating code (which isnt possible in the way we need it here in C#). I do like the idea and I think its simple enough for ppl to understand it.

Rick-van-Dam avatar Mar 29 '18 14:03 Rick-van-Dam

What about restricting this to 'global' depedencies like game managers, cameras etc? If thats the case all the reflection could then be cached as each situation will be the same for a given type so there won't be much of a performance impact.

Rick-van-Dam avatar Apr 11 '18 21:04 Rick-van-Dam

After looking at expression trees for a bit I now think such functionality is possible without having a heavy performance cost. The whole initialization code can be done in a single expression which is compiled and stored in a dictionary for quick lookups. This means the overhead is just a delegate call, a GetType call and a dictionary lookup.

Rick-van-Dam avatar Apr 16 '18 17:04 Rick-van-Dam

Below is some prototype code that will inject a int with value 5 to all properties in components marked with [Inject]. Ofcourse the value '5' will be replaced with other values or calls to GetComponent eventually. This code is just to get the idea on how to do this. Note that its possible to put any code in the expression body so we only have the overhead once per instance and not per property.

private Dictionary<Type, Action<object>> _injectionCache = new Dictionary<Type, Action<object>>();

protected override void OnGameStarting()
{
	Scene.ComponentAdded += Scene_ComponentAdded;
	Scene.Entered += Scene_Entered;
}

private void Scene_Entered(object sender, EventArgs e)
{
	InjectGameObjects(Scene.Current.AllObjects);
}

protected override void OnGameEnded()
{
	Scene.ComponentAdded -= Scene_ComponentAdded;
	Scene.Entered -= Scene_Entered;
}

private void Scene_ComponentAdded(object sender, ComponentEventArgs e)
{
	InjectComponent(e.Component);
}

public void InjectGameObjects(IEnumerable<GameObject> gameObjects)
{
	foreach (var gameObject in gameObjects)
	{
		foreach (var component in gameObject.GetComponents<Component>())
		{
			InjectComponent(component);
		}
	}
}

public void InjectGameObject(GameObject gameObject)
{
	foreach (var component in gameObject.GetComponents<Component>())
	{
		InjectComponent(component);
	}
}

public void InjectComponent(Component component)
{
	var type = component.GetType();
	if (!_injectionCache.TryGetValue(type, out var action))
	{
		action = GenerateInjectAction(type);
		_injectionCache.Add(type, action);
	}
	action.Invoke(component);
}

public Action<object> GenerateInjectAction(Type type)
{
	var instanceParameter = Expression.Parameter(typeof(object));

	var body = new List<Expression>();
	var instanceCasted = Expression.Variable(type, "instanceCasted");
	body.Add(instanceCasted);
	body.Add(Expression.Assign(instanceCasted, Expression.Convert(instanceParameter, type)));
	foreach (var propertyInfo in type.GetRuntimeProperties())
	{
		if (!propertyInfo.CustomAttributes.Any(x => x.AttributeType == typeof(InjectAttribute))) continue;
		body.Add(Expression.Call(instanceCasted, propertyInfo.SetMethod, Expression.Constant(5))); // Replace the constant with a real value eventually.
	}

	var block = Expression.Block(new[] { instanceCasted }, body);
	var expressionTree = Expression.Lambda<Action<object>>(block, instanceParameter);
	var action = expressionTree.Compile();
	return action;
}

Rick-van-Dam avatar Apr 16 '18 19:04 Rick-van-Dam

As far as the injection API goes for this there are 3 possible ways:

  • Use property injection like in the prototype.
  • Mark a method which will be used for injection. This is more like constructor injection but without the actual constructor.
  • Use constructor injection. I don't think this is possible as classes are instantiated before the game actually runs in the editor for instance and constructors don't give you any freedom in when you call them.

As for the 'AutoRef' functionality this can also be achieved through expression trees by adding a call to GetComponent in the expression. Though iam not yet sure about this functionality itself. It might be better to restrict this to global depedencies and let GetComponent do its own thing.

There is also the question of when it should inject dependencies. On game start and when new components are added or also in the editor (possibly optional?).

Aside from the delegate overhead calling it might cause GC pressure too: https://jacksondunstan.com/articles/3792. Investigate if this if performance turns out to be bad. First create a benchmark to see how fast/slow this expression tree approach is.

EDIT: No GC pressure seems to be generated.

Rick-van-Dam avatar Apr 16 '18 20:04 Rick-van-Dam

The problem seems to be that compiling expressions with Expression.Lambda.Compile causes it to be compiled in a separate assembly thats not fully trusted. This causes a substaintial overhead for each call in another trusted assembly as it has to check rights everytime.

The security overhead seems to go away if you apply the following assembly attribute [assembly: AllowPartiallyTrustedCallers]

This has other caveats though but atleast it makes clear that it really is security related overhead.

Still even with this overhead injecting 10 million objects (with each object having 3 [Inject] attributes) takes only 0.6-0.7 seconds. Manually writing this out with direct calls is about 10 times faster. Writing it out with interface calls is about 5 times faster. So its not that bad. I doubt you would really notice it in a game as you are not going to create even close to that many components. In a extreme case one could create 10000 components which will only take about 600-700 microseconds.

Instead of property injection iam now more leaning towards method injection (which might also have less call overhead as a bonus). I believe that a method makes it much clearer what the dependencies are and it leaves all options open to the developer on how to use them.

EDIT: injection through a method with expression trees takes ~37ns per call regardless of how many parameters.

Rick-van-Dam avatar Apr 19 '18 06:04 Rick-van-Dam

Below you can find the code for a basic injectioncontainer that will inject values based on either a constant value or a expression.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;

namespace ExpressionTrees
{
    public class InjectionContainer
    {
        private readonly Dictionary<Type, Expression> _dependencies = new Dictionary<Type, Expression>();
        private readonly Dictionary<Type, Action<object>> _injectionCache = new Dictionary<Type, Action<object>>();

        /// <summary>
        /// Adds the dependency. Everytime this dependency is needed the same instance will be injected.
        /// </summary>
        /// <typeparam name="TDependency"></typeparam>
        /// <typeparam name="TInstance"></typeparam>
        /// <param name="instance"></param>
        public void AddDependency<TDependency, TInstance>(TInstance instance)
            where TInstance : TDependency
        {
            var expression = Expression.Constant(instance);
            _dependencies.Add(typeof(TDependency), expression);
        }

        /// <summary>
        /// Adds the dependency. Everytime this dependency is needed a new instance will be created and injected.
        /// </summary>
        /// <typeparam name="TDependency"></typeparam>
        /// <typeparam name="TInstance"></typeparam>
        /// <param name="expression">This expression is used to create a new instance</param>
        public void AddDependency<TDependency, TInstance>(Expression<Func<TInstance>> expression)
            where TInstance : TDependency
        {
            _dependencies.Add(typeof(TDependency), expression);
        }

        public void InjectDependencies(object instance)
        {
            var type = instance.GetType();
            if (!_injectionCache.TryGetValue(type, out var action))
            {
                action = GenerateInjectionExpression(type);
                _injectionCache.Add(type, action);
            }
            action.Invoke(instance);
        }

        public Action<object> GenerateInjectionExpression(Type type)
        {
            var instanceParameter = Expression.Parameter(typeof(object));

            var body = new List<Expression>();
            var instanceCasted = Expression.Variable(type, "instanceCasted");
            body.Add(instanceCasted);
            body.Add(Expression.Assign(instanceCasted, Expression.Convert(instanceParameter, type)));
            foreach (var methodInfo in type.GetRuntimeMethods())
            {
                if (methodInfo.CustomAttributes.All(x => x.AttributeType != typeof(InjectAttribute))) continue;
                var parameterTypes = methodInfo.GetParameters();
                var parameterExpressions = GetParameterExpressions(parameterTypes);
                body.Add(Expression.Call(instanceCasted, methodInfo, parameterExpressions));
            }

            var block = Expression.Block(new[] { instanceCasted }, body);
            var expressionTree = Expression.Lambda<Action<object>>(block, instanceParameter);

            var action = expressionTree.Compile();
            return action;
        }

        private Expression[] GetParameterExpressions(ParameterInfo[] parameterTypes)
        {
            var parameterExpressions = new Expression[parameterTypes.Length];
            for (int i = 0; i < parameterTypes.Length; i++)
            {
                if (_dependencies.TryGetValue(parameterTypes[i].ParameterType, out var dependency))
                {
                    switch (dependency)
                    {
                        case ConstantExpression constantExpression:
                            parameterExpressions[i] = constantExpression;
                            break;
                        case LambdaExpression lambdaExpression:
                            parameterExpressions[i] = lambdaExpression.Body;
                            break;
                    }
                }
                else
                {
                    throw new Exception($"No configured dependency found for {parameterTypes[i].ParameterType}");
                }
            }

            return parameterExpressions;
        }
    }
}

Rick-van-Dam avatar Apr 26 '18 10:04 Rick-van-Dam