Single Instance vs Multiple Instances

[Bahaa]

OK so I know I should’ve known about this a while ago, but since this is slowly becoming a bigger issue day by day, and I pose a significant risk to my project if I don’t get it fixed, I have to ask. In coding terms, how do programmers split between instances of NPCs, or the Player for example, and everyone else using the same script?

For example, one way I recently did it, was to stop all animals (I’m working on animal systems right now, mixing Malbers’ code and our code architecture, and believe me it’s 100% doable, but you have to be extremely smart about it!) from stopping their patrol state if I interrupt one animal using a unique identifier checking system

But I also hear from time to time that if it weren’t a check for just the player, my code would collapse

So, my question is, apart from the UNIQUE ID check, what other solution can we do to ensure that the code doesn’t mix animal/NPC instances, for advanced cases like modifying the code of another assembly definition?

(I’m struggling to explain my own question… )

[Nina]

Hi Bahaa,

In which course and lecture are you?

[Bahaa]

Hi Nina, I’m way beyond the RPG Core Combat Creator Course Series, I forgot to change this to a talk, apologies.

However, I asked this question because sometimes I do struggle with the fact that my code runs every single script holder in the game scene, when all I meant was for this particular script holder, and I want to get this concept clear before I get myself into extremely difficult logical bugs to deal with down the line

It’s a theoretical question that I think @Brian_Trotter may have an answer for

[Pete107]

You could try the Scriptable approach and reduce the tightly coupled code dramatically.

    [CreateAssetMenu(fileName = "Scriptable Reference", menuName = "Scriptables/Game Object Reference", order = 0)]
    public class GameObjectReference : ScriptableObject
    {
        public UnityEvent<GameObject> OnValueChanged;
        [NonSerialized]
        private GameObject value;

        public GameObject Value
        {
            get => value;
            set
            {
                if (this.value == value) return;
                this.value = value;
                OnValueChanged.Invoke(value);
            }
        }
        
        public void AddListener(UnityAction<GameObject> action)
        {
            OnValueChanged.AddListener(action);
        }
        
        public void RemoveListener(UnityAction<GameObject> action)
        {
            OnValueChanged.RemoveListener(action);
        }
        
        public void RemoveAllListeners()
        {
            OnValueChanged.RemoveAllListeners();
        }
    }

This can then be used instead of singletons, my preferred approach.

[Bahaa]

OK I’m not sure how this relates to my problem, but here’s the reason why I asked this question. I’m trying to develop a system that uses probability to determine if the player can mount an animal or not, depending on their level of a skill I developed called ‘Equestrianism’, and it’s a bit of a whacky approach, mainly because I’m working with scripts from a third party developer (MalberS), and it doesn’t always work as expected. If you can help me out, that would be great. Here’s my approach to the problem:

1. Create an ‘EquestrianismEvents.cs’ static class. This will be the mediator between my code and MalberS code:

using System;

// This script is a mediator between 'SkillStore.cs' and 'MRider.cs'

public static class EquestrianismEvents
{
    // Used in 'SkillStore.cs' to be able to determine if the players'
    // Equestrianism level is higher than that of the Animal he's trying
    // to drive, or not
    public static event Func<bool> OnCheckEquestrianismSkill;
    // Used in 'MRider.cs' to block the player from driving an animal, if his equestrianism skill level
    // is not high enough
    public static bool HasEquestrianismLevel() => OnCheckEquestrianismSkill?.Invoke() ?? false;

    // When 'MRider.cs' fails to mount the animal,
    // BECAUSE THE PLAYERS' LEVEL IS NOT HIGH ENOUGH TO MOUNT
    // THIS ANIMAL, you run this event using the 'InvokeOnFailedMount'
    // Function below (Patrolling and Dwelling will handle the attack
    // mechanics for this)
    public static event Action OnFailedMount;

    public static void InvokeOnFailedMount()
    {
        OnFailedMount?.Invoke();
    }

    // For cases when the Player's Mounting level is higher than the minimum required
    // mounting level, this event will determine, through probability, if the player is
    // worth mounting the animal or not. The higher the player's level, the less likely
    // the animal is to start a fight with the player
    public static event Action OnSkillBasedMountFail;

    public static void InvokeOnSkillBasedMountFail()
    {
        OnSkillBasedMountFail?.Invoke();
    }

    // Boolean to determine the result of 'OnSkillBasedMountFail', in 
    // 'AnimalStateMachine.DetermineChanceOfMountSuccess()', and
    // 'AnimalDwellState.DetermineChanceOfMountSuccess()'. If it's true,
    // the player can drive the animal. If false, avoid the player from driving the animal
    // by invoking combat against the player from the animal
    // (The checking for true or false will be done in 'MRider.cs')
    public static bool isMountSuccessful;
}

We don’t care about ‘OnFailedMount’. That ended up working yesterday, so just let it be. For now, let’s focus on invoking ‘OnSkillBasedMountFail’, and ‘isMountSuccessful’ to determine if this mount was successful or not

