dimanche 30 juin 2019

Proper way to get the right implemented overload in a child class

Let me first introduce what I am doing.

In Unity I want to make some GameObjects (like the player) able to pick up items (others GameObjects).

In order to do that, I designed this basic code:

A component which pulls and pick up items:

public class PickupMagnet : MonoBehaviour
{   
    // [...]
    private void Update()
    {
        Transform item = FindClosestItemInRange(); // Well, this line doesn't exist, it's just a simplification.
            if (ítem != null)
             Pickup(item);
    }

    private void Pickup(Transform item)
    {
        IPickup pickup = item.GetComponent<IPickup>();
        if (pickup != null)
        {
            pickup.Pickup();
            Destroy(item);
        }
    }   
}

The interface of those (that at the moment) items:

public interface IPickup
{
    void Pickup();
    // [...]
}

And the single item I made at that moment:

public class Coin : MonoBehaviour, IPickup
{
    private int price;
    // [...]

    void IPickup.Pickup()
    {
        Global.money += price; // Increase player money
    }   
    // [...]
}

Everything worked perfectly fine until I wanted to add a new item: a health pack. This item, increases the health of the creature who pick up it. But in order to do that, I need the instance of the creature script: LivingObject.

public class HealthPack: MonoBehaviour, IPickup
{
    private int healthRestored;
    // [...]

    void IPickup.Pickup(LivingObject livingObject)
    {
        livingObject.TakeHealing(healthRestored);
    }   
    // [...]
}

The problem is that IPickup.Pickup() don't have any parameter on it. Obviously, I could just change it to IPickup.Pickup(LivingObject livingObject) and ignore the parameter on Coin.Pickup, but what if in the future I want to add more kinds of items, which require different arguments? Other option would be adding a new method to the interface, but that forces me to implement Coin.Pickup(LivingObject livingObject) and implement it.

After thinking about it I removed IPickup and replace it with:

public abstract class Pickupable : MonoBehaviour
{
    // [...]
    public abstract bool ShouldBeDestroyedOnPickup { get; }
    public virtual void Pickup() => throw new NotImplementedException();
    public virtual void Pickup(LivingObject livingObject) => throw new NotImplementedException();
}

And then override the necessary method in Coin and in HealthPack. Also, I changed the PickupMagnet.Pickup(Transform item) with:

public class PickupMagnet : MonoBehaviour
{
    // [...]
    private LivingObject livingObject;

    private void Start()
    {
        livingObject = gameObject.GetComponent<LivingObject>();
    }
    // [...]
    private void Pickup(Transform item)
    {
        Pickupable pickup = item.GetComponent<Pickupable>();
        if (pickup != null)
        {
            Action[] actions = new Action[] { pickup.Pickup, () => pickup.Pickup(livingObject) };
            bool hasFoundImplementedMethod = false;
            foreach (Action action in actions)
            {
                try
                {
                    action();
                    hasFoundImplementedMethod = true;
                    break;
                }
                catch (NotImplementedException) { }
            }

            if (!hasFoundImplementedMethod)
                throw new NotImplementedException($"The {item.gameObject}'s {nameof(Pickup)} class lack of any Pickup method implementation.");
            else if (pickup.ShouldBeDestroyedOnPickup)
                Destroy(item.gameObject);
        }
    }
}

Basically, this iterates over all the methods defined in actions and execute them. If they raise a NotImplementedException it keeps trying with other methods from the array.

This code works fine, but personally, I didn't like the idea of defining that array with each overload of the Pickable.Pickup.

So, I started to do some research, and I found something called "reflection". I still not sure how that works in deep, but I managed to make this working code.

private void Pickup(Transform item)
{
    Pickupable pickup = item.GetComponent<Pickupable>();
    if (pickup != null)
    {
        bool hasFoundImplementedMethod = false;
        foreach (MethodInfo method in typeof(Pickupable).GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly))
        {
            if (method.Name == "Pickup")
            {
                ParameterInfo[] parametersGetted = method.GetParameters();
                int parametersAmount = parametersGetted.Length;
                if (parametersAmount == 0)
                {
                    bool succed = TryCatchInvoke(pickup, method, Array.Empty<object>());
                    if (succed) hasFoundImplementedMethod = true;
                }
                else
                {
                    object[] parametersObjects = new object[method.GetParameters().Count()];
                    for (int i = 0; i < parametersAmount; i++)
                    {
                        Type parameterType = parametersGetted[i].ParameterType;
                        if (parameters.TryGetValue(parameterType, out object parameterObject))
                            parametersObjects[i] = parameterObject;
                        else
                            throw new KeyNotFoundException($"The key Type {parameterType} was not found in the {nameof(parameters)} dictionary.");
                    }
                    bool succed = TryCatchInvoke(pickup, method, parametersObjects);
                    if (succed) hasFoundImplementedMethod = true;
                }
            }
        }

        if (!hasFoundImplementedMethod)
            throw new NotImplementedException($"The {item.gameObject}'s {nameof(Pickup)} class lack of any Pickup method implementation.");
        else if (pickup.ShouldBeDestroyedOnPickup)
            Destroy(item.gameObject);
    }
}

private bool TryCatchInvoke(Pickupable instance, MethodInfo method, object[] args)
{
    try
    {
        method.Invoke(instance, args);
        return true;
    }
    catch (Exception) // NotImplementedException doesn't work...
    {
        return false;
    }
}

And added to MagnetPickup:

private LivingObject livingObject;
private Dictionary<Type, object> parameters;

private void Start()
{
    livingObject = gameObject.GetComponent<LivingObject>();
    parameters = new Dictionary<Type, object> { { typeof(LivingObject), livingObject } };
}

... and works.

I'm not very familiarized with Unity profiler, but I think that the last code worked a bit (less than 1%) faster.

The problem is that I'm not sure if that code will bring me problems in the future, so this is my question: Is reflection a proper way to solve this problem or should I use my try/catch attempt or maybe another code?

For just 1% I am not sure if I should take the risk of using it. I'm not looking for the best perfomance, just the proper way to solve this.





Aucun commentaire:

Enregistrer un commentaire