Search Unity

Yielding a nested IEnumerator waits a frame even on break

Discussion in 'Scripting' started by tweedie, Nov 5, 2019.

  1. tweedie

    tweedie

    Joined:
    Apr 24, 2013
    Posts:
    311
    Firstly - I understand why this behaviour happens, but as it is quite nuanced I do not think it is made especially clear in the documentation. I am not sure what, if anything, can be done to resolve it, but I want to start a discussion about whether this is something that could be dealt with.

    The problem is writing "yield return anything" will always wait a frame.

    I began this by needing to loop over a collection of objects in a coroutine, each of which have their own IEnumerator method that will optionally run some logic over the course of several frames, but if the condition is not met, will break out immediately with "yield break".

    Every write up I've since read about nested coroutines describes the following two means of nesting them as equivalent - this isn't strictly true.

    Code (CSharp):
    1. //First method of nesting
    2. yield return NestedRoutine();
    3.  
    4. //Second method of nesting
    5. IEnumerator nested = NestedRoutine();
    6. while(nested.MoveNext())
    7.     yield return nested.Current;
    8.  
    For the most part, these are indeed equivalent. They differ, however, in how they handle 'yield break'.

    If in the above code, NestedRoutine is implemented as follows:

    Code (CSharp):
    1. private IEnumerator NestedRoutine()
    2. {
    3.    if(!ShouldRun) yield break;
    4.  
    5.    //Run logic over some frames
    6. }
    Then the first means of nesting will wait a single frame before continuing the Coroutine, whereas the second will continue running the parent routine immediately.

    When you "yield return" anything other than a YieldInstruction - e.g. null, true, false, 1 etc - Unity will wait a frame before continuing the Coroutine. This remains true even if you return another IEnumerator that itself actually broke rather than returned null.

    This is subtly mentioned in the documentation:
    By wrapping the yield return statement within a while loop and checking MoveNext(), the second means of nesting works as expected, as when it reaches a break statement, MoveNext() will return false and skip the return entirely.

    This is a workaround, but becomes ugly very quickly - every time you need to yield another coroutine, you need to set a local variable and wrap its MoveNext() in a while loop before actually yielding it. This turns a single line of easy to read code into three, and moreover, three lines that can't be wrapped in a method, because as soon as you 'yield return' that method, you will wait a frame.

    The first, most commonly used means of nesting, results in any IEnumerator that could 'break' if some condition isn't met, consuming a frame. This isn't immediately apparent nor intuitive, because writing yield break infers different intent to yield return null, but this intent is lost in the parent loop, where they are both returned and thus evaluated as null.

    As such, my only proposition, and I'm unsure whether it's feasible, would be a YieldInstruction we can return that tells Unity to resume the parent coroutine immediately. Perhaps something like 'YieldResumeImmediate'. I am aware we can implement CustomYieldInstructions, but so far I have been unable to find a way of implementing one such that it never waits - at a minimum, it will always wait a single frame.

    Consider the following CustomYieldInstruction:

    Code (CSharp):
    1. public class ContinueImmediately : CustomYieldInstruction
    2. {
    3.     public override bool keepWaiting => false;
    4. }
    If you were to return this a few times within an IEnumerator and record the frameCount before and after, you will find each return statement waits a frame. With this being the case, I can't see how a CustomYieldInstruction would be able to continue immediately, unless there's something I need to override that I've been unable to find. If somebody knows how to implement a CustomYieldInstruction such that it will resume the parent method immediately, that would be fantastic. Any thoughts on the matter would be appreciated :) Thanks!
     
  2. Antistone

    Antistone

    Joined:
    Feb 22, 2014
    Posts:
    2,836
    I would imagine that Unity isn't looking at what value you yield-returned until the next time that a coroutine could potentially start up, and I imagine there's an argument from performance for doing it that way (adding those checks would slightly slow things down for all coroutine-users while benefiting only a tiny subset). Not sure how strong that argument is, but it probably exists.

    Writing 2 extra lines of boilerplate each time you do an unusual thing does not strike me as terribly onerous. It's kind of like the issue with value-type getters/setters that stops you from writing
    transform.position.x = 1;
    .

    You could probably reduce it to 1 extra line with some refactoring; conceptually, check "should run" before you start up the coroutine, rather than inside of it, so your code looks like:
    Code (CSharp):
    1. if (NestedRoutineShouldRun) yield return NestedRoutine();
     
  3. villevli

    villevli

    Joined:
    Jan 19, 2016
    Posts:
    87
    I got interested in how Unity's coroutines were implemented and decided to write my own implementation. This has most of the features that Unity's coroutines have and it does not have any frame delays when other things than null are yield returned. Unfortunately this does not support Unity's yield instructions that do not inherit from IEnumerator like WaitForSeconds or WaitForEndOfFrame.

    EDIT: I cleaned this up, added documentation and put it to github: https://github.com/villevli/UnitySimpleCoroutines
    I also got rid of the MyYieldInstruction as it seemed unnecessary and using Unity's CustomYieldInstruction or just IEnumerator works fine.

    Code (CSharp):
    1. using System.Collections;
    2. using System.Collections.Generic;
    3. using UnityEngine;
    4.  
    5. public class MyCoroutine
    6. {
    7.     private Stack<IEnumerator> ienumeratorStack = new Stack<IEnumerator>();
    8.     private MyYieldInstruction currentYieldInstruction = null; // in case a MyYieldInstruction has been yield returned
    9.     private MyCoroutine nestedCoroutine = null; // in case another MyCoroutine has been yield returned
    10.     private int yieldFrame = -1; // in case null has been yield returned
    11.  
    12.     public bool IsRunning()
    13.     {
    14.         if (yieldFrame == Time.frameCount)
    15.             return true;
    16.         if (ienumeratorStack.Count > 0)
    17.             return true;
    18.         if (currentYieldInstruction != null && currentYieldInstruction.keepWaiting)
    19.             return true;
    20.         if (nestedCoroutine != null && nestedCoroutine.IsRunning())
    21.             return true;
    22.         return false;
    23.     }
    24.  
    25.     public void Start(IEnumerator routine)
    26.     {
    27.         ienumeratorStack.Clear();
    28.         ienumeratorStack.Push(routine);
    29.         currentYieldInstruction = null;
    30.         nestedCoroutine = null;
    31.         yieldFrame = Time.frameCount - 1;
    32.  
    33.         Update();
    34.     }
    35.  
    36.     // Execute a single step of the coroutine. Unity equivalent is the MoveNext you see in coroutine stack traces
    37.     // Returns true if the coroutine should continue running
    38.     public bool Update()
    39.     {
    40.         // this ensures yield return null waits a frame in all situations
    41.         if (yieldFrame == Time.frameCount)
    42.             return true;
    43.  
    44.         // do not continue with the IEnumerators until the last returned yield instruction returns keepWaiting false
    45.         if (currentYieldInstruction != null && currentYieldInstruction.keepWaiting)
    46.             return true;
    47.         currentYieldInstruction = null;
    48.  
    49.         // do not continue with the IEnumerators until the last returned MyCoroutine has finished running
    50.         if (nestedCoroutine != null && nestedCoroutine.IsRunning())
    51.             return true;
    52.         nestedCoroutine = null;
    53.  
    54.         if (ienumeratorStack.Count > 0)
    55.         {
    56.             var ie = ienumeratorStack.Peek(); // the top of the stack is the currently executing IEnumerator
    57.             if (ie.MoveNext()) // execute IEnumerator until next yield (true) or break/end (false)
    58.             {
    59.                 // ie.Current is what was yield returned in the IEnumerator
    60.                 if (ie.Current is IEnumerator)
    61.                 {
    62.                     // move to execute a nested IEnumerator
    63.                     ienumeratorStack.Push(ie.Current as IEnumerator);
    64.                     return Update(); // nested IEnumerator execution is started without frame delay
    65.                 }
    66.                 if (ie.Current is MyYieldInstruction)
    67.                 {
    68.                     currentYieldInstruction = ie.Current as MyYieldInstruction;
    69.                     currentYieldInstruction.Reset();
    70.                 }
    71.                 if (ie.Current is MyCoroutine)
    72.                 {
    73.                     nestedCoroutine = ie.Current as MyCoroutine;
    74.                 }
    75.                 if (ie.Current == null)
    76.                 {
    77.                     // unity likely does this for "unsupported" yield returned objects as well
    78.                     yieldFrame = Time.frameCount;
    79.                 }
    80.                 // TODO: add logic for custom yieldable objects here
    81.             }
    82.             else
    83.             {
    84.                 ienumeratorStack.Pop();
    85.                 return Update(); // nested IEnumerator returns without frame delay
    86.             }
    87.             return true;
    88.         }
    89.         else
    90.         {
    91.             return false;
    92.         }
    93.     }
    94. }
    95.  
    96. // Start and run the coroutines in the player loop
    97. public class MyCoroutineRunner
    98. {
    99.     private List<MyCoroutine> runningCoroutines = new List<MyCoroutine>();
    100.  
    101.     // Call this as you would call MonoBehaviour.StartCoroutine
    102.     public MyCoroutine StartCoroutine(IEnumerator ie)
    103.     {
    104.         var myCoroutine = new MyCoroutine();
    105.         myCoroutine.Start(ie);
    106.         runningCoroutines.Add(myCoroutine);
    107.         return myCoroutine;
    108.     }
    109.  
    110.     // Call this each frame (MonoBehaviour.Update)
    111.     public void Update()
    112.     {
    113.         for (int i = runningCoroutines.Count - 1; i >= 0; i--)
    114.         {
    115.             if (!runningCoroutines[i].Update())
    116.                 runningCoroutines.RemoveAt(i);
    117.         }
    118.     }
    119. }
    120.  
    121. public abstract class MyYieldInstruction
    122. {
    123.     public abstract bool keepWaiting { get; }
    124.     public abstract void Reset();
    125. }
    126.  
    127. public class MyWaitForSeconds : MyYieldInstruction
    128. {
    129.     private float seconds;
    130.     private float waitCompleteTime;
    131.  
    132.     public MyWaitForSeconds(float seconds)
    133.     {
    134.         this.seconds = seconds;
    135.     }
    136.  
    137.     public override bool keepWaiting => Time.time < waitCompleteTime;
    138.  
    139.     public override void Reset()
    140.     {
    141.         waitCompleteTime = Time.time + seconds;
    142.     }
    143. }
    144.  
     
    Last edited: Sep 15, 2020
  4. tweedie

    tweedie

    Joined:
    Apr 24, 2013
    Posts:
    311

    I'm not sure it would slow it down. The current YieldInstructions allow a coroutine to resume its execution at various points later in the frame or in the next frame (for instance, end of frame, in fixed update etc). If it's able to delay until the end of the frame, I doubt there'd be any further expense in not delaying at all.

    I can see why 2 extra lines doesn't sound bad, but unlike the value setters, this also cannot be moved to an extension method. If you were to have a coroutine that yields even a few other processes, it can start to get hard to see the wood for the trees, as the important code is cluttered between while loops and the intent becomes less clear.

    Take for example a simple pattern where we might apply damage to an entity, that may need to execute some code before it takes damage, and optionally more after. If this sounds contrived, I assure you it is not at all uncommon in cases where entities can be effected by various "statuses" etc, which may or may not run code on certain events.

    Naively, you would probably write the following.

    Code (CSharp):
    1. private IEnumerator TakeDamage(DamageInfo info)
    2. {
    3.     yield return OnApplyingDamage();
    4.  
    5.     //Apply the damage
    6.  
    7.      yield return OnAppliedDamage();
    8. }

    But should the damageable not have any logic to run in either of those methods, each will have "eaten" a frame. To correct it, you would have to change it as follows:

    Code (CSharp):
    1. private IEnumerator TakeDamage()
    2. {
    3.     IEnumerator e = OnApplyingDamage();
    4.     while(e.MoveNext())
    5.         yield return e.Current;
    6.  
    7.     //Apply the damage
    8.  
    9.      e = OnAppliedDamage();
    10.      while(e.MoveNext())
    11.             yield return e.Current;
    12. }

    In the first example, I can immediately see the methods that are being called and that we are yielding them. In the second, that is much less clear.

    I appreciate a single frame may sound trivial, but imagine the call to TakeDamage() is called as follows:

    Code (CSharp):
    1. foreach(IDamageable in damageables)
    2. {
    3.     yield return damageable.TakeDamage(damageInfo);
    4. }
    Let's say you're dealing damage to 12 damageables, each of which may or may not run code in either OnApplyingDamage or OnAppliedDamage. Let's say 50% of these calls break out early. This is already bad; 6 frames will have been consumed for no real reason - plenty enough to be felt by the player if those frames delay visual feedback for the code that does run.

    You might suggest in this case, if we need to iterate over several elements and care about losing thsoe frames, we should change the implementation. However that is only because of this strange behaviour.

    The more I consider this, the more I consider it a bug, or at least an implementation detail that we need a means of working around more cleanly. I would suspect anybody writing yield break is expecting their method to break early and allow any parent loops to continue immediately; not that they're actually consuming a frame.

    Considering, I would suspect, most people would see yield break as analagous to return in a regular void method, i.e immediately breaking with no frame delay, if this cannot be differentiated (in the value of IEnumerator.Current) from null, I think we need a dedicated type we can return to indicate that instead.

    Checking the conditional outside of the method first seems like it might reduce the code, but it is both potentially quite a conceptual upheaval and probably won't reduce code, as every IEnumerator in an object now needs to have a companion method to indicate whether it can run.

    The bottom line of this is anybody who nests a coroutine that has an if statement which at some point in the method breaks early is (almost certainly unknowingly) losing a frame.
     
    Partybwal likes this.
  5. tweedie

    tweedie

    Joined:
    Apr 24, 2013
    Posts:
    311

    This looks interesting! I could probably do fine without the Unity YieldInstructions, the only one I ever tend to use is WaitForSeconds, and even that get's cached in our own class due to the alleged cost of creating new ones on the fly, so it wouldn't take much more to just switch to a completely custom system.
     
  6. Antistone

    Antistone

    Joined:
    Feb 22, 2014
    Posts:
    2,836
    Say...you're going to all this trouble to try to avoid an extra delay of 1 frame in the case where the nested coroutine ends early. Are you 100% sure that you don't also have an extra delay of 1 frame when the nested coroutine doesn't end early?

    I could totally imagine Unity doing something conceptually like:
    Code (CSharp):
    1. foreach (var coroutine in coroutines)
    2. {
    3.     if (coroutine is not waiting) coroutine.continue();
    4. }
    But if it's doing that, and you happen to evaluate the "parent" coroutine's condition before the nested coroutine's condition, then the parent will have already forfeited its opportunity to run in the current frame by the time the child finishes, so you might get a delay anyway. Have you checked?


    It also occurs to me that you could probably take advantage of the fact that coroutines are technically enumerators to run a nested coroutine like this:
    Code (CSharp):
    1. foreach (var result in NestedCoroutine())
    2. {
    3.     yield return result;
    4. }
    That should solve both problems--you'll continue running immediately on the frame that the NestedCoroutine ends, regardless of whether that's now or later, and regardless of how Unity tests the conditions.



    Needing to run code before and after taking damage doesn't sound contrived at all.

    But needing to wait for coroutines before and after taking damage sounds pretty contrived. Typical status effects could modify the damage taken or trigger new effects in reaction to it, but "wait a second before applying that damage" isn't a status effect that I can recall seeing before.

    And waiting for target #1 to react to the damage before you apply the same damage packet to target #2 sounds even more contrived. What sort of game are you making where a multi-target attack allows the first target to delay damage to the second target? But not vice-versa?

    In my experience coroutines are generally not very useful for gameplay-related stuff because gameplay-related stuff that takes place over a period of time generally wants to be constantly reacting to other stuff that's happening in parallel, and coroutines are just not a very convenient model for that sort of interactivity.

    Using coroutines for animations makes more sense because animations generally aren't reactive in this way. But by the same token, animations probably don't need to call stuff like BeforeTakingDamage() because that was already handled at the gameplay layer.
     
  7. tweedie

    tweedie

    Joined:
    Apr 24, 2013
    Posts:
    311
    Heh, I appreciate it may sound like a waste of effort, but unfortunately there are enough opportunities for these delays to accumulate that whilst it doesn't hang the game, it delays some logic that triggers visual feedback such that a player would notice it. In my case I can avoid it with the while loop method I initially outlined, but it started me on a curiosity rabbit hole and now I'm beginning to question anywhere I've ever yielded a coroutine that can return early.

    Yes that will also consume a frame, because a coroutine that contains some form of yield statement earlier in its body doesn't need to explicitly declare yield break afterward - it is implied - similarly to the use of return in a regular void method. However, no frames are consumed if every coroutine is yielded via the while(e.MoveNext()) wrapper, which is the significant difference between the implementations.


    This looks like it would be a good solution, but I'm unsure exactly how it would work. Neither a plain IEnumerator nor an actual Coroutine can be iterated in a foreach like that. Coroutines inherit YieldInstruction, which may be handled via iterators internally but I don't know how I would access it. Am I missing something?


    I appreciate that might seem contrived, alas it's not. Ours is probably a more uncommon case - we're working on a deckbuilder where several characters can each have status effects, with wildly varying behaviour.

    Take for instance a scenario where a character has a status effect that draws the player a card whenever that character takes damage. Then another character gets buffed whenever a card is drawn. If the first character is part of a multi-target attack, you may want the card-drawing effect to trigger before the next target is dealt damage. This likely means waiting for the card to visually finish the drawing animation, and then in turn trigger the second character's status afterwards. (I won't get into a discussion around the specifics of this order, queueing such scenarious, or how you might handle a recursive relationship, as these are unique design decisions we've discussed internally at length that are somewhat tangential here).

    Furthermore, the code needs to be paced across time for the player to be able to follow the chain of events.This could probably be handled with callbacks, but the code is far easier to follow if it's executed within coroutines - (or, I suppose for the sake of specificity, IEnumerators, as I only tend to have a single coroutine at the root of the chain) - as they clearly communicate a chronological sequence in a very parsable way that doesn't jump all over the project.

    We also have tooling in place that allows the construction of these graphs by a non-coder, which would make callbacks harder still.

    I would normally agree, however in this case the gameplay is quite tightly bound to a "boardgamey" rhythm where the order of events is crucial, and isn't executing in parallel with other systems.

    The above aside, I don't want to get too bogged down in the specifics of our project - my initial post was much more generally about whether nested coroutines that break early should be expected to consume a frame, regardless of the consequence of a single frame in most projects. It seems like a non-trivial detail that isn't very clearly documented to me, and one you would likely choose to avoid should there be a trivial way of doing so.
     
  8. Antistone

    Antistone

    Joined:
    Feb 22, 2014
    Posts:
    2,836
    I don't normally iterate IEnumerators directly, but there's obviously a way to do it, and you could easily write a helper function that lets you treat it as an IEnumerable if you prefer that syntax.

    I'm the lead programmer for 2 published digital board games made in Unity. Neither uses coroutines in this way.

    Sounds like you have no separation between between the model and the view, and therefore find yourself encoding your animation rules into your gameplay logic. Seems like a mistake to me, but hey, it's your project. Best of luck.
     
    tweedie likes this.
  9. tweedie

    tweedie

    Joined:
    Apr 24, 2013
    Posts:
    311
    That reads a little terse, so if I've in any way offended you I apologise. I would like to defend that we do have that separation, but that defence would stray away from the purpose of this thread. I don't wish to dismiss your advice though, so will send you a PM you can freely ignore if you wish :)

    EDIT: Ah, it appears I can't. That's a shame, I'm all ears for hearing different, better approaches, I just don't think this thread is the venue for it. Have a nice day anyway
     
  10. AndersMalmgren

    AndersMalmgren

    Joined:
    Aug 31, 2014
    Posts:
    5,358
    I do not agree with that, we use it all the time for gameplay and similar. For example here I drive the finger IK using user input

     
    tweedie likes this.
  11. Kushulain

    Kushulain

    Joined:
    Sep 20, 2012
    Posts:
    19
    Very nice work villevli !

    I encountered this problem lately, unability to nest IEnumerator without having them to "yield frames".

    Instead of having to deal with "runner" and its manual update(), what do you think of making an extension which will create a Unity coroutine which will execute the Update() automatically.

    So you only have to do :
    Code (CSharp):
    1. myMonoBehaviourClass.StartSCoroutine(mySCoroutine);
    Code (CSharp):
    1. public static class SCoroutineExtensionClass
    2. {
    3.     static IEnumerator SCoroutineWrapper(IEnumerator ieCoroutine)
    4.     {
    5.         SimpleCoroutines.SCoroutineRunner runner = new SimpleCoroutines.SCoroutineRunner();
    6.         runner.StartCoroutine(ieCoroutine);
    7.         while (runner.Update())
    8.         {
    9.             yield return null;
    10.         }
    11.     }
    12.  
    13.     public static void StartSCoroutine(this MonoBehaviour comp, IEnumerator ie)
    14.     {
    15.         comp.StartCoroutine(SCoroutineWrapper(ie));
    16.     }
    17. }
    To stop the Unity Coroutine when the SCoroutine is done :

    Code (CSharp):
    1. public class SCoroutineRunner
    2. {
    3.     [...]
    4.  
    5.     public bool Update()
    6.     {
    7.         for (int i = runningCoroutines.Count - 1; i >= 0; i--)
    8.         {
    9.             if (!runningCoroutines[i].Update())
    10.                 runningCoroutines.RemoveAt(i);
    11.         }
    12.  
    13.         return runningCoroutines.Count > 0;
    14.     }
    15. }
    Seems to work as expected, but I need to test it more thoroughly.

    In the case anyone tries and what to call it from inside your monobehaviour class, you need to use "this" keyword :
    Code (CSharp):
    1. this.StartSCoroutine(mySCoroutine);
    Maybe it could be something to add to your repos !
    Thank you !

     
  12. doctorpangloss

    doctorpangloss

    Joined:
    Feb 20, 2013
    Posts:
    270
    Just to help people out with the problem that this solves, since there's a ton of noise in this thread.

    You are experiencing an issue of the form
    1. You call a coroutine SomeCoroutine from another coroutine using yield return SomeCoroutine() or yield return StartCoroutine(SomeCoroutine()).
    2. Observe that even if SomeCoroutine does nothing (such as by yield break 'ing), Unity will resume execution next frame. It should instead continue executing "in the same frame" i.e. without interruption.

    Code (CSharp):
    1. IEnumerator SomeCoroutine() {
    2.   yield break;
    3. }
    If you want to call SomeCoroutine from another coroutine and not lose a frame, replace:

    Code (CSharp):
    1.  
    2. yield return SomeCoroutine();
    3.  
    with

    Code (CSharp):
    1. var childCoroutine = SomeCoroutine();
    2. while (childCoroutine.MoveNext()) {
    3.   yield return childCoroutine.Current;
    4. }
    wherever you are doing (instead of) yield return SomeCoroutine();
     
    joshuaFS and PeachyPixels like this.