2. In MalberS code, I went and implemented a few checks to ensure that we follow the rules to be capable of mounting the animal again:

        public virtual void MountAnimal()
        {
            if (!CanMount || !enabled) return;

            // CODE ADDED BY BAHAA - MAKE SURE THE PLAYER HAS A HIGH ENOUGH LEVEL
            // OF EQUESTRIANISM BEFORE BEING ALLOWED TO DRIVE THE ANIMAL, AND RETURN
            // FALSE OTHERWISE
            if (!EquestrianismEvents.HasEquestrianismLevel())
            {
                // If your level is not high enough, you can't mount this animal
                MalbersAnimations.InventorySystem.NotificationManager.Instance.OpenNotification("Action Not Allowed:\nYour Equestrianism Level\nis not high enough");
                EquestrianismEvents.InvokeOnFailedMount();
                return;
            }

            // TEST CODE ADDED BY BAHAA - IF YOU MADE IT THIS FAR, IT MEANS YOUR
            // EQUESTRIANISM LEVEL IS HIGH ENOUGH TO WARRANT AN ATTEMPT TO
            // MOUNT AN ANIMAL. INVOKE AN EVENT HERE TO DETERMINE 
            // THE PROBABILITY OF FAILURE. IF IT'S HIGH, GET THE ANIMAL 
            // TO ATTACK THE PLAYER AGAIN. IF NOT, THEN ALLOW THE MOUNTING
            EquestrianismEvents.InvokeOnSkillBasedMountFail();

            // TEST CODE ADDED BY BAHAA - When the 'OnSkillBasedMountFail' is called, 
            // (LINE ABOVE), some probabilistic calculations will happen in 
            // both patrol and/or dwell states to determine if mounting was 
            // successful or not, otherwise we will invoke on failed mount (just like above) 
            // again:

            if (EquestrianismEvents.isMountSuccessful) // TEST - ONLY THE IF STATEMENT LINE IS ADDED BY BAHAA
            {

            // CODE ADDED BY BAHAA - SOLUTION TO ENSURE ANIMALS AND
            // CHARACTERS (NPCs, PLAYER, etc) DON'T COLLIDE WHEN THE ANIMAL
            // IS DRIVEN, BUT CAN COLLIDE WHEN THE ANIMAL IS NOT DRIVEN, SO
            // COMBAT DOESN'T HAVE ANY ISSUES, AND A MORE REALISTIC INTERACTION
            int animalLayer = LayerMask.NameToLayer("Animal");
            int charactersLayer = LayerMask.NameToLayer("Characters");
            Physics.IgnoreLayerCollision(animalLayer, charactersLayer, true);
            Debug.Log($"[MountAnimal] Ignoring collisions between 'Animal' layer (Layer {animalLayer}) and 'Character' Layer (Layer {charactersLayer})");

            if (!Montura.InstantMount)                                           //If is instant Mount play it      
            {
                Debbuging("Mount Animal", "cyan");
                Mounted = true;                                                  //Update MountSide Parameter In the Animator
                SetMountSide(MountTrigger.MountID);                              //Update MountSide Parameter In the Animator
                // Anim?.Play(MountTrigger.MountAnimation, MountLayerIndex);      //Play the Mounting Animations
            }
            else
            {
                Debbuging("Instant Mount", "cyan");

                Anim?.Play(Montura.MountIdle, MountLayerIndex);                //Ingore the Mounting Animations
                Anim?.Update(0);                             //Update the Animator ????

                Start_Mounting();
                End_Mounting();

                // ADDED BY BAHAA (next 3 lines, that is):
                AnimalMountManager.isOnAnimalMount = true;
                AnimalMountManager.InvokeOnAnimalMounted();
                Debug.Log($"isOnAnimalMount: {AnimalMountManager.isOnAnimalMount}");

                Montura.Rider.transform.SetParent(Montura.transform);
            }
            }
            else
            {
                // TEST CODE BY BAHAA - IF MOUNTING WAS NOT SUCCESSFUL,
                // YOU INVOKE THE SAME 'OnFailedMount' THAT YOU CALL
                // WHEN THE LEVEL IS NOT HIGH ENOUGH AGAIN (Starting an
                // Animal Attacking System against the player)
                EquestrianismEvents.InvokeOnFailedMount();
            }
        }

We don’t care about most of the code, this is the only part that matters for my problem:

            if (EquestrianismEvents.isMountSuccessful) // TEST - ONLY THE IF STATEMENT LINE IS ADDED BY BAHAA
            {
                 // working code here... Ignore this part
            }
            else
            {
                // TEST CODE BY BAHAA - IF MOUNTING WAS NOT SUCCESSFUL,
                // YOU INVOKE THE SAME 'OnFailedMount' THAT YOU CALL
                // WHEN THE LEVEL IS NOT HIGH ENOUGH AGAIN (Starting an
                // Animal Attacking System against the player)
                EquestrianismEvents.InvokeOnFailedMount();
            }

3. Now, in Patrolling and Dwelling states of the animals, I did the following to determine the chance of successfully mounting an animal, based on the level of the player:

// in 'Enter()':

        // Attack the player if he failed to mount the animal
        // (Due to risk probability being high, because the level is not much higher than
        // the level required to start mounting the animal)
        EquestrianismEvents.OnSkillBasedMountFail += DetermineChanceOfMountSuccess;

// in 'Exit()':
        // Attack the player if he failed to mount the animal
        // (Due to risk probability being high, because the level is not much higher than
        // the level required to start mounting the animal)
        EquestrianismEvents.OnSkillBasedMountFail -= DetermineChanceOfMountSuccess;

