Observer Pattern instead of the built-in Update() method

[ScorpionMango]

Since I generally tend to avoid updates where the code doesn’t have to run in every frame, I’ve made a few optimizations by using an observer pattern instead, and would like to get feedback on whether it’s better this way or kind of redundant:

This is the Mana.cs:

using System;
using UnityEngine;

namespace RPG.Attributes
{
    public class Mana : MonoBehaviour
    {
        [SerializeField] float maxMana = 200;
        [SerializeField] float manaRegenRate = 1;

        IManaObserver observer = null;

        public Action<float, float> onManaUse;

        float currentMana = 0;

        void Awake()
        {
            currentMana = maxMana;
        }

        void Update()
        {
            if (Mathf.Approximately(currentMana, maxMana)) return;

            RegenerateMana();
        }

        public float MaxMana { get => maxMana; }

        public float CurrentMana { get => currentMana; }

        public IManaObserver Observer { set { observer = value; } }

        public bool UseMana(float mana)
        {
            if (currentMana - mana < 0)
            {
                return false;
            }

            currentMana -= mana;
            onManaUse?.Invoke(currentMana, maxMana);

            return true;
        }

        void RegenerateMana()
        {
            if (currentMana < maxMana)
            {
                currentMana += manaRegenRate * Time.deltaTime;
                NotifyObserver();

                if (currentMana > maxMana)
                {
                    currentMana = maxMana;
                }
            }
        }

        void NotifyObserver()
        {
            observer.OnManaChanged(currentMana, maxMana);
        }

    }
}

This is my ManaDisplay.cs, which is responsible for rendering the amount of mana the player has left. All changes in the Mana.cs are updated accordingly via a call-back:

using RPG.Attributes;
using TMPro;
using UnityEngine;

namespace RPG.Abilities.UI
{
    public class ManaDisplay : MonoBehaviour, IManaObserver
    {
        TextMeshProUGUI manaText;
        Mana mana;

        void Awake()
        {
            manaText = GetComponent<TextMeshProUGUI>();
            mana = GameObject.FindWithTag("Player").GetComponent<Mana>();
            mana.Observer = this;
        }

        void Start()
        {
            OnManaChanged(mana.CurrentMana, mana.MaxMana);
        }

        void OnEnable()
        {
            if (mana != null)
            {
                mana.onManaUse += OnManaChanged;
            }
        }

        void OnDisable()
        {
            if (mana != null)
            {
                mana.onManaUse -= OnManaChanged;
            }
        }

        public void OnManaChanged(float currentMana, float maxMana)
        {
            manaText.text = $"Mana: {Mathf.FloorToInt(currentMana)} / {maxMana}";
        }
    }
}

And finally the interface:

namespace RPG.Attributes
{
    public interface IManaObserver
    {
        public void OnManaChanged(float currentMana, float maxMana);
    }
}

The reason I’m still using Update() in Mana.cs is because of the way we use it in the Ability.cs. We do the check twice if we have enough Mana to perform the operation, first when overriding the Use() method and when we call TargetAcquired() when we lose mana while selecting the targets somehow or for whatever reason, which is still unclear to me.

[Brian_Trotter]

While there’s nothing inherently wrong with this particular solution, It can be greatly simplified by calling onManaUse in RegenerateMana, as your ManaDisplay is already subscribed to it, and it’s already sending the same information along.

You’re quite right, though, that the observer pattern is a better alternative to checking each Update() in ManaDisplay.

[ScorpionMango]

I do get the idea that Unity Events are lightweight, & are a way of allowing a set of methods subscribed to this event to be triggered from elsewhere in the script.
I did my research first before asking this question, "Whether it’s bad practice to invoke Unity Events in Upate()", & couldn’t find any answers regarding the performance implications.

Turns out if there’s just one method subscribed to it, it’s nothing than calling it via a public method. But to avoid circular dependencies, I simply invoke the event to do the job this way.
The only times where this can become crucial are if there are many of them (it can create a sort of “queue” of invocations) or if these subscribed methods are complex (takes heavy computations and takes time)

Therefore, it is important to keep the methods subscribed to an event as efficient as possible. I even tried to profile this part and could not find any significant differences in the Update() section.
So I learned something new through my optimization.

[Brian_Trotter]

There is a way of reducing the number of calls, by only calling when the change is “significant”. For example, it’s probably not terribly useful to change between 98.1 points and 98.15 points… If your only displaying to the whole digit, then it doesn’t make any sense to update 98 to 98…

        void RegenerateMana()
        {
            if (currentMana < maxMana)
            {
                int oldMana = Mathf.Floor(currentMana);
                currentMana += manaRegenRate * Time.deltaTime;
                if (currentMana > maxMana)
                {
                    currentMana = maxMana;
                }
                if(oldMana<Mathf.Floor(currentMana)) NotifyObserver();
            }
        }

[ScorpionMango]

But of course, you can drop certain calls down when rounding a number. That’s clever, I hadn’t thought of that:

So this is my final piece of code for regenerating Mana:

void RegenerateMana()
{
    if (currentMana < maxMana)
    {
        float oldManaPoints = currentMana;
        currentMana += manaRegenRate * Time.deltaTime;

        if (currentMana > maxMana)
        {
            currentMana = maxMana;
        }

        int flooredManaPoints = Mathf.FloorToInt(currentMana);
        if (flooredManaPoints > oldManaPoints)
        {
            onManaUse?.Invoke(flooredManaPoints, maxMana);
        }
    }
}

You take the floor of currentMana and store it in oldMana, while we are only interested in checking if the integer part of currentMana has been incremented, because oldMana is always updated to be the value of currentMana at the beginning of the method, and currentMana is incremented over time.
Both variants do their job, don’t they?

[Brian_Trotter]

This will work perfectly