Search Unity

  1. Megacity Metro Demo now available. Download now.
    Dismiss Notice
  2. Unity support for visionOS is now available. Learn more in our blog post.
    Dismiss Notice

Coroutine Starts Once and cant be stopped?

Discussion in 'Scripting' started by nevaran, Aug 25, 2018.

  1. nevaran

    nevaran

    Joined:
    May 21, 2010
    Posts:
    247
    Hey, I've been having this weird bug(?) or something or other with coroutines that worked a few years back when I used it:

    I create an IEnumerator function and a reference to it, I set the reference at start and then use Start/StopCoroutine(reference) to start and stop it.

    But the thing is, it starts only once, and it cannot be stopped.

    Am I forgetting something?

    Heres the code:

    Code (CSharp):
    1. public void ApplyDamage(int dmg)
    2.     {
    3.         hitPoints -= dmg;
    4.         //NOTE: overkill damage code here
    5.         hitPoints = Mathf.Clamp(hitPoints, 0, hitPointsMax);
    6.         //TODO: sound and effects when damaged
    7.  
    8.         GameManager.control.UpdateHitPointsUI();
    9.  
    10.         if (hitPoints <= 0)
    11.             Die();
    12.         else
    13.         {
    14.             StopCoroutine(regenerateHealth);
    15.             StartCoroutine(regenerateHealth);
    16.         }
    17.     }
    18.  
    19.     IEnumerator RegenerateHealth()
    20.     {
    21.         yield return new WaitForSeconds(2f);
    22.         Debug.Log("started regen");
    23.         while(hitPoints < hitPointsMax)
    24.         {
    25.             hitPoints += Time.fixedDeltaTime * (hitPointsMax * 0.03f);
    26.             GameManager.control.UpdateHitPointsUI();
    27.  
    28.             yield return new WaitForFixedUpdate();
    29.         }
    30.  
    31.         hitPoints = Mathf.Clamp(hitPoints, 0, hitPointsMax);
    32.     }
     
  2. Doug_B

    Doug_B

    Joined:
    Jun 4, 2017
    Posts:
    1,596
    Is line 15 even compiling for you? It may be worth taking a look at the examples given in the Unity documentation.

    You could record the return value of the StartCoroutine() and stop that. So maybe something like this :
    Code (CSharp):
    1.     public void ApplyDamage(int dmg)
    2.     {
    3.         hitPoints -= dmg;
    4.         //NOTE: overkill damage code here
    5.         hitPoints = Mathf.Clamp(hitPoints, 0, hitPointsMax);
    6.         //TODO: sound and effects when damaged
    7.  
    8.         GameManager.control.UpdateHitPointsUI();
    9.  
    10.         if (hitPoints <= 0)
    11.             Die();
    12.         else
    13.         {
    14.             StopCoroutine(m_regenHealth);
    15.             m_regenHealth = StartCoroutine(RegenerateHealth());
    16.         }
    17.     }
    18.  
    19.     Coroutine m_regenHealth;
     
  3. nevaran

    nevaran

    Joined:
    May 21, 2010
    Posts:
    247
    Everything is fine, I neglected to add the variable to the code because its separated, but here:


    private IEnumerator regenerateHealth;
    void Start(){
    regenerateHealth = RegenerateHealth();
    }


    This is the rest of the code on the coroutine
     
  4. nevaran

    nevaran

    Joined:
    May 21, 2010
    Posts:
    247
    After I tried your method(with Coroutine instead of IEnumerator, and assigning new reference each time I start a new one) it seems to be working. Though I dont think it is very efficient to be calling often?
     
  5. Doug_B

    Doug_B

    Joined:
    Jun 4, 2017
    Posts:
    1,596
    Can I just check what exactly you are trying to do. Are you wanting:
    1. On regenerating health, the health value grows smoothly over a number of frames.
    2. On receiving damage, the health value reduces smoothly over a number of frames.
    3. Any change to the health value is adjusted smoothly over a number of frames (i.e. both 1 and 2).
    4. Something else?
     
  6. nevaran

    nevaran

    Joined:
    May 21, 2010
    Posts:
    247

    Whenever the player takes damage, if he doesnt take any more damage over X number of seconds, it will gradually start regenerating (3% per second for example)
     
  7. nevaran

    nevaran

    Joined:
    May 21, 2010
    Posts:
    247
    Heres the current version, that seems to be working properly now - though i'd like more performant version, if possible...

    Code (CSharp):
    1. private Coroutine regenerateHealth;
    2.  
    3.     public void ApplyDamage(int dmg)
    4.     {
    5.         hitPoints -= dmg;
    6.  
    7.         if (regenerateHealth != null)
    8.             StopCoroutine(regenerateHealth);
    9.  
    10.         if (hitPoints <= 0)
    11.             Die();
    12.         else
    13.             regenerateHealth = StartCoroutine(RegenerateHealth());
    14.     }
    15.  
    16.     IEnumerator RegenerateHealth()
    17.     {
    18.         yield return new WaitForSeconds(2f);
    19.         Debug.Log("started regen");
    20.         while(hitPoints < hitPointsMax)
    21.         {
    22.             hitPoints += Time.fixedDeltaTime * (hitPointsMax * 0.03f);
    23.             GameManager.control.UpdateHitPointsUI();
    24.  
    25.             yield return new WaitForFixedUpdate();
    26.         }
    27.  
    28.         hitPoints = Mathf.Clamp(hitPoints, 0, hitPointsMax);
    29.     }
     
  8. Doug_B

    Doug_B

    Joined:
    Jun 4, 2017
    Posts:
    1,596
    Cool. If I could make some observations :
    1. As you have the null check on line 7, you could set it to null at the end of RegenerateHealth() just before it exits.
    2. You haven't shown hitPoints but ApplyDamage takes an int. If hitPoints is an int, then the logic on line 22 may not work as intended. Hence I am guessing that hitPoints is a float?
    3. On line 25, you only really want to wait til the next display frame. You would likely be better using
      yield return null;
      .
    4. Following that, line 22 would use
      Time.deltaTime
      rather than
      fixedDeltaTime
      .
    Why? What performance issues are you seeing? Have you got the results from the profiler- does that show that this Coroutine is causing the bottleneck? :)
     
  9. nevaran

    nevaran

    Joined:
    May 21, 2010
    Posts:
    247

    i used fixed time because it is realtime-accurate. Upon having low framerate, I have noticed there is a slight slowdown in deltaTime code, but not fixedDeltaTime. Thus the usage
     
  10. Doug_B

    Doug_B

    Joined:
    Jun 4, 2017
    Posts:
    1,596
    Changing the health value in fixed update will not update the screen any faster or slower.
     
  11. CodeSmile

    CodeSmile

    Joined:
    Apr 10, 2014
    Posts:
    5,533
    Code (CSharp):
    1. if (regenerateHealth != null)
    2.             StopCoroutine(regenerateHealth);
    can be replaced with

    Code (CSharp):
    1. StopCoroutine(regenerateHealth);
    If I remember correctly you can safely pass null to StopCoroutine().

    And to help understand the issue at hand, it's because every time you run StartCoroutine it will create and return a new instance of Coroutine, so you have to assign it to make StopCoroutine work like how Doug_B did it:

    Code (CSharp):
    1. m_regenHealth = StartCoroutine(RegenerateHealth());
    That was the issue. With the original code you were trying to stop an already stopped coroutine (the one that was initially assigned) while continuing to start new ones. In the example code for StopCoroutine they sort of hide that fact from you, but it's there:

    Code (CSharp):
    1.  
    2.  
    3.         Coroutine b = StartCoroutine(coroutineB());
    4.  
    5.         yield return new WaitForSeconds(2.0f);
    6.         Debug.Log("coroutineA() finished " + Time.time);
    7.  
    8.         // B() was expected to run for 10 seconds
    9.         // but was shut down here after 3.0f
    10.         StopCoroutine(b);
     
    Last edited: Aug 25, 2018
  12. nevaran

    nevaran

    Joined:
    May 21, 2010
    Posts:
    247

    The first part about the null check is false: I ran into an error, thats why I had added it xD

    As for overall, this is the only way to do it? No way of improving performance by making it not create a new object on each start I assume?
     
  13. Doug_B

    Doug_B

    Joined:
    Jun 4, 2017
    Posts:
    1,596
    No, it isn't. For example, you could have the 'regenerate' coroutine always running and set a trigger each time you want it to activate. But then you have the problem of running the coroutine every frame to check for the trigger condition.

    You could also create another script whose sole purpose is to constantly regenerate the health. Then activate / deactivate that script accordingly.

    But I doubt you will find any of these methods make any noticeable performance difference. :)
     
  14. nevaran

    nevaran

    Joined:
    May 21, 2010
    Posts:
    247

    Well both choices you have given will have a considerable performance hit, not improvement. But anyhow - the reason I went this path is because I also have 2 more stats that use the same code, just different parameters/variables and whatnot. I suppose I will stay with this code
     
  15. Doug_B

    Doug_B

    Joined:
    Jun 4, 2017
    Posts:
    1,596
    Just out of interest, what do you mean by 'considerable'?
     
  16. CodeSmile

    CodeSmile

    Joined:
    Apr 10, 2014
    Posts:
    5,533
    This is how enumerators work. The same thing (creating a new enumerator) happens when you do:

    Code (CSharp):
    1. foreach(var something in list) ...
    This is hardly a place to look for optimisations.
     
    Doug_B likes this.
  17. AndersMalmgren

    AndersMalmgren

    Joined:
    Aug 31, 2014
    Posts:
    5,358
    Some folks here would argue you should use a for loop ;)
     
  18. nevaran

    nevaran

    Joined:
    May 21, 2010
    Posts:
    247
    I would agree with a for instead foreach loop, also - The performance bonuses are big enough while the difference in the amount of code you need to write is insignificant.

    I do enjoy my code being more flexible, like a for loop, than having a slower code that does most stuff for you without much flexibility
     
  19. Doug_B

    Doug_B

    Joined:
    Jun 4, 2017
    Posts:
    1,596
    This may depend on the situation. For example, this analysis shows a
    foreach
    iterating faster over an
    int[]
    than a
    for
    and a
    while
    loop.
     
  20. AndersMalmgren

    AndersMalmgren

    Joined:
    Aug 31, 2014
    Posts:
    5,358
    No that is just plain wrong, its suboptimization