I'm making an RPG in Unity with an ability-based combat system, think Guild Wars 2 or World of Warcraft. I'm implementing abilities in an inheritance tree structure which makes creating multiple similar abilities very easy and quick, but the whole system feels like it could be structured better.

Any tips for improving this system? I'm not averse a big rewrite if it means more scalable/maintainable code. I've considered rewriting it using a decorator design pattern. What are your opinions?

Also feel free to point out any bad habits in my code. I already know fields in C# shouldn't be public and capitalized and I should use properties instead, but Unity doesn't serialize properties and I don't want to write a [SerializeField] tag over every protected/private field.

Note: I've used Unity's ScriptableObject system on other projects and really dislike it, so I wouldn't want to implement abilities using that.

I'll show you the usual chain of inheritance in order to implement some abilities.

Base Ability Class:

public abstract class Ability

 public string Name;

 public string ImagePath;

 public float PowerCoefficient;
 public float TotalPower get return Owner.AbilityPower * PowerCoefficient; 

 /// <summary>
 /// Total cast time is Owner's GCD + Cast Add
 /// e.g. For a 2 second casted spell, make CastAdd = 0.5f
 /// </summary>
 public float CastAdd;
 public float CastRemaining;

 /// <summary>
 /// Spell's total cast time with CastAdd and Owner's GCD factored in
 /// </summary>
 public float CastTime get return IsCastedAbility ? Owner.GlobalCooldown + CastAdd : 0; 

 /// <summary>
 /// Gives a float 0-1 to indicate percentage of cast completion.
 /// Used for cast bar.
 /// </summary>
 public float CastProgress get return 1.0f - (CastRemaining / CastTime); 

 public bool IsCastedAbility get return CastAdd > 0; 
 public bool IsBeingCasted get return CastRemaining > 0; 
 public bool CastReady get return CastRemaining == 0; 

 public float Cooldown;
 public float CooldownRemaining;

 /// <summary>
 /// Gives a float 0-1 to indicate percentage of cooldown completion.
 /// Used for cooldown indicator.
 /// </summary>
 public float CooldownProgress get return 1.0f - (CooldownRemaining / Cooldown); 

 public bool OffCooldown get return CooldownRemaining <= 0f; 

 public Entity Owner;

 public List<Entity> TargetList;

 // Shorthand for TargetList[0] for single-target abilities
 public Entity Target get return TargetList.Count > 0 ? TargetList[0] : null; 

 public Ability(Entity owner = null)
 
 Owner = owner;
 TargetList = new List<Entity>();
 

 /// <summary>
 /// Called every frame to lower cooldown remaining.
 /// Override for channeled spells and HoTs/DoTs to tick healing/damage.
 /// </summary>
 public virtual void Tick()
 
 // Being casted, tick cast timer and check if ready or if target is dead
 if(IsBeingCasted)
 
 CastRemaining = Mathf.Max(CastRemaining - Time.deltaTime, 0.0f);

 if (TargetList.All(t => t.IsDead))
 
 CancelCast();
 
 else if (CastReady)
 
 Do();
 
 
 // Not being casted, tick cooldown
 else
 
 CooldownRemaining = Mathf.Max(CooldownRemaining - Time.deltaTime, 0.0f);
 
 

 /// <summary>
 /// Default behavior for single target cast, adds target to Targets.
 /// Override for chain/splash/aoe abilities.
 /// </summary>
 /// <param name="target"></param>
 public virtual void StartCast(Entity target = null)
 
 if (target != null)
 
 TargetList.Add(target);
 

 if (IsCastedAbility)
 
 CastRemaining = CastTime;
 
 else
 
 Do();
 
 

 /// <summary>
 /// Clears Targets, doesn't trigger cooldown. Call base.CancelCast() last in overridden function.
 /// </summary>
 public virtual void CancelCast()
 
 TargetList.Clear();
 CastRemaining = 0f;
 

 /// <summary>
 /// Override to make ability do action. base.Do() should be called at end of overridden function.
 /// Default behavior logs action for each Target, clears Targets, and triggers its cooldown
 /// </summary>
 /// <param name="target"></param>
 protected virtual void Do()
 
 if(TargetList.Count > 0)
 
 foreach (var entity in TargetList)
 
 Owner.Mgr.LogAction(Owner, entity, this);
 

 TargetList.Clear();
 

 CooldownRemaining = Cooldown;
 Owner.FinishCast();
 

 /// <summary>
 /// Adds heal predict to each target.
 /// Call when starting to cast a heal.
 /// </summary>
 public void AddHealPredict()
 
 foreach(var target in TargetList)
 
 target.HealPredict += TotalPower;
 
 

 /// <summary>
 /// Removes heal predict from each target.
 /// Call when finishing or canceling a heal cast.
 /// </summary>
 public void RemoveHealPredict()
 
 foreach (var target in TargetList)
 
 target.HealPredict -= TotalPower;
 
 

