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.
