Ability system in Unity: Scriptable objects and collections of generic types

Question

I've decided to implement an ability system for my game and set the following requirements:

• Abilities must be MonoBehaviors, that-is, components of Player/NPC gameobjects
• Abilities must be able to be added/removed at runtime. Instead of all entites having all abilities on their gameobjects that are disabled/enabled, I'd like to dynamically add/remove abilities using AddComponent/Destroy(component)

Given these I've implemented the following:

• Settings classes which inherit from a base AbilitySettings class which is a ScriptableObject. These contain configurable ability settings as well as an enum called AbilityIdentifier which identifies the ability (for example a jump ability would have the identifier AbilityIdentifier.JUMP)

• IAbility non-generic interface containing a few common ability methods (such as TriggerAbility and CanTrigger)

• AbstractAbility<T> class which implements IAbility and T is a type that extends AbilitySettings. It implements some of the IAbility methods and defines others as abstract. Actual abilities extend this class.

• AbilityManager is a MonoBehavior which contains an array of all possible settings for that entity (added through unity editor) and internally contains a dictionary of <AbilityIdentifier, IAbility>. All of the entities abilities are added/removed using the AbilityManager

It looks something like this:

public class AbilityManager : MonoBehavior
{
    [SerializeField] private AbstractAbilitySettings[] allAbilitiesSettings = { };    

    private readonly Dictionary<AbilityIdentifier, IAbility> abilities = new Dictionary<AbilityIdentifier, IAbility>();

    // Add/remove ability methods
}

For example, a jump ability pickup gameobject is set somewhere in the world as a trigger. When the player moves over the pick-up object and OnTriggerEnter is executed. The script on the pick-up object gets the AbilityManager and calls AddAbility(AbilityIdentifier.JUMP)

This sounds good but It's far from perfect. First of all, I couldn't figure out an elegant way of creating/removing a component when given the settings class so I've added the creation/destruction code to the settings class itself. That-is I've added the following abstract methods to AbilitySettings

public abstract IAbility InstantiateAbility(GameObject gameObject);

public abstract void RemoveAbility(GameObject gameObject);

which are then implemented in each of the concrete settings classes like this:

public override IAbility InstantiateAbility(GameObject gameObject)
{
    JumpAbility ability = gameObject.AddComponent<JumpAbility>();
    ability.Settings = this;
    return ability;
}

public override void RemoveAbility(GameObject gameObject)
{
    JumpAbility ability = gameObject.GetComponent<JumpAbility>();
    Destroy(ability);
}

And these methods are called in the AbilityManager like this

public void AddAbility(AbilityIdentifier identifier)
{
    AbilitySettings abilitySettings = Array.Find(allAbilitiesSettings, s => s.Identifier == identifier);

    abilitySettings.InstantiateAbility(gameObject);
}

The implementation of InstantiateAbility and RemoveAbility is the same for every single ability, the only difference being the ability type. This is a big smell for me. I can't make AbilitySettings generic and generify the two methods as these settings are in an array.

My questions are:

• Adding methods such as InstantiateAbility and RemoveAbility to a scriptable object seems like a code smell to me. Take into account that I'm using the AbilityIdentifier to specify to the manager which ability I want to create. I have thought of perhaps creating an AbilityFactory<T> but since it's a generic class it can't be a part of an array/list so I'm facing the same problem I did with the settings. Is there a different way I could handle this without having the code in the scriptable object?

• Having the implementation of these two methods InstantiateAbility and RemoveAbility be the same for every implementation with the only difference being the type is also a big code smell. Is there any way I can generify this but at the same time avoid problems with the inability of having an array or list of those generic classes?

Answer 1

Here's how I often build lists of generics:

public abstract class AbilitySettingsBase : ScriptableObject {
    public abstract IAbility InstantiateAbility(GameObject gameObject);
    public abstract void RemoveAbility(GameObject gameObject);
}

public abstract class AbilitySettings<T> : AbilitySettingsBase where T : Ability {
    public override IAbility InstantiateAbility(GameObject gameObject) {
    Assert.IsNotNull(gameObject);
        T ability = gameObject.AddComponent<T>();
        ability.Settings = this;
        return ability;
    }

    public override void RemoveAbility(GameObject gameObject) {
        T ability = gameObject.GetComponent<T>();
        if (ability != null) Destroy(ability);
    }
}

[CreateAssetMenu(menuName="Jump Settings")]
public class JumpSettings : AbilitySettings<JumpAbility> {
}

[CreateAssetMenu(menuName="Fight Settings")]
public class FightSettings : AbilitySettings<FightAbility> {
}

public class AbilityManager : MonoBehaviour
{
    //This will show up in the Inspector and we can assign the appropriate assets
    [SerializeField] private AbilitySettingsBase[] abilitySettings = { };    

    //...
}

Here we have a non-generic base type AbilitySettingsBase that defines some abstract functions. We can have a serialized list of AbilitySettingsBase because, even though it's abstract, it's not generic. This will expose the list in the Inspector and allow us to select asset files made from ScriptableObject scripts that extend AbilitySettingsBase.

Where it can get more complicated is if the Ability scripts need to understand the AbilitySettings type. If you try to use generics you might end up with a much more convoluted generic scheme, e.g.

public abstract class AbilitySettings<T, U> : AbilitySettingsBase where T : Ability<T, U> where U : AbilitySettings<T, U> {
    //code ommitted
}

public abstract class Ability<T, U> where T : Ability<T, U> where U : AbilitySettings<T, U> {
    //code ommitted
}

I'm not sure off the top of my head if that would work; I think you'd end up at a catch-22 somewhere in the implementation. Another possibility is to not try to make the generic scheme more complex, and instead do something like this:

public class JumpAbility : Ability {
    private JumpAbilitySettings settings;

    override public AbilitySettingsBase Settings {
        get => settings;
        set {
            Assert.IsNotNull(value);
            JumpAbilitySettings settings = value as JumpAbilitySettings;
            Assert.IsNotNull(settings);
            this.settings = settings;
            //apply settings here
        }
    }
}

This isn't as type-safe but will save some headaches in generic design.

---

EDIT: Part of what makes the above answer complicated is that you've got double-coupling - the Settings is aware of the Ability and the Ability is aware of the Settings. You probably should not make InstantiateAbility and RemoveAbility functions of the AbilitySettingsBase; that way the Settings doesn't need to be generic at all. Instead, you might have a function somewhat like this on your AbilityManager:

public T AddAbility<T, U>(GameObject go) where T : Ability<U> where U : AbilitySettings {
    Assert.IsNotNull(go);
    T ability = go.AddComponent<T>();
    if (abilitySettingsDictionary.TryGetValue(typeof(U), out var settings)) {
        ability.Settings = settings;
    } else {
        Debug.LogError("Didn't find a settings asset for " + typeof(T));
    }
    return ability;
}