Abilties: Preventing premature consumption of timers, mana, and charges

As some students may have noticed, there is a slight flaw in the ointment when it comes to using Abilities in the Shops and Abilities course. Actually… there are two flaws

  • If there are no valid targets selected by the targetingstrategy, the cooldown and mana are still spent.
  • If you can’t actually use an ability due to lack of Mana, cooldown, or a lack of valid targets, but the item is consumable, then the item still disappears from inventory!

It’s easy not to notice these things in playtesting, but they are certainly not issues you want to carry into your production.

The tricky part is the way we implement the abilities. The actual Use method calls a Targeting Strategy, which then returns in Targeting Finished, then applying filter strategies and finally effect strategies. Where do we actually consume the three consumable resources (mana, cooldown, and the item itself)?

Let’s start with dealing with the first issue… if there are no valid targets selected by the TargetingStrategy, the cooldown and mana are still spent.

This has to do with the order in which we are doing things in the TargetAquired method:

        private void TargetAquired(AbilityData data)
        {
            if (data.IsCancelled()) return;

            Mana mana = data.GetUser().GetComponent<Mana>();
            if (!mana.UseMana(manaCost)) return;

            CooldownStore cooldownStore = data.GetUser().GetComponent<CooldownStore>();
            cooldownStore.StartCooldown(this, cooldownTime);

            foreach (var filterStrategy in filterStrategies)
            {
                data.SetTargets(filterStrategy.Filter(data.GetTargets()));
            }
            
            foreach (var effect in effectStrategies)
            {
                effect.StartEffect(data, EffectFinished);
            }
        }

Note that we first check to see if the data is cancelled… Then we cook off the mana and set the cooldown. Finally, we filter. Now suppose you are casting a healing spell, but your character is already at full health. You would be wasting that healing spell altogether. Similarly, if you cast a spell to burn all characters within a target circle, but there are no characters, you probably don’t want to cast the spell…
So we can do a little re-ordering and use if conditions to avoid this.

        private void TargetAquired(AbilityData data)
        {
            if (data.IsCancelled()) return;
            foreach (var filterStrategy in filterStrategies) data.SetTargets(filterStrategy.Filter(data.GetTargets()));
            if(data.GetTargets().Any())
            {
                var mana = data.GetUser().GetComponent<Mana>();
                if (!mana.UseMana(manaCost)) return;
                var cooldownStore = data.GetUser().GetComponent<CooldownStore>();
                cooldownStore.StartCooldown(this, cooldownTime);
                foreach (var effect in effectStrategies) effect.StartEffect(data, EffectFinished);
            }
        }

Now, if there are no targets, nothing happens…
Of course, this doesn’t work if our targeting strategy is DirectionalTargetting like the fireball that shoots at the location of the mouse… because we don’t actually get a target, we just fire in a certain direction.
In this case, you might not want to short circuit the operation of the spell just because of no targets…

For this, we can add a Serialized Field to the Ability class

[SerializeField] private bool noTargetRequired = false;

Now we can change the if statement to read

if(noTargetRequired || data.GetTargets().Any())

Now for spells like the Directional Fireball, you can set noTargetRequired and there won’t even be a check to see if there are any targets for the spell to act on.

So this takes care of the first issue… now to tackle the harder issue, not consuming an item if we can’t actually use the spell at this time…

Currently, our AbilityItem.Use() spell has one parameter, the calling GameObject, and the AbilityStore Use method simply checks to see if the item is consumable and removes the item if it is upon Use(). But the truth is we don’t know if we’ll consume the item until we’ve gotten through the Targeting and possibly filtering stage. We haven’t even checked the Mana and Cooldown, we just deduct the item… So we might be firing off fireballs at all the enemies, but not noticing that because the first fireball’s cooldown hasn’t finished, we can’t actually call the next. The player spams the keyboard and bewm, his wand of fireballs is empty, and he’s only fired off 1 or 2… where did the other 10 go?

This solution is a bit more complicated, and it’s going to rely on a trick we learned in the Shops section, the lambda expression.

All of the following changes will need to be made for the code to compile, so bare with me…
First… we need to have a way of saying to the ActionStore “Hey, we’ve actually used this item”. The trouble is we don’t know if that message is going to come right away or five minutes from now when the user finally clicks on his DelayedClickTargeting (or ever, if he cancels the ability!)