4. And to determine the chance of success:

    public void DetermineChanceOfMountSuccess()
    {
        stateMachine.StartCoroutine(DelayedDetermineChanceOfMountSuccess());
    }

    private IEnumerator DelayedDetermineChanceOfMountSuccess()
    {
        yield return null; // Spare a frame for 'GetLastFailedMountAnimal()' to be available
        if (stateMachine.PlayerStateMachine.GetLastFailedMountAnimal().GetComponent<Animal>().GetUniqueID() == stateMachine.GetComponent<Animal>().GetUniqueID())
        {
            EquestrianismEvents.isMountSuccessful = false; // This is a static variable, make sure to always reverse it after each attempt

            // If the animal was not mounted before, there's a chance of resistance
            var playerEquestrianismLevel = stateMachine.PlayerStateMachine.SkillStore.GetSkillLevel(Skill.Equestrianism);
            // Values between 0 and 1
            // Chance of successful mounting
            var successfulMountChance = Mathf.Clamp01((playerEquestrianismLevel - stateMachine.MinimumEquestrianismLevelToMount) / (stateMachine.MaximumMountResistanceEquestrianismLevel - stateMachine.MinimumEquestrianismLevelToMount));
            // Chance of random mounting
            var randomMountChance = Random.value;

            var successfulMountChanceFormatted = successfulMountChance.ToString("F2");
            var randomMountChanceFormatted = randomMountChance.ToString("F2");

            if (randomMountChance <= successfulMountChance)
            {
                MalbersAnimations.InventorySystem.NotificationManager.Instance.OpenNotification($"Mounting Success:\nRandomChance: {randomMountChanceFormatted}\nSuccess Chance: {successfulMountChanceFormatted}");
                EquestrianismEvents.isMountSuccessful = true;
            }
            else
            {
                MalbersAnimations.InventorySystem.NotificationManager.Instance.OpenNotification($"Mounting Failed:\nRandom Chance: {randomMountChanceFormatted}\nSuccess Chance: {successfulMountChanceFormatted}");
                EquestrianismEvents.isMountSuccessful = false;
            }

            // IsMountSuccessful is used in 'MRider.cs' to determine if the player can mount the animal,
            // or will we invoke an attacking state by the animal again
        }

You can consider the notification system as an in-game debugger. My problem is, sometimes the notifications don’t show up at all (indicating something is terribly wrong)

and worse, sometimes an animal that I have declared as friendly (i.e: ‘EquestrianismEvents.isMountSuccessful = true’, for ‘MRider.cs’) sometimes fails to make the player capable of mounting the animal again, defying the rule I placed in ‘AnimalIdleState.cs’ (Animal Idle State is my equivalent of a state where the animal essentially is controlled by the player, where I give the controls made by MalberS control once again):

        // When the animal is idle, it means that this animal has been driven
        // before, and it does not need to show any sort of resistance towards
        // the player trying to ride it again, since it's been ridden before
        // (So that 'MRider.cs' can allow the player to freely drive this animal again)
        if (stateMachine.PlayerStateMachine.GetLastFailedMountAnimal().GetComponent<Animal>().GetUniqueID() == stateMachine.GetComponent<Animal>().GetUniqueID())
        {
            // ULTIMATELY, MALBERS 'MRider.cs' WILL BE SEARCHING FOR 'EquestrianismEvents.isMountSuccessful':
            EquestrianismEvents.isMountSuccessful = true; // Ensures that this animal will offer no resistance to mounting attempts from the player ever again
        }

Any idea how to solve the problem of the animal, although it was mounted before, is non-mountable again?

It’s like somewhere I have permanently set ‘isMountSuccessful’ to false by accident, and never reset it again (usually after I try mounting the animal that I setup as a test and failing multiple times)

I’ll leave this up for anyone who can help me fix this

(I know this approach is pretty bad. The fact that I’m using a static variable for this is dangerous, I know, but this is the only way I can think of implementing part of my code in third party scripts, and not to mention, there’s an assembly reference in the same file as the ‘EquestrianismEvents.cs’ script, which is the mediator between my code, and MalberS code. Without that, these two scripts can literally never communicate)

[Pete107]

@Bahaa

isMountSuccessful

Is the coroutine already running? (running the operation multiple times, should it be ran only once per attempt?)
Maybe store the coroutine and check if that routine is already running.
Has it (the coroutine) finished before it gets to the next part
if (EquestrianismEvents.isMountSuccessful) // TEST - ONLY THE IF STATEMENT LINE IS ADDED BY BAHAA

Those are the two that stick out to me.

[Bahaa]

The coroutine works, yes. However, for god knows what reason, let’s say I go and try mounting a higher level animal multiple times and return to a lower level animal, no notifications show up at all, which tells me that the coroutine probably stopped working overall

Because the code doesn’t even get to ‘MRider.MountAnimal’'s check for ‘isMountSuccessful’ (if it did, at least the animal would attack me. That doesn’t happen, and neither does the player mount the animal, and the equestrianism level is high enough to surpass the first threshold in ‘MRider.cs’ (which, again, it probably doesn’t get to see))

[Pete107]

Have you done much in terms of debugging with breakpoints to follow the code and the values? (step over/into/out)
This gives a much broader view on what is actually going on with code.

[Bahaa]

Not yet, will get to work on debugging right away

[Pete107]

I’m unaware of your skill level or knowledge but I mean this is the nicest way possible.
Debugging is an important tool to get used to, I haven’t seen it much, if at all, in any of the courses on here which is surprising as debugging can solve half the issues people have if they were taught.

[Bahaa]

Ehh I’m quite advanced to some extent, xD. I debug all the time, but sometimes I just need to spend some time idle thinking of a solution, or chatting with someone until somehow, I end up in a pitfall where one of us mentions something that miraculously leads to the solution

For now, based on what I noticed, it’s because the horse is in a state where it doesn’t consider the mounting conditions, from what I noticed

By pure accident, I noticed all these mounting rules don’t apply to the animals when they’re in stuff like an attacking state, chasing state, etc - That’s when I realized it’s probably in a state that I didn’t account for (but which one? I don’t know yet, let’s start with idle)

OK AND AFTER A LITTLE BIT OF INVESTIGATION, I NOTICED SOMETHING NEW AND ODD

Let’s say your level is high enough to mount a second animal (or you got any other animal in the scene for this case), and that second animal is in chase or some state where I didn’t get to code it yet. What happens is, the first animal, or other animals in the scene, won’t respond to mounting attempts unless the other animals are all in a state which actually recognizes the mounting attempts

A bit of a weird bug, but all it’s saying is that the code is incomplete. In other words, I need to tell all the animal states exactly what needs to be done when the player tries to mount them when they are, or another animal in the scene, is in a state that does not recognize the mounting attempts just yet.

I’m guessing it’s because literally every single state will be using the ‘EquestrianismEvents.isMountSuccessful’, which will be shared between all sorts of animals, and they’re all probably not classified by instances, even after I created a unique identifier for them all.

And this will be extremely troublesome, because let’s say one animal is in attacking state, and you want to try mount a whole other animal, who has nothing to do with the fight going on. If I were to take a wild guess, you can’t, because based on my current code, you got someone else, completely irrelevant to the animal you want to mount, in attacking state

Let’s see what happens after I complete the code for all other state attempts

[Bahaa]

And now I’m extremely confused. The formula below:

            float successfulMountChance = Mathf.Clamp01((playerEquestrianismLevel - stateMachine.MinimumEquestrianismLevelToMount) / (stateMachine.MaximumMountResistanceEquestrianismLevel - stateMachine.MinimumEquestrianismLevelToMount));

Always returns a zero for god knows what reason. For testing purposes, the player level is 1, the minimum level to mount is zero, and the maximum level to mount is 3, so theoretically it should return 1/3 right? WHY IS IT GIVING ME A ZERO…?!

Edit: Each value of that formula needed a float cast, my bad

[Brian_Trotter]

@Bahaa

In coding terms, how do programmers split between instances of NPCs, or the Player for example, and everyone else using the same script?

As this is OOP (Object Oriented Programming), in general, each character using a script should have it’s own copy of the variables.

@Bahaa

Create an ‘EquestrianismEvents.cs’ static class. This will be the mediator between my code and MalberS code:

Everything in this class is static, meaning that the receivers of the event lack context, breaking the OOP approach… With static events, it’s a good idea to include the sender of the event. Otherwise you don’t know which animal is involved, so they’re all responding if they can.

A better approach may be to use a callback in the method… For example, DetermineChanceOfMountSuccess

    public void DetermineChanceOfMountSuccess(System.Action<bool> callback)
    {
        stateMachine.StartCoroutine(DelayedDetermineChanceOfMountSuccess(callback));
    }

    private IEnumerator DelayedDetermineChanceOfMountSuccess(System.Action<bool> callback)
    ...//skipping to the if
    //Then instead of EquestrianismEvents.isMountSuccessful = true;
    callback?.Invoke(true); //or for the fail, callback?.Invoke(false);

[Bahaa]

@Brian_Trotter

Otherwise you don’t know which animal is involved, so they’re all responding if they can.

To avoid this, I used a unique identifier approach. I didn’t know any better back then, so I worked with what I have

It all boils down to this line, which classifies them apart:

        if (stateMachine.PlayerStateMachine.GetLastFailedMountAnimal().GetComponent<Animal>().GetUniqueID() == stateMachine.GetComponent<Animal>().GetUniqueID())

Regardless, I’ll have a second look on your suggestion and see how I can change my code around

[Bahaa]

@Brian_Trotter OK so let’s assume that I follow along with your context (P.S: I deleted ‘isMountSuccessful’ in ‘EquestrianismEvents.cs’ as per your recommendation), and make some major changes, now I have a few questions:

1. in ‘EquestrianismEvents.cs’, is this correct?:

    // For cases when the Player's Mounting level is higher than the minimum required
    // mounting level, this event will determine, through probability, if the player is
    // worth mounting the animal or not. The higher the player's level, the less likely
    // the animal is to start a fight with the player
    public static Action<System.Action<bool>> OnSkillBasedMountAttempt;

    public static void InvokeOnSkillBasedMountAttempt(System.Action<bool> callback)
    {
        OnSkillBasedMountAttempt?.Invoke(callback);
    }

2. in ‘AnimalPatrolState.cs’, is this new context correct?:

    public void DetermineChanceOfMountSuccess(System.Action<bool> callback)
    {
        stateMachine.StartCoroutine(DelayedDetermineChanceOfMountSuccess(callback));
    }

    private IEnumerator DelayedDetermineChanceOfMountSuccess(System.Action<bool> callback)
    {
        Debug.Log($"DelayedDetermineChanceOfMountSuccess called");
        yield return null; // Spare a frame for 'GetLastFailedMountAnimal()' to be available
        if (stateMachine.PlayerStateMachine.GetLastFailedMountAnimal().GetComponent<Animal>().GetUniqueID() == stateMachine.GetComponent<Animal>().GetUniqueID())
        {
            // If the animal was not mounted before, there's a chance of resistance
            var playerEquestrianismLevel = stateMachine.PlayerStateMachine.SkillStore.GetSkillLevel(Skill.Equestrianism);
            // Values between 0 and 1
            // Chance of successful mounting
            float successfulMountChance = Mathf.Clamp01((float)playerEquestrianismLevel - (float)stateMachine.MinimumEquestrianismLevelToMount) / ((float)stateMachine.MaximumMountResistanceEquestrianismLevel - (float)stateMachine.MinimumEquestrianismLevelToMount);
            // Chance of random mounting
            float randomMountChance = UnityEngine.Random.value;

            if (randomMountChance <= successfulMountChance)
            {
                MalbersAnimations.InventorySystem.NotificationManager.Instance.OpenNotification($"Mounting Success:\nRandomChance: {randomMountChance}\nSuccess Chance: {successfulMountChance}");
                callback?.Invoke(true);
            }
            else
            {
                MalbersAnimations.InventorySystem.NotificationManager.Instance.OpenNotification($"Mounting Failed:\nRandom Chance: {randomMountChance}\nSuccess Chance: {successfulMountChance}");
                callback?.Invoke(false);
            }

            // IsMountSuccessful is used in 'MRider.cs' to determine if the player can mount the animal,
            // or will we invoke an attacking state by the animal again
        }
    }

3. in ‘MRider.cs’, what do I feed it?:

            // TEST CODE ADDED BY BAHAA - IF YOU MADE IT THIS FAR, IT MEANS YOUR
            // EQUESTRIANISM LEVEL IS HIGH ENOUGH TO WARRANT AN ATTEMPT TO
            // MOUNT AN ANIMAL. INVOKE AN EVENT HERE TO DETERMINE 
            // THE PROBABILITY OF FAILURE. IF IT'S HIGH, GET THE ANIMAL 
            // TO ATTACK THE PLAYER AGAIN. IF NOT, THEN ALLOW THE MOUNTING
            EquestrianismEvents.InvokeOnSkillBasedMountAttempt(); // it needs something here...

note, I can’t just give ‘MountAnimal()’ a parameter without risking messing other stuff from Malbers’ code up

4. Can I safely delete this line at the top of ‘DelayedDetermineChanceOfMountSuccess’?:

        if (stateMachine.PlayerStateMachine.GetLastFailedMountAnimal().GetComponent<Animal>().GetUniqueID() == stateMachine.GetComponent<Animal>().GetUniqueID())

That line was just there to get the correct copy, because everyone was responding otherwise

I found myself forced to follow your new approach because for god knows what reason, my old approach was extremely messy with the probabilities of getting the correct value

There’s two wildly wrong things going on here, with my old context:

1. The animal can be mounted ON THE NEXT ATTEMPT, NOT THE CURRENT ATTEMPT, of a ‘success value’ beating the ‘random value’. I want the mount to be done on the same attempt, not the iterative one

2. If I go try mounting another animal, the minimum and maximum values are calculated based on the animal I first tried to mount, until the new animal first fails. Again, something is deeply wrong with the updating system here

[Bahaa]

OK I’m not sure if what I’m doing is correct or wrong, but here’s my original code (so I have something to fall back on if things go wrong):

 // For cases when the Player's Mounting level is higher than the minimum required
    // mounting level, this event will determine, through probability, if the player is
    // worth mounting the animal or not. The higher the player's level, the less likely
    // the animal is to start a fight with the player
    public static event Action OnSkillBasedMountAttempt;

    public static void InvokeOnSkillBasedMountAttempt()
    {
        OnSkillBasedMountAttempt?.Invoke();
    }

    // Boolean to determine the result of 'OnSkillBasedMountFail', in
    // 'AnimalStateMachine.DetermineChanceOfMountSuccess()', and
    // 'AnimalDwellState.DetermineChanceOfMountSuccess()'. If it's true,
    // the player can drive the animal. If false, avoid the player from driving the animal
    // by invoking combat against the player from the animal
    // (The checking for true or false will be done in 'MRider.cs')
    public static bool isMountSuccessful;

and here’s an example of a function that uses that:

// Enter():

        // Attack the player if he failed to mount the animal
        // (Due to level not being high enough to mount the animal to begin with)
        EquestrianismEvents.OnLowLevelMount += SwitchToChasingState;

        // Attack the player if he failed to mount the animal
        // (Due to risk probability being high, because the level is not much higher than
        // the level required to start mounting the animal)
        EquestrianismEvents.OnSkillBasedMountAttempt += DetermineChanceOfMountSuccess;

// Exit():

        // Attack the player if he failed to mount the animal
        // (Due to level not being high enough to mount the animal to begin with)
        EquestrianismEvents.OnLowLevelMount -= SwitchToChasingState;

        // Attack the player if he failed to mount the animal
        // (Due to risk probability being high, because the level is not much higher than
        // the level required to start mounting the animal)
        EquestrianismEvents.OnSkillBasedMountAttempt -= DetermineChanceOfMountSuccess;

    public void DetermineChanceOfMountSuccess()
    {
        stateMachine.StartCoroutine(DelayedDetermineChanceOfMountSuccess());
    }

    private IEnumerator DelayedDetermineChanceOfMountSuccess()
    {
        yield return null; // Spare a frame for 'GetLastFailedMountAnimal()' to be available
        if (stateMachine.PlayerStateMachine.GetLastFailedMountAnimal().GetComponent<Animal>().GetUniqueID() == stateMachine.GetComponent<Animal>().GetUniqueID())
        {
            EquestrianismEvents.isMountSuccessful = false; // This is a static variable, make sure to always reverse it after each attempt

            // If the animal was not mounted before, there's a chance of resistance
            var playerEquestrianismLevel = stateMachine.PlayerStateMachine.SkillStore.GetSkillLevel(Skill.Equestrianism);
            // Values between 0 and 1
            // Chance of successful mounting
            float successfulMountChance = Mathf.Clamp01(((float)playerEquestrianismLevel - (float)stateMachine.MinimumEquestrianismLevelToMount) / ((float)stateMachine.MaximumMountResistanceEquestrianismLevel - (float)stateMachine.MinimumEquestrianismLevelToMount));
            // Chance of random mounting
            float randomMountChance = Random.value;

            if (randomMountChance <= successfulMountChance)
            {
                MalbersAnimations.InventorySystem.NotificationManager.Instance.OpenNotification($"Mounting Success:\nRandomChance: {randomMountChance}\nSuccess Chance: {successfulMountChance}");
                EquestrianismEvents.isMountSuccessful = true;
            }
            else
            {
                MalbersAnimations.InventorySystem.NotificationManager.Instance.OpenNotification($"Mounting Failed:\nRandom Chance: {randomMountChance}\nSuccess Chance: {successfulMountChance}");
                EquestrianismEvents.isMountSuccessful = false;
            }

            // IsMountSuccessful is used in 'MRider.cs' to determine if the player can mount the animal,
            // or will we invoke an attacking state by the animal again
        }
    }

---

and here’s an example of a brand new attempt, in ‘EquestrianismEvents.cs’:

    public static event Action<Action<bool>> OnSkillBasedMountAttempt;
    public static void InvokeOnSkillBasedMountAttempt(Action<bool> callback)
    {
        OnSkillBasedMountAttempt?.Invoke(callback);
    }

and here’s something that corresponds to it:

    public void DetermineChanceOfMountSuccess(Action<bool> callback)
    {
        stateMachine.StartCoroutine(DelayedDetermineChanceOfMountSuccess(callback));
    }

    private IEnumerator DelayedDetermineChanceOfMountSuccess(Action<bool> callback)
    {
        Debug.Log($"DelayedDetermineChanceOfMountSuccess called");
        yield return null; // Spare a frame for 'GetLastFailedMountAnimal()' to be available

        if (stateMachine.PlayerStateMachine.GetLastFailedMountAnimal().GetComponent<Animal>().GetUniqueID() == stateMachine.GetComponent<Animal>().GetUniqueID())
        {
            // If the animal was not mounted before, there's a chance of resistance
            var playerEquestrianismLevel = stateMachine.PlayerStateMachine.SkillStore.GetSkillLevel(Skill.Equestrianism);

            // Values between 0 and 1
            // Chance of successful mounting
            float successfulMountChance = Mathf.Clamp01(
                ((float)playerEquestrianismLevel - (float)stateMachine.MinimumEquestrianismLevelToMount) /
                ((float)stateMachine.MaximumMountResistanceEquestrianismLevel - (float)stateMachine.MinimumEquestrianismLevelToMount)
            );

            // Chance of random mounting
            float randomMountChance = UnityEngine.Random.value;

            Debug.Log($"player equestrianism level: {playerEquestrianismLevel}, minimum level: {stateMachine.MinimumEquestrianismLevelToMount}, max mount level: {stateMachine.MaximumMountResistanceEquestrianismLevel}, minimum level: {stateMachine.MinimumEquestrianismLevelToMount}, successful mount chance: {(float)((playerEquestrianismLevel - stateMachine.MinimumEquestrianismLevelToMount) / (stateMachine.MaximumMountResistanceEquestrianismLevel - stateMachine.MinimumEquestrianismLevelToMount))}, random mount chance: {randomMountChance}");

            if (randomMountChance <= successfulMountChance)
            {
                MalbersAnimations.InventorySystem.NotificationManager.Instance.OpenNotification($"Mounting Success:\nRandomChance: {randomMountChance}\nSuccess Chance: {successfulMountChance}");
                callback?.Invoke(true);
            }
            else
            {
                MalbersAnimations.InventorySystem.NotificationManager.Instance.OpenNotification($"Mounting Failed:\nRandom Chance: {randomMountChance}\nSuccess Chance: {successfulMountChance}");
                callback?.Invoke(false);                
            }
        }
    }

Please let me know if I’m on the right or wrong track

and in ‘MRider.cs’:

// NEW CONTEXT:

            // TEST CODE ADDED BY BAHAA - IF YOU MADE IT THIS FAR, IT MEANS YOUR
            // EQUESTRIANISM LEVEL IS HIGH ENOUGH TO WARRANT AN ATTEMPT TO
            // MOUNT AN ANIMAL. INVOKE AN EVENT HERE TO DETERMINE 
            // THE PROBABILITY OF FAILURE. IF IT'S HIGH, GET THE ANIMAL 
            // TO ATTACK THE PLAYER AGAIN. IF NOT, THEN ALLOW THE MOUNTING
            EquestrianismEvents.InvokeOnSkillBasedMountAttempt(isMountSuccessful => {

            if (isMountSuccessful) 
            {
            // CODE ADDED BY BAHAA - SOLUTION TO ENSURE ANIMALS AND
            // CHARACTERS (NPCs, PLAYER, etc) DON'T COLLIDE WHEN THE ANIMAL
            // IS DRIVEN, BUT CAN COLLIDE WHEN THE ANIMAL IS NOT DRIVEN, SO
            // COMBAT DOESN'T HAVE ANY ISSUES, AND A MORE REALISTIC INTERACTION
            int animalLayer = LayerMask.NameToLayer("Animal");
            int charactersLayer = LayerMask.NameToLayer("Characters");
            Physics.IgnoreLayerCollision(animalLayer, charactersLayer, true);
            Debug.Log($"[MountAnimal] Ignoring collisions between 'Animal' layer (Layer {animalLayer}) and 'Character' Layer (Layer {charactersLayer})");

            if (!Montura.InstantMount)                                           //If is instant Mount play it      
            {
                Debbuging("Mount Animal", "cyan");
                Mounted = true;                                                  //Update MountSide Parameter In the Animator
                SetMountSide(MountTrigger.MountID);                              //Update MountSide Parameter In the Animator
                // Anim?.Play(MountTrigger.MountAnimation, MountLayerIndex);      //Play the Mounting Animations
            }
            else
            {
                Debbuging("Instant Mount", "cyan");

                Anim?.Play(Montura.MountIdle, MountLayerIndex);                //Ingore the Mounting Animations
                Anim?.Update(0);                             //Update the Animator ????

                Start_Mounting();
                End_Mounting();

                // ADDED BY BAHAA (next 3 lines, that is):
                AnimalMountManager.isPlayerOnAnimalMount = true;
                AnimalMountManager.InvokeOnAnimalMounted();
                Debug.Log($"isPlayerOnAnimalMount: {AnimalMountManager.isPlayerOnAnimalMount}");

                Montura.Rider.transform.SetParent(Montura.transform);
            }
            }
            else
            {
                // TEST CODE BY BAHAA - IF MOUNTING WAS NOT SUCCESSFUL,
                // YOU INVOKE THE SAME 'OnFailedMount' THAT YOU CALL
                // WHEN THE LEVEL IS NOT HIGH ENOUGH AGAIN (Starting an
                // Animal Attacking System against the player)
                EquestrianismEvents.InvokeOnLowLevelMount();
            }
        });
        }

