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

Question Problem with WaitUntil and GetKeyDown

Discussion in 'Scripting' started by Stevens-R-Miller, Mar 29, 2023.

  1. Stevens-R-Miller

    Stevens-R-Miller

    Joined:
    Oct 20, 2017
    Posts:
    676
    I want my coroutine to react to key presses, so I have it yield return a WaitUntil, with the delegate returning Input.GetKeyDown for the key I am interested in. If my coroutine ever yields any other way before yielding the WaitUntil again, things work fine. But if I have my coroutine run and then yield the same WaitUntil, it resumes execution after the yield on the next update, without my pressing the key again, even though Input.GetKeyDown returns false for that key. If I follow that with a third yield return of that same WaitUntil, execution does not resume.

    I would expect either the WaitUntil would always resume execution immediately after the first resumption (as Input.GetKeyDown would still be true in that update for that key), or it wouldn't (on the assumption that you always want to wait after a yield until the next update to check the WaitUntil's delegate). But I can't figure out why it resumes on the next update, yet also returns false for GetKeyDown on the same key.

    Can anyone make sense of this? Is it a bug? Am I just missing something? Thanks!

    Code (CSharp):
    1. using System.Collections;
    2. using UnityEngine;
    3.  
    4. public class CoBug : MonoBehaviour
    5. {
    6.     int updates = 0;
    7.  
    8.     void Start()
    9.     {
    10.         StartCoroutine(KeyReport());
    11.     }
    12.  
    13.     private void Update()
    14.     {
    15.         updates = updates + 1;
    16.     }
    17.  
    18.     IEnumerator KeyReport()
    19.     {
    20.         WaitUntil wait = new WaitUntil(() => Input.GetKeyDown(KeyCode.UpArrow));
    21.  
    22.         Debug.Log($"Starting at {Time.time} after {updates} updates.");
    23.  
    24.         while (true)
    25.         {
    26.             Debug.Log($"Yielding at {Time.time} after {updates} updates.");
    27.  
    28.             yield return wait;
    29.  
    30.             Debug.Log($"Returned {Input.GetKeyDown(KeyCode.UpArrow)} at {Time.time} after {updates} updates.");
    31.  
    32.             yield return wait;
    33.  
    34.             Debug.Log($"Again, but {Input.GetKeyDown(KeyCode.UpArrow)} at {Time.time} after {updates} updates.");
    35.  
    36.             yield return wait;
    37.  
    38.             Debug.Log($"Yet again, but {Input.GetKeyDown(KeyCode.UpArrow)} at {Time.time} after {updates} updates.");
    39.         }
    40.     }
    41. }
    42.  
    Here's the output when I press the UpArrow once:

    upload_2023-3-29_15-6-52.png
     
    Bunny83 likes this.
  2. Bunny83

    Bunny83

    Joined:
    Oct 18, 2010
    Posts:
    3,913
    Well, the issue here is the way Unity handles nested IEnumerators. As you may know WaitUntil is simply a CustomYieldInstruction which is a very basic implementation of a IEnumerator. MoveNext is directly linked to your predicate. When a coroutine is waiting on a nested IEnumerator it calls MoveNext to determine if it should still wait or not. As soon as MoveNext returns false (when your predicate is true) the nested IEnumerator is dropped and the outer coroutine is resumed the same frame as the key down happened. Since you yield the same IEnumerator again, MoveNext would still return false since we're still in the same frame.

    Now here comes the special part: When you actually yield your IEnumerator to the scheduler, it of course calls MoveNext on it immediately so Unity knows that iterator is already expired. However when you yield an expired IEnumerator Unity explicitly waits one frame. I'm not sure if that's just a fragment of the implementation or a deliberate choice to avoid potential infinite loops. If yielding an expired IEnumerator would return immediately, your coroutine would rush through all your yields within the same frame and Unity would crash / hang.

    You can test this by doing:

    Code (CSharp):
    1.         yield return new WaitForSeconds(0.1f);
    2.         Debug.Log($"Start {Time.frameCount} after {updates} updates.");
    3.         yield return new Test();
    4.         Debug.Log($"Step {Time.frameCount} after {updates} updates.");
    5.         yield return new Test();
    6.         Debug.Log($"Step {Time.frameCount} after {updates} updates.");
    7.         yield return new Test();
    8.         Debug.Log($"Step {Time.frameCount} after {updates} updates.");
    9.         yield return new Test();
    10.         Debug.Log($"Step {Time.frameCount} after {updates} updates.");
    11.  
    with this Test class:
    Code (CSharp):
    1.     class Test : IEnumerator
    2.     {
    3.         public object Current => new WaitForSeconds(10f);
    4.  
    5.         public bool MoveNext()
    6.         {
    7.             return false;
    8.         }
    9.         public void Reset() { }
    10.     }
    11.  
    This is essentially a dead enumerator. However as you will see, Unity will always wait one frame at each yield. Note that it waits one frame, not 10 seconds. That's because when MoveNext returns false, you must not read the Current property since the iterator is dead and doesn't have a new value. So Unity explicitly waits one frame on its own.

    So the scheduler code probably looks something like this:

    Code (CSharp):
    1.  
    2. if (currentIterator.MoveNext())
    3. {
    4.     if (currentIterator.Current is IEnumerator sub)
    5.     {
    6.         if (sub.MoveNext())
    7.         {
    8.             stack.Push(currentIterator);
    9.             currentIterator = sub;
    10.             DetermineNextScheduleBasedOnYield(sub.Current);
    11.         }
    12.         else
    13.         {
    14.             DetermineNextScheduleBasedOnYield(null);
    15.         }
    16.     }
    17.     else
    18.     {
    19.         DetermineNextScheduleBasedOnYield(currentIterator.Current);
    20.     }
    21. }
    22. else
    23. {
    24.     if (stack.Count > 0)
    25.         currentIterator = stack.Pop();
    26.     else
    27.         TerminateCoroutine();
    28. }
    29.  
    Of course this is just some pseudo code how they may implement nested ienumerators. Though is is roughly the behaviour. Note when the last iterator is popped from the stack, the outer one is immediately resumed. So this is the case when the nested iterator is the active one and expires. So the coroutine switches back to the next one up the hierarchy and resumes that. The only exception is when you give it an already expired enumerator. In that case it doesn't even make it the current IEnumerator but simply waits one frame and resumes the outer coroutine after one frame.

    As I said, if Unity would not do that, your code would make Unity hang in an infinite loop since each yield would receive an expired IEnumerator and immediately resume without any interruption.

    You may want to use a custom IEnumerator like this:
    Code (CSharp):
    1.  
    2.     public class WaitForKeyDown : IEnumerator
    3.     {
    4.         public object Current => null;
    5.         private KeyCode m_Key;
    6.         private bool m_FirstFrame = true;
    7.         public WaitForKeyDown(KeyCode aKey)
    8.         {
    9.             m_Key = aKey;
    10.         }
    11.         public bool MoveNext()
    12.         {
    13.             if (m_FirstFrame)
    14.             {
    15.                 m_FirstFrame = false;
    16.                 return true;
    17.             }
    18.             if (!Input.GetKeyDown(m_Key))
    19.                 return true;
    20.             m_FirstFrame = true;
    21.             return false;
    22.         }
    23.         public void Reset() {  }
    24.     }
    25.  
    This one will always wait one frame before it starts checking for the key down. So it's impossible to fall through one as it enforces a one frame delay.

    Of course another solution is to just add a
    yield return null;
    before each of your WaitUntil call to ensure you wait one frame so GetKeyDown is false again before waiting for the next keypress.
     
    Kurt-Dekker and Nefisto like this.
  3. Bunny83

    Bunny83

    Joined:
    Oct 18, 2010
    Posts:
    3,913
    I just realised I had a mistake in my WaitForKeyDown class. I just fixed it. Not sure if you already saw the broken version :). In order to be able to reuse the instance we had to reset the m_FirstFrame variable back to true. I also had the condition reversed, so it was kinda broken ^^. Now it should work.
     
  4. Stevens-R-Miller

    Stevens-R-Miller

    Joined:
    Oct 20, 2017
    Posts:
    676
    Great explanation, thanks! I wish they would document this behavior a little better. It took me quite some time to reduce the problem the form I posted above. I do get that they want to avoid infinite loops, but that's a problem Unity has in other ways already. As far as I know, if my game code goes infinite, there is no way to exit play mode. You have to kill the process itself (and lose your unsaved work). Forcing the delay of an unnecessary update (rather than returning at once) might be the safer play, but it doesn't solve the problem more generally, and it does lead to the apparently inconsistent results from GetKeyDown that we're seeing here.

    I did notice that adding a(n otherwise superfluous)
    yield return null
    solved the problem, but that's yet another unneeded update delay.

    Much appreciated.
     
  5. Bunny83

    Bunny83

    Joined:
    Oct 18, 2010
    Posts:
    3,913
    Well, actually it is needed. That's the actual issue in your code. When the key down happens, the nested IEnumerator returns immediately. So yielding the same one again in the same frame is what caused the issue because the key down state is still true so the newly yielded IEnumerator is already expired. As I said, if they didn't had this one frame yield, your example would result in an infinite loop. So the additional yield to wait one frame is required since we have to wait at least one frame so that the GetKeyDown state is back to false.

    Conceptionally instead of waiting one frame, one could wait for GetKeyDown to be false again before going further after a key down was detected. However since the key down state is only true for one frame that would be unneccessary complexity:
    Code (CSharp):
    1.  
    2.     IEnumerator KeyReport()
    3.     {
    4.         WaitUntil waitDown = new WaitUntil(() => Input.GetKeyDown(KeyCode.UpArrow));
    5.         WaitUntil waitUp = new WaitUntil(() => !Input.GetKeyDown(KeyCode.UpArrow));
    6.         Debug.Log($"Starting at {Time.time} after {updates} updates.");
    7.         while (true)
    8.         {
    9.             Debug.Log($"Yielding at {Time.time} after {updates} updates.");
    10.             yield return waitDown;
    11.             Debug.Log($"Returned {Input.GetKeyDown(KeyCode.UpArrow)} at {Time.time} after {updates} updates.");
    12.             yield return waitUp;
    13.             yield return waitDown;
    14.             Debug.Log($"Again, but {Input.GetKeyDown(KeyCode.UpArrow)} at {Time.time} after {updates} updates.");
    15.             yield return waitUp;
    16.             yield return waitDown;
    17.             Debug.Log($"Yet again, but {Input.GetKeyDown(KeyCode.UpArrow)} at {Time.time} after {updates} updates.");
    18.             yield return waitUp;
    19.         }
    20.     }
    Just using a
    yield return null;
    if of course much simpler ^^

    Note that in some sense using WaitUntil the way you used it is essentially the same as just doing

    Code (CSharp):
    1. while(!Input.GetKeyDown(KeyCode.UpArrow))
    2.     yield return null;
    This actually has way less overhead and doesn't have that implicit yield. However here the same thing is true. If your code looked like this:

    Code (CSharp):
    1. IEnumerator KeyReport()
    2.     {
    3.         Debug.Log($"Starting at {Time.time} after {updates} updates.");
    4.         while (true)
    5.         {
    6.             Debug.Log($"Yielding at {Time.time} after {updates} updates.");
    7.             while(!Input.GetKeyDown(KeyCode.UpArrow))
    8.                 yield return null;
    9.             Debug.Log($"Returned {Input.GetKeyDown(KeyCode.UpArrow)} at {Time.time} after {updates} updates.");
    10.             while(!Input.GetKeyDown(KeyCode.UpArrow))
    11.                 yield return null;
    12.             Debug.Log($"Again, but {Input.GetKeyDown(KeyCode.UpArrow)} at {Time.time} after {updates} updates.");
    13.             while(!Input.GetKeyDown(KeyCode.UpArrow))
    14.                 yield return null;
    15.             Debug.Log($"Yet again, but {Input.GetKeyDown(KeyCode.UpArrow)} at {Time.time} after {updates} updates.");
    16.         }
    17.     }
    it's kinda obvious that when you press the up arrow that you created an infinite loop. So you NEED an additional one frame delay in between 2 key down checks. There's no way around that :)
     
  6. Stevens-R-Miller

    Stevens-R-Miller

    Joined:
    Oct 20, 2017
    Posts:
    676
    Does it? I would expect the coroutine manager (or whatever Unity calls it) to be doing much the same thing, but without re-entering and re-yielding from the coroutine itself. If that's not the case, what real advantage is there to using WaitUntil instead of the tight while/yield as you've shown?
     
  7. Stevens-R-Miller

    Stevens-R-Miller

    Joined:
    Oct 20, 2017
    Posts:
    676
    I read your explanation more closely, to be sure I understand it. I believe I do, which has me wondering if Unity's decision to force the extra update was their best option. It's clear that yielding back an expired IEnumerator with a WaitUntil with its condition already met could start an infinite loop. But why not put the burden of coping with that on me? In the code I've got (not listed here) there are always several other yields before my next yield wait, so the condition is not always met on the next yield wait (it might be, but that would only be if the key had gone down again, which would be fine). As you point out, if I really did want to run some code once and only once each time a key went down, I could either explicitly yield with WaitUntil(<that key goes up>), or just a yield return null. Except for the case where the only yielding I'm doing is related to keys going up or down, this approach would get me out of the necessary additional update Unity uses now (that's why I called it "unnecessary;" I see from your analysis why it is necessary, but it wouldn't be if Unity used the approach I'm describing here, would it?), albeit at an increased risk of an infinite loop.

    Again, thanks for this great break-down. Really kind of you to lay it out in such detail.
     
  8. Bunny83

    Bunny83

    Joined:
    Oct 18, 2010
    Posts:
    3,913
    Not much to be honest :) Since it's a class it will allocate memory. The predicate / delegate also requires an allocation. The execution has several method calls (MoveNext --> keepWaiting property --> delegate). The only "advantage" would be that the yield is a single line of code.

    That yield certainly is not necessary. However I think it's simply their way of handling such an error case (essentially starting a dead sub coroutine).

    I actually implemented coroutine schedulers myself in different contexts. For example in SpaceEnginieers which has ingame C# scripting (which is awesome ^^). I essentially build my own very simple coroutine scheduler but it also allowed nested IEnumerators. I also didn't inject an artificial yield. This allows to call multiple even deeper nested coroutines which could all finish synchronously when they don't need to wait. But of course you have to care about potential infinite loops :) I also love coroutines in lua. They are are much more powerful since a yield is actually a method call and you can pass multiple values back to the caller. But the nice thing is, when you resume a coroutine, the caller can actually pass multiple arguments to the resume method which get returned by the yield inside the coroutine. So you have a two way communication between caller and coroutine. Also you can even yield from a normal sub routine. Coroutines work more like threads and when you yield you essentially suspend the execution and preserve the whole callstack. Though this is kinda off topic now ^^.

    I'm still working on a computercraft implementation in Unity. CC is a minecraft mod that uses lua for ingame scripting. I used Moonsharp which is a pure C# implementation of lua, so it even works in a WebGL build. However a big problem was the single thread restriction and what happens when a script causes an infinite loop. CC handles the whole execution through coroutines. The yield method essentially is the message pump. So a lua script can yield and when the game has an event it just resumes the coroutine and passes the event along. The yield on the other hand can return a filter string, so the game can selectively ignore events.

    Moonsharp already had an instruction limiting mechanism built in that causes a special yield value to be returned that indicates that the coroutine was suspended because it reached its max instruction count. Though there was an issue that this didn't propergate correctly up the chain when you use nested coroutines. So I actually patched the resume method, looked for the special yield value and simply passed it up the hierarchy. So a very long process that would actually block the main thread, could be split up in chunks automatically. This was the magic patch:
    Code (CSharp):
    1.         var newResume = script.DoString(@"
    2.        local oldResume = coroutine.resume
    3.        local function newResume(co, ...)
    4.           local t,args = nil, {...}
    5.           if #args == 0 then
    6.               t = {oldResume(co)}
    7.           else
    8.               t = {oldResume(co, ...)}
    9.           end
    10.           while t[1] and tostring(t[2]) == ""(YieldRequest -- INTERNAL!)"" do
    11.              coroutine.yield(t[2])
    12.              t = {coroutine.resume(co)}
    13.           end
    14.           return unpack(t)
    15.        end
    16.        return newResume");
    17.  
    So the patch was actually written in lua ^^. So usually the caller would call resume to call / continue a coroutine, but it expects the resume method to return certain values. So that special interrupt yield value is an issue. So what this does is to call the original resume method and when it returns that special yield it would spin in that while loop and pass that special yield up to the next coroutine. This goes up to the top where it hits my C# entry point. All I do in C# is remembering that this coroutine was interrupted and resume it the next frame.

    It's completely wild but it actually works ^^. So I get somewhat preemptive multitasking in a single thread environment :).
     
  9. Stevens-R-Miller

    Stevens-R-Miller

    Joined:
    Oct 20, 2017
    Posts:
    676
    That actually is pretty cool. I find Lua a bit annoying, but maybe I need more time with it.

    Interesting how much one can do with this paradigm that resembles multi-threaded code, but isn't. I actually have to warn my students who are adept at concurrency not to worry about things like race conditions, as they can't happen in single-threaded coroutines. But coroutines feel so much like multi-threaded code that some of them get nervous and ask about locking and so forth. One asked me if that meant the code from the coroutine's entry point to the first yield could be considered "atomic." I had a tough time answering that because, in the sense that it would all execute before any more code outside the coroutine would execute, yes it is. But, in the sense that it could not be pre-empted so another thread could run, no it isn't. But that only comes up in a truly multi-threaded context, which coroutines are not, except that they could be since you could be running multi-threaded code and coroutines at the same time.

    So, it wasn't as easy to answer as I would have liked.