Base Healing Ability class:

public class HealAbility : Ability

 public HealAbility(Entity owner = null)
 : base(owner)
 

 

 public override void StartCast(Entity target = null)
 
 base.StartCast(target);

 if(IsCastedAbility)
 
 AddHealPredict();
 
 

 public override void CancelCast()
 
 if(IsCastedAbility)
 
 RemoveHealPredict();
 

 base.CancelCast();
 

 protected override void Do()
 
 foreach (var raider in TargetList)
 
 raider.TakeHeal(TotalPower);
 

 if(IsCastedAbility)
 
 RemoveHealPredict();
 

 base.Do();
 

A standard single-target healing ability:

public class Restore : HealAbility

 public Restore(Entity owner = null)
 : base(owner)
 
 Name = "Restore";
 CastAdd = 1.0f;
 PowerCoefficient = 1.25f;
 ImagePath = "Image/Cleric/restore";
 

An chain-heal type ability:

public class Prayer : HealAbility

 int TargetCount;

 public Prayer(Entity owner = null)
 : base(owner)
 
 Name = "Prayer";
 CastAdd = 0.5f;
 PowerCoefficient = 0.33f;
 Cooldown = 0f;
 ImagePath = "Image/Cleric/prayer";
 TargetCount = 3;
 

 public override void StartCast(Entity target)
 
 TargetList = Owner.Group.GetSmartChain(target, TargetCount);
 base.StartCast(null);
 

Glad you took my suggestion and posted over here. I'm going to start from the most important (but also high-level) bits, and work my way towards specifics and code here.

DESIGN ISSUES

The main issue is that you're using lots of inheritance, which is a BadThing(tm). See below:

There are a few things about your design that could be greatly improved, before we go into code. First off are some basic principles of OOP, called SOLID for short. I'm listing all of them for completeness, but your posted code is falling foul of the O and L bits:

Single Object Responsibility: Each class should have one thing that it does (you are doing this).

Open/Closed: A component should be open for extension, but closed for modification - you're falling down here because you're using very deep inheritance trees. This means that, if you need a specific ability to do something new, you will either have to cast that ability to the specific type or add a virtual method in the base class (thus needing to modify the base class design). This process eventually leads to an anti-pattern where the base class accumulates more and more virtual methods. https://en.wikipedia.org/wiki/Open/closed_principle

Liskov Substitution Principle: This is perhaps the most important, and the one that your design fails quite badly on. Basically, this says that if you have a base class, instances of that base class should all adhere to the same contract, regardless of the types derived from it. Roughly speaking, this equates to "don't specialise by inheritance". As in, inherit from a type ONLY to extend its capabilities, not to restrict them. Clearly almost any deep inheritance hierarchy is going to violate this principle. The alternative is to "own" a capability rather than "being" a capability. https://en.wikipedia.org/wiki/Liskov_substitution_principle (a little abstract) and also https://en.wikipedia.org/wiki/Composition_over_inheritance (which is basically a design philosophy that arises from Liskov substitution).

Interface Segregation - interfaces should represent indivisible capabilities. You aren't using interfaces here, but you should be (see below).

Dependency Inversion - your Ability class depends on your Entity class. Assuming that your Entity class also depends on your ability class, you're falling foul of this. The best way to avoid it is to have the Ability class depend on a minimal interface, which has just the properties and methods that you need.

CONFUSION BETWEEN TYPE AND INSTANCE

There is a key issue with this design, beyond OOP principles, which is that you appear to be confusing type and instance - there isn't a distinction between the ability to pray, and the ability of a particular character to pray, for instance.