// OLD CONTEXT:

            // TEST CODE ADDED BY BAHAA - IF YOU MADE IT THIS FAR, IT MEANS YOUR
            // EQUESTRIANISM LEVEL IS HIGH ENOUGH TO WARRANT AN ATTEMPT TO
            // MOUNT AN ANIMAL. INVOKE AN EVENT HERE TO DETERMINE 
            // THE PROBABILITY OF FAILURE. IF IT'S HIGH, GET THE ANIMAL 
            // TO ATTACK THE PLAYER AGAIN. IF NOT, THEN ALLOW THE MOUNTING
            EquestrianismEvents.InvokeOnSkillBasedMountAttempt();

            // TEST CODE ADDED BY BAHAA - When the 'OnSkillBasedMountFail' is called, 
            // (LINE ABOVE), some probabilistic calculations will happen in 
            // both patrol and/or dwell states to determine if mounting was 
            // successful or not, otherwise we will invoke on failed mount (just like above) 
            // again:

            if (EquestrianismEvents.isMountSuccessful) // TEST - ONLY THE IF STATEMENT LINE IS ADDED BY BAHAA
            {

            // CODE ADDED BY BAHAA - SOLUTION TO ENSURE ANIMALS AND
            // CHARACTERS (NPCs, PLAYER, etc) DON'T COLLIDE WHEN THE ANIMAL
            // IS DRIVEN, BUT CAN COLLIDE WHEN THE ANIMAL IS NOT DRIVEN, SO
            // COMBAT DOESN'T HAVE ANY ISSUES, AND A MORE REALISTIC INTERACTION
            int animalLayer = LayerMask.NameToLayer("Animal");
            int charactersLayer = LayerMask.NameToLayer("Characters");
            Physics.IgnoreLayerCollision(animalLayer, charactersLayer, true);
            Debug.Log($"[MountAnimal] Ignoring collisions between 'Animal' layer (Layer {animalLayer}) and 'Character' Layer (Layer {charactersLayer})");

            if (!Montura.InstantMount)                                           //If is instant Mount play it      
            {
                Debbuging("Mount Animal", "cyan");
                Mounted = true;                                                  //Update MountSide Parameter In the Animator
                SetMountSide(MountTrigger.MountID);                              //Update MountSide Parameter In the Animator
                // Anim?.Play(MountTrigger.MountAnimation, MountLayerIndex);      //Play the Mounting Animations
            }
            else
            {
                Debbuging("Instant Mount", "cyan");

                Anim?.Play(Montura.MountIdle, MountLayerIndex);                //Ingore the Mounting Animations
                Anim?.Update(0);                             //Update the Animator ????

                Start_Mounting();
                End_Mounting();

                // ADDED BY BAHAA (next 3 lines, that is):
                AnimalMountManager.isPlayerOnAnimalMount = true;
                AnimalMountManager.InvokeOnAnimalMounted();
                Debug.Log($"isPlayerOnAnimalMount: {AnimalMountManager.isPlayerOnAnimalMount}");

                Montura.Rider.transform.SetParent(Montura.transform);
            }
            }
            else
            {
                // TEST CODE BY BAHAA - IF MOUNTING WAS NOT SUCCESSFUL,
                // YOU INVOKE THE SAME 'OnFailedMount' THAT YOU CALL
                // WHEN THE LEVEL IS NOT HIGH ENOUGH AGAIN (Starting an
                // Animal Attacking System against the player)
                EquestrianismEvents.InvokeOnLowLevelMount();
            }

