Cancelling Coroutine when Home button presseddown or returned main menu

Viewed 357

some pretext of what I am doing ; I am currently locking down my skill buttons via setting interactable = false in coroutines. Showing text of remaning seconds via textmeshpro and setting them deactive when countdown is over. But I am having problem when home button is pressed/ returned main menu. I would like to refresh my buttons cooldowns and stop coroutines when its pressed. But it is staying in lock position.

this is my cooldown coroutine

static List<CancellationToken> cancelTokens = new List<CancellationToken>();

...

public IEnumerator StartCountdown(float countdownValue, CancellationToken cancellationToken)
    {
        try
        {
            this.currentCooldownDuration = countdownValue;
            // Deactivate myButton
            this.myButton.interactable = false;
            //activate text to show remaining cooldown seconds
            this.m_Text.SetActive(true);
            while (this.currentCooldownDuration > 0 && !cancellationToken.IsCancellationRequested)
            {

                this.m_Text.GetComponent<TMPro.TextMeshProUGUI>().text = this.currentCooldownDuration.ToString(); //Showing the Score on the Canvas
                yield return new WaitForSeconds(1.0f);
                this.currentCooldownDuration--;

            }
        }
        finally
        {
            // deactivate text and Reactivate myButton
            // deactivate text
            this.m_Text.SetActive(false);
            // Reactivate myButton
            this.myButton.interactable = true;
        }

    }
static public void cancelAllCoroutines()
   {
       Debug.Log("cancelling all coroutines with total of : " + cancelTokens.Count);
       foreach (CancellationToken ca in cancelTokens)
       {
           ca.IsCancellationRequested = true;
       }
   }


void OnButtonClick()
    {

    CancellationToken cancelToken = new CancellationToken();
        cancelTokens.Add(cancelToken);

        Coroutine co;
        co = StartCoroutine(StartCountdown(cooldownDuration, cancelToken));
        myCoroutines.Add(co);

    }

this is where I catch when home button pressed/returned main menu. when catch it and pop pauseMenu