So we need to have some sort of callback… Fortunately, System.Actions are great for just this thing…
So we’ll start with a System.Action completedCallback.
Now if the item isn’t consumable, then there’s really no reason to have a callback, but if it is consumable, then we absolutely need one… And what we want that callback to do is what we’re already doing in the original Use code:
Here’s the original Use method in ActionStore for reference:

        public bool Use(int index, GameObject user)
        {
            if (dockedItems.ContainsKey(index))
            {
                dockedItems[index].item.Use(user);
                if (dockedItems[index].item.isConsumable())
                {
                    RemoveItems(index, 1);
                }
                return true;
            }
            return false;
        }

So in this case, we’re simply removing the item right away if it exists within the ActionStore, not even considering whether or not it’s actually been sucessfully used… let’s change the way this is managed…

Here’s my modified Use method:

        public bool Use(int index, GameObject user)
        {
            if (dockedItems.ContainsKey(index))
            {
                System.Action callback = () =>
                {
                    RemoveItems(index, 1);
                };
                dockedItems[index].item.Use(user, dockedItems[index].item.isConsumable()?callback : null);
                return true;
            }
            return false;
        }

So what we’ve done here is create a System.Action which is a Lambda expression (remember we used Lambda expressions in the Shops section when we assigned the buttons. Our expression contains one simple statement,

RemoveItems(index, 1);

Now when this callback is created, an anonymous function is essentially declared with the ()=>, and the status of the variables in this function are locked into place… So even if we later called this method with a different index, it doesn’t matter, because for this anonymous function, the index is set as the index passed into the method at this time. This is commonly called a Closure.

At this point, however, your compiler is likely screaming at you that no method in ActionItem takes in a System.Action… so that brings us to the next change, in ActionItem.cs

        public virtual void Use(GameObject user, System.Action consumeCallback)
        {
            Debug.Log("Using action: " + this);
            consumeCallback?.Invoke();
        }

We’ve simply added the consumeCallback to the method declaration. This virtual method should always be overridden, but there’s a simple framework for testing.
And now our compiler should be complaining about Ability.cs not having a suitable method to override, because the signature doesn’t match.
That’s the next change, adding the System.Action consumecallback to the Use declaration

        public override void Use(GameObject user, System.Action consumeCallback)

So now we have the link to our consumeCallback, and technically speaking our code will compile, but we still haven’t actually called the consumeCallback yet…
We can’t really call it in Use, because in Use itself we actually have no idea if we’re going to really use the ability or not, until we get through the Targeting phase and Filtering phase. So we’re going to have to store that callback somewhere.
Remember that since our abililities are ScriptableObjects, we can’t actually store the callback there, because if we eventually have our enemies use abilities, they will be using the same ScriptableObjects. Therefore, we need to put the callback in our User data.

Head into AbilityData, and add these lines

private System.Action consumedCallback; //Edit:  typo in first draft said abilityCallback

    public void SetConsumedCallback(System.Action callback)
    {
        consumedCallback = callback;
    }

    public void InvokeConsumedCallback()
    {
        consumedCallback?.Invoke();
    }

The first method assigns the callback. The second method invokes the callback if it is not null.
Now in our AbilityItem.Use() method, we can set the consumedCallback on the AbilityData (after creating the new AbilityData, of course!

data.SetConsumedCallback(consumeCallback);

Now that our AbilityData has a copy of the callback, we can continue on with our Targeting and Effect strategies… let’s go back to the TargetAquired method and put the final touches on our fix:

        private void TargetAquired(AbilityData data)
        {
            if (data.IsCancelled()) return;
            foreach (var filterStrategy in filterStrategies) data.SetTargets(filterStrategy.Filter(data.GetTargets()));
            if(noTargetRequired || data.GetTargets().Any())
            {
                var mana = data.GetUser().GetComponent<Mana>();
                if (!mana.UseMana(manaCost)) return;
                data.InvokeConsumedCallback();
                var cooldownStore = data.GetUser().GetComponent<CooldownStore>();
                cooldownStore.StartCooldown(this, cooldownTime);
                foreach (var effect in effectStrategies) effect.StartEffect(data, EffectFinished);
            }
        }

Now we should have patched both of these inconsistencies in the Abilities.
If there is no target (when a target is required), the ability will not use Mana or start the Cooldown. Consumable abilities will only be consumed when the Mana and Cooldown are actually set, not at the attempt to activate the ability.

2 Likes

Hi again Brian, thanks again for spending the time to address my issues. I had a few issues when trying to follow along with this amazing tutorial:

When fixing the second issue, my compiler doesn’t recognize ‘consumeCallback’ for either of the lines of code below:

private System.Action abilityCallback;

public void SetConsumedCallback(System.Action callback)
    {
        consumedCallback = callback;
    }

    public void InvokeConsumedCallback()
    {
        consumedCallback?.Invoke();
    }

neither do I have a script known as ‘AbilityItem.cs’, hence I cannot apply this line of code either:

data.SetConsumedCallback(consumeCallback);

which was mentioned in the post under this code. Did I miss something out? (I replaced ‘consumedCallback’ with ‘abilityCallback’, and the code… worked for now, but with a few issues, as mentioned below):

  • My health potion, when consumed at full health, or any health at all, doesn’t get destroyed from the game, although it is labelled ‘Consumable’. The player heals, the effect strategies work, but it still stays in the game

  • My Fire Spray spell pretty much doesn’t work, thanks to a Null Reference Exception.

Any fixes for these please? :slight_smile: (the Super Mega Splash works perfectly now)

The first issue was a typo, as I pasted in the methods, but typed in the global variable directly. That should be consumedCallback. (Fixed in post).

The call to data.SetConsumbedCallback(consumeCallback) should be in the Use() method of Ability.cs. The ActionStore is centered around ActionItems, and our Ability class inherits from ActionItem.

The line should go right after

var data = new AbilityData(user);

Here’s the complete Ability.Use() method:

        public override void Use(GameObject user, System.Action consumeCallback)
        {
            var mana = user.GetComponent<Mana>();
            if (mana.GetMana() < manaCost) return;

            var cooldownStore = user.GetComponent<CooldownStore>();
            if (cooldownStore.GetTimeRemaining(this) > 0) return;

            var data = new AbilityData(user);
            data.SetConsumedCallback(consumeCallback);

            var actionScheduler = user.GetComponent<ActionScheduler>();
            actionScheduler.StartAction(data);

            targetingStrategy.StartTargeting(data,
                () => { TargetAquired(data); });
            
        }

That should fix the Health Potion (because if you never set the consumedCallback in the data, then the InvokeConsumedCallback() should do nothing).

For the Fire Spray, I’m not sure what would be going on. I actually tested the finished setup against the Fire Spray in the Course repo.
What’s giving the null reference error (paste in the script with the NRE and mark the line the NRE is occuring on with a ///<<<---- at the end so I can find it.

Hi Brian, the Health Potion issue is not entirely fixed. Yes, it is consumable now, as expected, but it can still be consumed even if the player has full health (is that what is supposed to happen?). As for the Fire Spray, here is my issue:

NullReferenceException: Object reference not set to an instance of an object
RPG.Abilities.Filters.IsAliveFilter+d__0.MoveNext () (at Assets/Project Backup/Scripts/Abilities/Filters/IsAliveFilter.cs:12)
System.Linq.Enumerable.Any[TSource] (System.Collections.Generic.IEnumerable`1[T] source) (at :0)
RPG.Abilities.Ability.TargetAcquired (AbilityData data) (at Assets/Project Backup/Scripts/Abilities/Ability.cs:87)
RPG.Abilities.Ability+<>c__DisplayClass6_0.b__0 () (at Assets/Project Backup/Scripts/Abilities/Ability.cs:57)
RPG.Abilities.Targeting.DirectionalTargeting.StartTargeting (AbilityData data, System.Action finished) (at Assets/Project Backup/Scripts/Abilities/Targeting/DirectionalTargeting.cs:35)
RPG.Abilities.Ability.Use (UnityEngine.GameObject user, System.Action consumeCallback) (at Assets/Project Backup/Scripts/Abilities/Ability.cs:56)
GameDevTV.Inventories.ActionStore.Use (System.Int32 index, UnityEngine.GameObject user) (at Assets/GameDev.tv Assets/Scripts/Inventories/ActionStore.cs:119)
RPG.Control.PlayerController.UseAbilities () (at Assets/Project Backup/Scripts/Control/PlayerController.cs:130)
RPG.Control.PlayerController.Update () (at Assets/Project Backup/Scripts/Control/PlayerController.cs:79)

These are all the lines where the errors occur

The Fire Spray probably shouldn’t be using an IsAlive filter, as it’s aiming in a general direction (DirectionalTargeting). It doesn’t really need a filter at all.

The Health Potion should have a filter that checks each target to see if it is NOT at full health…
For that, you’ll need a filter like this:

IsInjuredFilter.cs
using System.Collections.Generic;
using RPG.Attributes;
using UnityEngine;

namespace RPG.Abilities.Filters
{
    [CreateAssetMenu(fileName = "IsInjuredFilter", menuName = "Filters/Is Injured Filter", order = 0)]
    public class IsInjuredFilter : FilterStrategy
    {
        
        public override IEnumerable<GameObject> Filter(IEnumerable<GameObject> objectsToFilter)
        {
            foreach (GameObject gameObject in objectsToFilter)
            {
                if (gameObject.TryGetComponent(out Health health) && health.GetPercentage()<1.0f)
                {
                    yield return gameObject;
                }
            }
        }
    }
}

Paste in your IsAlive filter, regardless of where you’re using it, it shouldn’t be returning a null reference error.

Hi Brian, here is my IsAliveFilter.cs:

using System.Collections.Generic;
using RPG.Attributes;
using UnityEngine;
 
namespace RPG.Abilities.Filters
{
    [CreateAssetMenu(fileName = "IsAliveFilter", menuName = "Filters/IsAliveFilter", order = 0)]
    public class IsAliveFilter : FilterStrategy
    {
        public override IEnumerable<GameObject> Filter(IEnumerable<GameObject> objectsToFilter)
        {
            foreach (var gameObject in objectsToFilter)
                if (gameObject.TryGetComponent(out Health health) && !health.IsDead())
                    yield return gameObject;
        }
    }
}

Speaking of the Fire Spray, I am aware that it’s a directional ability, but if we are firing something like this on enemies (or allies for this case, if it’s a health potion for instance), I just wanted to make sure that if you’re dead, you can’t get affected (be it health or harm) by this ability, hence why I wanted this filter integrated. As in, what’s the point of using an Ability on something or someone who is dead?

Edit: even when I removed the filter, the FireSpray ability is still not working. I just get a runtime error saying ‘value cannot be null’

Edit 2: The health Potion IsInjuredFilter makes it stop working for some reason. Without the ‘IsInjuredFilter’ mentioned above, it works perfectly fine

Yes, but there are no enemies to target, because the Directional Targeting gives a point/direction, not a target to strike…
That still shouldn’t give a null reference, though… (in fact, I just put the IsAlive filter on the Fire Spray in game, and it still worked as normal. Since the objectsToFilter is empty, nothing went through the foreach.
In any event, remove any filters from FireSpray, since it should always fire towards the mouse regardless of a target.

Even when I removed the filter from the FireSpray, I still get this error:

ArgumentNullException: Value cannot be null.
Parameter name: source
System.Linq.Enumerable.Any[TSource] (System.Collections.Generic.IEnumerable`1[T] source) (at :0)
RPG.Abilities.Ability.TargetAcquired (AbilityData data) (at Assets/Project Backup/Scripts/Abilities/Ability.cs:87)
RPG.Abilities.Ability+<>c__DisplayClass6_0.b__0 () (at Assets/Project Backup/Scripts/Abilities/Ability.cs:57)
RPG.Abilities.Targeting.DirectionalTargeting.StartTargeting (AbilityData data, System.Action finished) (at Assets/Project Backup/Scripts/Abilities/Targeting/DirectionalTargeting.cs:35)
RPG.Abilities.Ability.Use (UnityEngine.GameObject user, System.Action consumeCallback) (at Assets/Project Backup/Scripts/Abilities/Ability.cs:56)
GameDevTV.Inventories.ActionStore.Use (System.Int32 index, UnityEngine.GameObject user) (at Assets/GameDev.tv Assets/Scripts/Inventories/ActionStore.cs:119)
RPG.Control.PlayerController.UseAbilities () (at Assets/Project Backup/Scripts/Control/PlayerController.cs:130)
RPG.Control.PlayerController.Update () (at Assets/Project Backup/Scripts/Control/PlayerController.cs:79)

Out of curiosity… paste in your AbilityData.cs

Sure, here is my ‘AbilityData.cs’:

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using RPG.Core;

public class AbilityData : IAction
{

    GameObject user;
    Vector3 targetedPoint;
    IEnumerable<GameObject> targets;
    bool cancelled = false; // boolean (to cancel our abilities from playing, when we want to play an Ability)

    public AbilityData(GameObject user)
    {

        this.user = user;

    }

    public IEnumerable<GameObject> GetTargets()
    {
        return targets;
    }

    public Vector3 GetTargetedPoint() {

        return targetedPoint;

    }
    
    public void SetTargetedPoint(Vector3 targetedPoint) {

        this.targetedPoint = targetedPoint;

    }

    public GameObject GetUser()
    {
        return user;
    }

    public void SetTargets(IEnumerable<GameObject> targets)
    {
        this.targets = targets;
    }

    public void StartCoroutine(IEnumerator coroutine) {

        // This function takes care of the time to destroy a particle effect, in-game:
        user.GetComponent<MonoBehaviour>().StartCoroutine(coroutine);

    }

    public void Cancel()
    {
        
        // IAction Interface:
        // Cancelling whatever we do, so our ability can be executed:
        cancelled = true;

    }

    public bool IsCancelled() {

        // This boolean is a Getter, used to get the status of whether we have cancelled our action (to play
        // our ability), or not:
        return cancelled;

    }

    private System.Action consumedCallback;

    public void SetConsumedCallback(System.Action callback)
    {

        consumedCallback = callback;

    }

    public void InvokeConsumedCallback()
    {

        consumedCallback?.Invoke();

    }

}

Is the noTargetRequired boolean set? It shouldn’t even be evaluating data.GetTargets().Any()
Let’s take a look at your complete Ability.cs class.

Sure, here is my ‘Ability.cs’:

using GameDevTV.Inventories;
using RPG.Attributes;
using RPG.Core;
using System.Collections.Generic;
using System.Linq;
using UnityEngine;


namespace RPG.Abilities {

    [CreateAssetMenu(fileName = "Ability", menuName = "Abilities/Ability", order = 0)]

    public class Ability : ActionItem
    {

        [SerializeField] TargetingStrategy targetingStrategy;
        [SerializeField] FilterStrategy[] filterStrategies;
        [SerializeField] EffectStrategy[] effectStrategies;
        [SerializeField] float cooldownTime;    // the duration of cooling down our Ability, before its available for reuse to our user (Player)
        [SerializeField] float manaCost = 0;    // how much Mana is used to operate an Ability
        [SerializeField] bool noTargetRequired = false; // checks if our ability requires targets or not (if it does, specific abilities are not played)

        public override void Use(GameObject user, System.Action consumeCallback) {

            // 0. Check for Mana
            Mana mana = user.GetComponent<Mana>();

            if (mana.GetMana() < manaCost) {

                // if we dont have enough Mana, we cant operate the Ability!
                return;

            }
            
            // 1. Get the CooldownStore component:
            CooldownStore cooldownStore = user.GetComponent<CooldownStore>();

            // 2. Check if the Time Remaining > 0, reduce the timer, and get out of this function:
            if (cooldownStore.GetTimeRemaining(this) > 0) {

                return;

            }

            // 3. if there is no timer running, you can use the ability again:

            // This function overrides 'ActionItem.Use()' functionality
            AbilityData data = new AbilityData(user);
            data.SetConsumedCallback(consumeCallback);

            // Preparing to cancel anything we're doing, just so we can play our Ability:
            ActionSchedular actionSchedular = user.GetComponent<ActionSchedular>();
            // Cancelling whatever action we are doing, so we can play our abilities:
            actionSchedular.StartAction(data);

            targetingStrategy.StartTargeting(data, 
            () => {TargetAcquired(data);}); // startTargeting is overridden by 'DemoTargeting.cs' from 'TargetingStrategy.cs'
            // TargetAcquired is a Lambda function, but we dont pass in the () when calling it because we are not calling the function
            // right now, just refering it, so we can call it later from 'TargetingStrategy.cs'

        }

        private void TargetAcquired(AbilityData data) {

            // Let the target go, if the player cancels his action:
            if (data.IsCancelled()) return;

/* 
            // 0. When Acquiring a Target for our ability, the first step is to Consume the Mana
            Mana mana = data.GetUser().GetComponent<Mana>();
            if (!mana.UseMana(manaCost)) return; // consumes the mana, based on the chosen Ability (if Ability is chosen to be used)


            // 1. Get the Player (user) CooldownStore, and then start the 
            // cooldown timer for that, once the effect is played/Target is acquired:
            CooldownStore cooldownStore = data.GetUser().GetComponent<CooldownStore>();
            cooldownStore.StartCooldown(this, cooldownTime); */

            // To apply the bunch of filters, maybe if you want to classify enemies from bosses, enemies from friends, etc:
            
                foreach (var filterStrategy in filterStrategies) {

                    data.SetTargets(filterStrategy.Filter(data.GetTargets()));

                }

                if (noTargetRequired || data.GetTargets().Any()) {

                    // If we have no Target Required for our spells, or any targets at all, apply the Mana and Cooldown Time:

                // 0. When Acquiring a Target for our ability, the first step is to Consume the Mana
                Mana mana = data.GetUser().GetComponent<Mana>();
                if (!mana.UseMana(manaCost)) return; // consumes the mana, based on the chosen Ability (if Ability is chosen to be used)

                data.InvokeConsumedCallback();

                // 1. Get the Player (user) CooldownStore, and then start the 
                // cooldown timer for that, once the effect is played/Target is acquired:
                CooldownStore cooldownStore = data.GetUser().GetComponent<CooldownStore>();
                cooldownStore.StartCooldown(this, cooldownTime);

                foreach (var effect in effectStrategies) {

                    effect.StartEffect(data, EffectFinished);

                }

                }
            
            }

        private void EffectFinished() {

            // This function doesnt do anything, but it must exist if we are inheriting
            // from ActionItem.cs

        }

        }

    }

You love to play stump the TA…
If noTargetsRequired is Checked, then data.GetTargets().Any() should never be called…

Hmmm… I ran this without noTargetsRequired and it gave me a null reference error on this line with data.GetTargets… or more to the point, since there was a filter, it showed the error in the filter, but the callback trace was on the if(noTargetRequired) line…

Working on a patch

Believe it or not, I’m not trying to stump you, and I do apologize if it came along this way. I just have an idea in mind I’d love to see come to life one day :slight_smile:

Hehe, the Stump the TA line is a bit of a jest.
Ok, so here’s the problem… If no targets were ever assigned, then the IEnumerable is technically null… this actually isn’t noticed in the case of FireSpray with the old paradigm because an IEnumerable is a bit like a query statement. It isn’t “real” until you do something with it, like iterate over it or use a Link method like .ToList(), .ToArray(), or in this case .Any()

So the solution is to null check the data.GetTargets() before actualizing it. Here’s my revised method:

        private void TargetAquired(AbilityData data)
        {
            if (data.IsCancelled()) return;
            foreach (var filterStrategy in filterStrategies)
            {
                if (data.GetTargets() == null || !data.GetTargets().Any()) continue;
                data.SetTargets(filterStrategy.Filter(data.GetTargets()));
            }

            if(noTargetRequired || (data.GetTargets()!=null && data.GetTargets().Any()))
            {
                var mana = data.GetUser().GetComponent<Mana>();
                if (!mana.UseMana(manaCost)) return;
                data.InvokeConsumedCallback();
                var cooldownStore = data.GetUser().GetComponent<CooldownStore>();
                cooldownStore.StartCooldown(this, cooldownTime);
                foreach (var effect in effectStrategies) effect.StartEffect(data, EffectFinished);
            }
        }

Any other fixes that might need to be done? Because surprisingly, it’s not working for me

Can you be more specific? New error?

Privacy & Terms