Problem is, based on notifications, everything gets called twice now…

And if I were to take a wild guess, it’s because there’s two animals in the scene, and as a result, the animals can invoke a false state with one of them, leading to unintended behaviour. @Brian_Trotter any ideas why that would be the case?

Edit: I added this line check again:

        if (stateMachine.PlayerStateMachine.SkillStore.GetNearestAnimalStateMachine().GetComponent<Animal>().GetUniqueID() == stateMachine.GetComponent<Animal>().GetUniqueID())

and returned to my coroutine approach, ending up with something like this:

    public void DetermineChanceOfMountSuccess(Action<bool> callback)
    {
        stateMachine.StartCoroutine(DelayedDetermineChanceOfMountSuccess(callback));
    }

    private IEnumerator DelayedDetermineChanceOfMountSuccess(Action<bool> callback)
    {
        yield return null; // Spare a frame for 'SkillStore.GetNearestAnimalStateMachine()' to get ready
        if (stateMachine.PlayerStateMachine.SkillStore.GetNearestAnimalStateMachine().GetComponent<Animal>().GetUniqueID() == stateMachine.GetComponent<Animal>().GetUniqueID())
        {
            Debug.Log($"DetermineChanceOfMountSuccess called from {stateMachine.gameObject.name} Attacking State");
            // If the animal was not mounted before, there's a chance of resistance
            var playerEquestrianismLevel = stateMachine.PlayerStateMachine.SkillStore.GetSkillLevel(Skill.Equestrianism);

            // Values between 0 and 1
            // Chance of successful mounting
            float successfulMountChance = Mathf.Clamp01
            (
                ((float)playerEquestrianismLevel - (float)stateMachine.MinimumEquestrianismLevelToMount) /
                ((float)stateMachine.MaximumMountResistanceEquestrianismLevel - (float)stateMachine.MinimumEquestrianismLevelToMount)
            );

            // Chance of random mounting
            float randomMountChance = UnityEngine.Random.value;

            Debug.Log($"player equestrianism level: {playerEquestrianismLevel}, minimum level: {stateMachine.MinimumEquestrianismLevelToMount}, max mount level: {stateMachine.MaximumMountResistanceEquestrianismLevel}, minimum level: {stateMachine.MinimumEquestrianismLevelToMount}, successful mount chance: {(float)((playerEquestrianismLevel - stateMachine.MinimumEquestrianismLevelToMount) / (stateMachine.MaximumMountResistanceEquestrianismLevel - stateMachine.MinimumEquestrianismLevelToMount))}, random mount chance: {randomMountChance}");

            if (randomMountChance <= successfulMountChance)
            {
                MalbersAnimations.InventorySystem.NotificationManager.Instance.OpenNotification($"Mounting Success:\nRandomChance: {randomMountChance}\nSuccess Chance: {successfulMountChance}");
                callback?.Invoke(true);
            }
            else
            {
                MalbersAnimations.InventorySystem.NotificationManager.Instance.OpenNotification($"Mounting Failed:\nRandom Chance: {randomMountChance}\nSuccess Chance: {successfulMountChance}");
                callback?.Invoke(false);
            }
        }
    }