Essentially, you're missing an entire set of classes that actually cast these abilities. For instance, suppose you wanted to introduce an ability that could be re-cast a limited number of times while it was already in progress. You would have a problem, because the class hierarchy that captures the ability to pray is the same one that's responsible for tracking the execution of a particular prayer.

I would do the following:

1. Introduce and AbilityCaster class, which is basically the instance of an ability owned by an Entity. This would know about the owner (but not the target list). The idea is that calling StartCast() would take a list of targets as a parameter, and return a new CastTracker (you can return a special "already finished" cast tracker for instantaneous abilities).




2. Introduce a CastTracker class that just tracks the timing of a cast, and maintains the list of targets. This would have the Tick() method, and a Cancel() method if something other than timing can cancel a cast. Each tick would call back into the associated AbilityCaster class, with the necessary information to decide what to do.




3. Remove all time tracking code from the Ability class, so it just becomes a stateless specification of capability. This could potentially be abstract, with just one level of implementation for the different capabilities. Something like an abstract DoDamage(Entity owner, IEnumerable targets) method.

The advantage of this design is that you'd quickly find that you don't really need much inheritance at all. I would aim to have at most a single pure abstract base class, and implementations for the different abilities. Any time you need to share some code, just make a new class, and have the other classes own one.

LEAKY ENCAPSULATION

Make everything private. If something needs to be public, check to see where it's used. If it's only used somewhere else, move it.

Never use public fields - anything public should be a property. Never set data on another class instance - all setters should be at most private, but better, simply non-existent (i.e. either set in the constructor or implicit functions).

That's it for now. Two general points to always bear in mind:

1. Inheritance is the devil. Never implement an "is-a" relationship with inheritance. Any time you reach for inheritance, you need to cultivate an instinct to stop and do something different. Note this is different from abstract definition and sealed implementation - that's just a strategy pattern, which is absolutely fine.




2. Make more classes, that do fewer things.

Hope that helps!

Ability

You have some properties and some public fields. In general you should not have public uncontrolled fields. As first step let's make them properties, for example:

public float PowerCoefficient get; set; 

Now let's ask yourself where you will modify them. From the code snippets you posted it seems that some of them are initialized only inside derived classes ctor, let's - at least - mark their setters as protected:

public float PowerCoefficient get; protected set; 

When applicable, for example CastRemaining you may mark the setter as private because it's not changed outside Ability class.

Validate your inputs! Except for private setters all the other values must be validated.

You're using many float properties/fields. Unless you're really trying to save those few bytes required to store values as double then I honestly see no reason to avoid double (it has better precision and it's probably even faster than float without any rounding/speed issue for VERY small numbers when going out from 80 bit CPU registers).

Calculated properties may use a shorter syntax, for example:

public float TotalPower 
 => Owner.AbilityPower * PowerCoefficient;

AddHealPredict() and RemoveHealPredict() don't belong to Ability but to HealAbility (IMO).

Names might be more self-descriptive. What Do() does? It does something but...what? Spell it out.

Owner might be null (you even have a default argument value in ctor) but you use it without any check (for example in Do()).

This class seems not something you can create an instance of. Mark it as abstract. Moreover, exactly because it's abstract, you can remove that default value for owner argument in ctor.

HealAbility

Same as above: make it abstract and remove default value for owner in ctor. In general: double think about default values, there must be a very STRONG reason to use them.

Restore

Same as above for public fields.

Prayer

See Austin's comment, this might not apply.

This makes me perplex. With class Prayer : HealAbility you're actually saying that Prayer IS an HealAbility. Note that it does not mean that it HAS. There is too little about your architecture in this snippet but I'd consider to use interfaces for what a character can DO. Assuming that a character may have more than one ability then I'd also expect a collection, something like this (just a proof of concept):

abstract class Character

 public AbilityCollection Abilities get; 
 = new AbilityCollection();


sealed class Prayer : Character

 public Prayer()
 
 Name = "Prayer;
 ImagePath = "Image/Cleric/prayer";

 Abilities.Add(new HealAbility
 
 CastAdd = 0.5f,
 PowerCoefficient = 0.33f
 );
 

Whare, at first, AbilityCollection might be as simple as:

sealed class AbilityCollection : Collection<IAbility>