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;
object[] parametersObjects = new object[parametersAmount ];
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.
I do think the best way of doing this is to send in a reference to your Player object in Pickup and then do custom logic per type of Pickup object. I you could probably skip the interface and just have a base object called "PickupObject" or something and then let the FindClosestItemInRange return those.
And lastly you should destroy the GameObject instead of what ever you pass in to the Pickup function. You can probably destroy the Transform and get the same result (I have not tried this), but it's just good practice to actually destroy the GameObject and not any component of the GameObject
public class PickupObject : MonoBehaviour
{
virtual void Pickup(Player playerObject) { }
}
public class Coin : PickupObject
{
public int price;
override void Pickup(Player playerObject)
{
playerObject.money += price; // Move money over to the player as it probably makes more sense
}
}
public class HealthPack : PickupObject
{
public int healthRestored;
override void Pickup(Player playerObject)
{
playerObject.health += healthRestored;
}
}
public class PickupMagnet : MonoBehaviour
{
public Player PlayerObject;
private void Update()
{
PickupObject item = FindClosestItemInRange();
Pickup(item);
}
private void Pickup(PickupObject pickup)
{
pickup.Pickup(PlayerObject);
Destroy(pickup.gameObject);
}
}
Edit, some general thoughts on you code:
If you have general "LivingObject" that can pickup both health and coins as your code suggest, you are probably generalizing a bit too much. It sounds like you just have a player that needs to pickups stuff. To let "anything" be able to pickup "anything" is in my experience just too much of a generalization. Don't try to solve every future problem on the first lines of code. If you are not sure where you are headed or the structure is tricky at this early stage. Write code that does what you need it to do and refactor it when patterns and repetition emerges.