This is not only easier to read, but @Brian_Trotter 's suggestion, although it didn’t solve the “who is calling this script”, it absolutely killed the lag issue I had, where I’d be capable of driving the animal based on the results of my previous attempt, rather than the new one. Thank you Brian!

I feel much better now (tomorrow I’ll work on an animal shop. I decided to make the wild animals wild, with their own characteristics, and then you got animals you can buy, which I’ll work on, and these ones will be your friends through it all) - I still want to fully understand what the new approach of ‘callback?.Invoke(true)’ did to help out, though (because, no offence, it didn’t solve my “two scripts being called at the same time” the way the following line did):

        if (stateMachine.PlayerStateMachine.SkillStore.GetNearestAnimalStateMachine().GetComponent<Animal>().GetUniqueID() == stateMachine.GetComponent<Animal>().GetUniqueID())

In simple terms, the line gets the animal you just tried to mount after the mounting frame, and compares it to the animal you’re trying to mount

[Brian_Trotter]

@Bahaa

3. in ‘MRider.cs’, what do I feed it?:

            // TEST CODE ADDED BY BAHAA - IF YOU MADE IT THIS FAR, IT MEANS YOUR
            // EQUESTRIANISM LEVEL IS HIGH ENOUGH TO WARRANT AN ATTEMPT TO
            // MOUNT AN ANIMAL. INVOKE AN EVENT HERE TO DETERMINE 
            // THE PROBABILITY OF FAILURE. IF IT'S HIGH, GET THE ANIMAL 
            // TO ATTACK THE PLAYER AGAIN. IF NOT, THEN ALLOW THE MOUNTING
            EquestrianismEvents.InvokeOnSkillBasedMountAttempt(); // it needs something here...