public void PauseGame()
    {
        GameObject menu = Instantiate(PauseMenu);
        menu.transform.SetParent(Canvas.transform, false);
        gameManager.PauseGame();
        EventManager.StartListening("ReturnMainMenu", (e) =>
        {
            Cooldown.cancelAllCoroutines();
            Destroy(menu);
            BackToMainMenu();
            EventManager.StopListening("ReturnMainMenu");
        });
        ...

I also stop time when game on the pause

public void PauseGame() {
        Time.timeScale = 0.0001f;
    }
3 Answers

You are using CancellationToken incorrectly in this case. CancellationToken is a struct that wraps a CancellationTokenSource like this:

public bool IsCancellationRequested 
{
    get
    {
        return source != null && source.IsCancellationRequested;
    }
}

Because it's a struct, it gets passed around by value, meaning the one you store in your list is not the same instance as the one that your Coroutine has.

The typical way to handle cancellation is to create a CancellationTokenSource and pass its Token around. Whenever you want to cancel it, you simply call the .Cancel() method on the CancellationTokenSource. The reason for it being this way is so that the CancellationToken can only be cancelled through the 'source' reference and not by consumers of the token.

In your case, you are creating a token with no source at all so I would suggest making the following changes:

First of all, change your cancelTokens list to be a:

List<CancellationTokenSource>

Next, change your OnButtonClick() method to look like this:

public void OnButtonClick()
{
    // You should probably call `cancelAllCoroutines()` here  
    cancelAllCoroutines();

    var cancellationTokenSource = new CancellationTokenSource();
    cancelTokens.Add(cancellationTokenSource);
    Coroutine co = StartCoroutine(StartCountdown(cooldownDuration, cancellationTokenSource.Token));
    myCoroutines.Add(co);
}

And lastly, change your cancelAllCoroutines() method to this:

public static void CancelAllCoroutines()
{
   Debug.Log("cancelling all coroutines with total of : " + cancelTokens.Count);
   foreach (CancellationTokenSource ca in cancelTokens)
   {
       ca.Cancel();
   }

   // Clear the list as @Jack Mariani mentioned
   cancelTokens.Clear();
}

I would suggest reading the docs on Cancellation Tokens or alternatively, as @JLum suggested, use the StopCoroutine method that Unity provides.

EDIT: I forgot to mention that it is recommended that CancallationTokenSources be disposed of when no longer in use so as to ensure no memory leaks occur. I would recommend doing this in an OnDestroy() hook for your MonoBehaviour like so:

private void OnDestroy()
{
    foreach(var source in cancelTokens)
    {
        source.Dispose();
    }
}

EDIT 2: As @Jack Mariani mentioned in his answer, multiple CancellationTokenSources is overkill in this case. All it would really allow you to do is have more fine-grained control over which Coroutine gets cancelled. In this case, you are cancelling them all in one go, so yeah, an optimisation would be to only create one of them. There are multiple optimisations that could be made here, but they are beyond the scope of this question. I did not include them because I felt like it would bloat this answer out more than necessary.

However, I would argue his point about CancellationToken being 'mostly intended for Task'. Pulled straight from the first couple of lines in the MSDN docs:

Starting with the .NET Framework 4, the .NET Framework uses a unified model for cooperative cancellation of asynchronous or long-running synchronous operations. This model is based on a lightweight object called a cancellation token

CancellationTokens are lightweight objects. For the most part, they are just simple Structs that reference a CancellationTokenSource. The 'overhead' that is mentioned in his answer is negligible and, in my eyes, totally worth it when considering readability and intention.

You could pass a load of booleans around with indices or subscribe to events using string literals and those approaches would work. But at what cost? Confusing and difficult-to-read code? I would say not worth it.

The choice is ultimately yours though.

MAIN ISSUE

In your question you just use the bool cancellationToken.IsCancellationRequested and no other functionalities of Cancellation Token.

So, following the logic of your method, you might just want to have a List<bool> cancellationRequests and pass them using ref keyword.

Still, I would not go ahead with that logic nor with the logic proposed by Darren Ruane, because they have one main flaw.

FLAW => these solutions keep adding things to 2 lists cancelTokens.Add(...) and myCoroutines.Add(co), without ever clearing them.


public void OnButtonClick()
{
    ...other code
    //this never get cleared
    cancelTokens.Add(cancelToken);
    ...other code
    //this never get cleared
    myCoroutines.Add(co);
}

If you want to go down this way you could remove them manually, but it's tricky, because you never know when they will be required (the Coroutine can be called many frames after CancelAllCoroutines method).


SOLUTION

Use a static event instead of a list

To remove the list and make the class even more decoupled you might use a static event, created and invoked in the script where you call the PauseGame method.

//the event somewhere in the script
public static event Action OnCancelCooldowns;

public void PauseGame()
{
    ...your code here, with no changes...
    EventManager.StartListening("ReturnMainMenu", (e) =>
    {
        //---> Removed ->Cooldown.cancelAllCoroutines();
        //replaced by the event
        OnCancelCooldowns?.Invoke();

        ...your code here, with no changes...
    });
    ...

You will listen to the static event in your coroutine.

public IEnumerator StartCountdown(float countdownValue, CancellationToken cancellationToken)
{
    try
    {
        bool wantToStop = false;
        //change YourGameManager with the name of the script with your PauseGame method.
        YourGameManager.OnCancelCooldowns += () => wantToStop = true;

        ...your code here, with no changes...

        while (this.currentCooldownDuration > 0 && !wantToStop)
        {
            ...your code here, with no changes...
        }
    }
    finally
    {
        ...your code here, with no changes...
    }
}

EASIER SOLUTION

Or you might just stay simple and use MEC Coroutines instead of Unity ones (they are also more performant ).

MEC Free offers the tag functionality that solves the problem entirely.

SINGLE COROUTINE START

void OnButtonClick() => Timing.RunCoroutine(CooldownCoroutine, "CooldownTag");  

STOP ALL COROUTINE with a specific tag

public void PauseGame()
{
    ...your code here, with no changes...
    EventManager.StartListening("ReturnMainMenu", (e) =>
    {
        //---> Removed ->Cooldown.cancelAllCoroutines();
        //replaced by this
        Timing.KillCoroutines("CooldownTag");

        ...your code here, with no changes...
    });
    ...

Further notes on tags (please consider that MEC free has only tags and not layers, but you require just tags in your use case).

EDIT: After some thought I just decided to remove the details of the bool solution, it might confuse the answer and it was going beyond the scope of this question.

Unity has StopCoroutine() specifically for ending Coroutines early.

You will want to create a function that you can call on every skill button object when you want to reset them all like this:

void resetButton()
{
    StopCoroutine(StartCountdown);
    this.currentCooldownDuration = 0;
    this.m_Text.SetActive(false);
    this.myButton.interactable = true;
}
Related