Search Unity

  1. Welcome to the Unity Forums! Please take the time to read our Code of Conduct to familiarize yourself with the forum rules and how to post constructively.
  2. Have a look at our Games Focus blog post series which will show what Unity is doing for all game developers – now, next year, and in the future.
    Dismiss Notice

Coroutine not completing loops

Discussion in 'Scripting' started by F21H43, Jan 15, 2022.

  1. F21H43

    F21H43

    Joined:
    Feb 3, 2017
    Posts:
    4
    I've been working on a grid system and I'm trying to put my movement into a coroutine so that my movement is animated when moving from tile to tile, but when I have yield return in either of the loops only the last movement happens. The game allows you to give to multiple moves that will happen sequentially, and it logs the separate instances as occurring but doesn't move into the correct tile, always moving as if it moved from it's original position to the tile one move in the last direction it was told to move in (so if you tell it to move up 3 tiles then right 2 tiles, it will only move 1 tile to the right). The code puts the unit in the correct position if I have no yield return lines within either loop.
    Code (CSharp):
    1.     public void MoveUnit(Vector2 dir, int index)
    2.     {
    3.         MenuManager.Instance.LogEvent(unitName + " travels " + directionToString(dir) + " " + speeds[index] + " paces.", this);
    4.         StartCoroutine(LerpUnit(dir, index));  
    5.     }
    6.  
    7.     //dir is a vector 2 with a magntue of 1 in one of the 4 cardinal directions
    8.     //speeds[index] is an int that says how many tiles we want to move in this current direction
    9.     IEnumerator LerpUnit(Vector2 dir, int index)
    10.     {
    11.         for (int s = 0; s < speeds[index]; s++)
    12.         {
    13.             Debug.Log(unitName + " s: " + s + " dir " + dir.ToString());
    14.             Vector2 curPos = occupiedTile.GetPosition();
    15.             Vector2 newPos = new Vector2(curPos.x + dir.x, curPos.y + dir.y);
    16.             float elapsedTime = 0f;
    17.  
    18.             Tile newTile = GridManager.Instance.GetTileAtPosition(newPos);
    19.             Debug.Log(unitName + " tries to move into " + newTile.GetPosition().ToString());
    20.             if (newTile != null)
    21.             {
    22.                 while (elapsedTime < moveAnimTime)
    23.                 {
    24.                     if (!newTile.walkable)
    25.                     {
    26.                         break;
    27.                     }
    28.                     transform.position = Vector2.Lerp(curPos, newPos, (elapsedTime / moveAnimTime));
    29.                     elapsedTime += Time.deltaTime;
    30.                     yield return null;
    31.                 }
    32.                 //this funtion sets this units position to the tile
    33.                 newTile.SetUnit(this);
    34.             }
    35.         }
    36.         yield return null;
    37.     }
     
  2. Vadimskyi

    Vadimskyi

    Joined:
    Jan 13, 2020
    Posts:
    12
    The problem is that your coroutines stack on top of each other. So the last coroutine you start will override previous inputs.
    You can manage this by starting coroutine one time and continuously poll user inputs as they arrive, like this:

    Code (CSharp):
    1. {
    2.     //process user input while _pollUserInput variable is true:
    3.     private bool _pollUserInput;
    4.     //store each user input inside queue:
    5.     private Queue<PlayerInput> _playerInputs = new Queue<PlayerInput>();
    6.  
    7.     //starting coroutine
    8.     private void Start(){
    9.         _pollUserInput = true;
    10.         StartCoroutine(LerpUnit());
    11.     }
    12.  
    13.     public void MoveUnit(Vector2 dir, int index)
    14.     {
    15.         MenuManager.Instance.LogEvent(unitName + " travels " + directionToString(dir) + " " + speeds[index] + " paces.", this);
    16.         //add user input to queue:
    17.         _playerInputs.Enqueue(new PlayerInput{Direction = dir, Index = index});
    18.     }
    19.  
    20.     private IEnumerator LerpUnit()
    21.     {
    22.         while(_pollUserInput){
    23.             while(_playerInputs.Count > 0){
    24.                 var input = _playerInputs.Dequeue();
    25.                 var dir = input.Direction;
    26.                 var index = input.Index;
    27.                 //your lerp logic:
    28.                 for (int s = 0; s < speeds[index]; s++)
    29.                 {
    30.                     Debug.Log(unitName + " s: " + s + " dir " + dir.ToString());
    31.                     Vector2 curPos = occupiedTile.GetPosition();
    32.                     Vector2 newPos = new Vector2(curPos.x + dir.x, curPos.y + dir.y);
    33.                     float elapsedTime = 0f;
    34.      
    35.                     Tile newTile = GridManager.Instance.GetTileAtPosition(newPos);
    36.                     Debug.Log(unitName + " tries to move into " + newTile.GetPosition().ToString());
    37.                     if (newTile != null)
    38.                     {
    39.                         while (elapsedTime < moveAnimTime)
    40.                         {
    41.                             if (!newTile.walkable)
    42.                             {
    43.                                 break;
    44.                             }
    45.                             transform.position = Vector2.Lerp(curPos, newPos, (elapsedTime / moveAnimTime));
    46.                             elapsedTime += Time.deltaTime;
    47.                             yield return null;
    48.                         }
    49.                         //this funtion sets this units position to the tile
    50.                         newTile.SetUnit(this);
    51.                     }
    52.                 }
    53.                 yield return null;
    54.             }
    55.             yield return null;
    56.         }
    57.     }
    58.  
    59.     class PlayerInput{
    60.         public Vector2 Direction;
    61.         public int Index;
    62.     }
    63. }
     
    Bunny83 likes this.
  3. Bunny83

    Bunny83

    Joined:
    Oct 18, 2010
    Posts:
    2,403
    You can actually "de-indent" your coroutine by doing an "early skip" (like an early exit) so you can get rid of some of the nesting and the code is easier to read:

    Code (CSharp):
    1. private IEnumerator LerpUnit()
    2.     {
    3.         while(_pollUserInput)
    4.         {
    5.             while(_playerInputs.Count == 0)
    6.                 yield return null;
    7.             var input = _playerInputs.Dequeue();
    8.             // [ ... ]
    9.  
    So instead of having all those nested while loops, you can simply wait on the spot until a queued input shows up. The outer loop will essentially loop through all the queued inputs until the queue is empty and the coroutine will wait in that nested loop until a new input is available.

    Though I've seen a couple of other things which doesn't seem quite right. First of all the
    if (!newTile.walkable)
    condition really should be checked before you decide to move onto that tile. Especially when you break out of the movement loop, you set the new tile to your unit anyways, even though you stopped the movement. That seems really odd. I think that condition should be part of the
    if (newTile != null)
    if statement.

    The next thing which seems a bit mysterious is where "occupiedTile" comes from. It's not changed inside the coroutine so the behaviour may not be what you expect. I think after completing one movement step, the occupiedTile should probably be set to the newTile. Though since we don't know where that variable is defined or set we can't really argue about its usage. It's also not quite clear what behaviour you'd expect when the target tile is not walkable. Just continuing with the next "speed" in the same direction doesn't make much sense. So I guess there's some logic work and rethinking necessary :)
     
    Vadimskyi likes this.
  4. F21H43

    F21H43

    Joined:
    Feb 3, 2017
    Posts:
    4
    ok this partially worked,
    I know have it moving in multiple directions,
    but the for loop only ever goes through the first iteration, which seems weird,
    but I think I might be able to pull it out of the coroutine and just queue multiple moves in the same direction
     
  5. F21H43

    F21H43

    Joined:
    Feb 3, 2017
    Posts:
    4
    all of the this odd logic is because it's handled on the tiles end when I set the tile, and could probably be cleaned up a little. I wasn't sure how in depth I should get since I was fairly confident in that part of the code working.
     
  6. F21H43

    F21H43

    Joined:
    Feb 3, 2017
    Posts:
    4
    Yep this works like a charm, thanks for the tips
     
unityunity