note, I can’t just give ‘MountAnimal()’ a parameter without risking messing other stuff from Malbers’ code up

That’s where I’m not sure, and where I’m not comfortable integrating with 3rd party software. From everything I’ve seen, it looks like Malbers is set up for one riding animal, but I can’t be certain.

[Brian_Trotter]

@Bahaa

        if (stateMachine.PlayerStateMachine.SkillStore.GetNearestAnimalStateMachine().GetComponent<Animal>().GetUniqueID() == stateMachine.GetComponent<Animal>().GetUniqueID())

In simple terms, the line gets the animal you just tried to mount after the mounting frame, and compares it to the animal you’re trying to mount

There’s an old saying about caching state, that goes something like this:

I have a performance problem, so I think I’ll add a cache. Now I have two problems…

While it’s ok to cache some things (the component attached to the same GameObject as the script, for example), other things, it’s not always ok to cache. In Asynchronous programming (which is what’s happening here because you’re using the IEnumerator to wait a frame), there is a concept known as thread safety. Nothing should be happening outside of the thread you’re working on that could change the underlying set of data that another thread is working on. If it does, then it’s not thread safe (I’m oversimplifying thread safety’s definition by a lot, but you get the idea).

[Bahaa]

@Brian_Trotter

That’s where I’m not sure, and where I’m not comfortable integrating with 3rd party software. From everything I’ve seen, it looks like Malbers is set up for one riding animal, but I can’t be certain.

            // TEST CODE ADDED BY BAHAA - IF YOU MADE IT THIS FAR, IT MEANS YOUR
            // EQUESTRIANISM LEVEL IS HIGH ENOUGH TO WARRANT AN ATTEMPT TO
            // MOUNT AN ANIMAL. INVOKE AN EVENT HERE TO DETERMINE 
            // THE PROBABILITY OF FAILURE. IF IT'S HIGH, GET THE ANIMAL 
            // TO ATTACK THE PLAYER AGAIN. IF NOT, THEN ALLOW THE MOUNTING
            EquestrianismEvents.InvokeOnSkillBasedMountAttempt(isMountSuccessful =>
            {
                if (isMountSuccessful)
                {
                     // hippity hop, skippity skop, here lies my original code block :P
                });
            }

It’s always good to have someone willing to take risks from time to time. You never know, he can open new gates for ya

@Brian_Trotter

Nothing should be happening outside of the thread you’re working on that could change the underlying set of data that another thread is working on. If it does, then it’s not thread safe

So, on a scale of 1-10, how bad is my approach? As in, assuming I won’t ever touch it again, it should be fine, right? (On an extremely serious note, though, how do I fix this problem? I genuinely want to fix this one. I can smell so many problems down the line if this remains unfixed, and honestly, I want to use best programming practices whenever I can)

Anyway, here’s a video of the game so far: https://www.youtube.com/watch?v=MTgFOiLIe44&t=10s

[Bahaa]

On a side note, does this look correct to be duplication-proof for Unique Identifiers? I did a few changes in my small ‘Animal.cs’ script, changing it from this:

using System;
using GameDevTV.Saving;
using UnityEngine;

namespace RPG.Animals {

public enum AnimalType
{
    Horse,
    Raven,
}

public class Animal : MonoBehaviour, IAnimal, ISerializationCallbackReceiver
{
    [SerializeField] private AnimalType animalType;
    public AnimalType Type => animalType;
    [SerializeField] string uniqueID;

    public Animal GetAnimal()
    {
        return this;
    }

    public void OnBeforeSerialize()
    {
        if (string.IsNullOrEmpty(uniqueID))
        {
            uniqueID = Guid.NewGuid().ToString();
        }
    }

    public void OnAfterDeserialize()
    {
        // Nothing here, just to obey the 'ISerializationCallbackReceiver' Interface rules
    }

    public string GetUniqueID() 
    {
        return uniqueID;
    }
    }
}

to this:

using System;
using GameDevTV.Saving;
using UnityEngine;

namespace RPG.Animals {

public enum AnimalType
{
    Horse,
    Raven,
}

public class Animal : MonoBehaviour, IAnimal, ISerializationCallbackReceiver
{
    [SerializeField] private AnimalType animalType;
    public AnimalType Type => animalType;
    [SerializeField] string uniqueID;

    public Animal GetAnimal()
    {
        return this;
    }

    public void OnBeforeSerialize()
    {
        if (string.IsNullOrEmpty(uniqueID) || !IsUnique(uniqueID))
        {
            uniqueID = Guid.NewGuid().ToString();
            JSONSaveableEntity.globalLookup.Add(uniqueID, this.GetComponent<JSONSaveableEntity>());
        }
    }

    public void OnAfterDeserialize()
    {
        // Nothing here, just to obey the 'ISerializationCallbackReceiver' Interface rules
    }

    public string GetUniqueID() 
    {
        return uniqueID;
    }

    private bool IsUnique(string candidate)
    {

        if (!JSONSaveableEntity.globalLookup.ContainsKey(candidate)) return true;

        if (JSONSaveableEntity.globalLookup[candidate] == this) return true;

        if (JSONSaveableEntity.globalLookup[candidate] == null)
        {
            JSONSaveableEntity.globalLookup.Remove(candidate);
            return true;
        }

        if (JSONSaveableEntity.globalLookup[candidate].GetUniqueIdentifier() != candidate)
        {
            JSONSaveableEntity.globalLookup.Remove(candidate);
            return true;
        }

            return false;

        }
    }
}

and not to mention, I went to ‘JSONSaveableEntity.cs’ and made the ‘globalLookup’ dictionary public, like this (it was initially private):

        // CACHED STATE
        public static Dictionary <string, JSONSaveableEntity> globalLookup = new Dictionary<string, JSONSaveableEntity>();

(Edit: Reversed it, it didn’t behave as expected, and I’m a little deeply busy nowadays )