StartCoroutine is not working correctly in C#

Hi, I’m pretty new to unity and I’m having trouble coding the movement for my player. I want to create a game in which you give the player directions with the WASD keys before he starts moving. Once you hit spacebar, the player will move based on the series of directions you gave him. To do this I have created the list called movements, and every time you press a WASD key, one of the strings “up” “left” “down” or “right” is added to this list. However, when i press the spacebar, I don’t want the player to move to the final position in one frame. So I added a coroutine. Yet currently my player waits 2 seconds and then moves to the final position in one frame. But I want to wait 2 seconds between every move/loop of the for loop. This is my code…

using System.Collections;
using System.Collections.Generic;
using UnityEngine;
using UnityEngine.SceneManagement;

[RequireComponent (typeof(Rigidbody))]
public class movementInMaze : MonoBehaviour {

	private List<string> movements = new List<string>();
	private bool canGiveDirections;
	private bool movementCanStart;
	public GameObject player;
	private Vector3 startingPos;
	private MazeLoader mazeScript;
	private string currentMove;


	// Use this for initialization
	void Start () {
		
		canGiveDirections = true;
		movementCanStart = false;
		startingPos = new Vector3 (0, 0, 0);
		player.transform.localPosition = startingPos;
	}
	
	// Update is called once per frame
	void Update () {

		print (movements.Count);
		
		if(Input.GetKeyDown(KeyCode.W) && canGiveDirections)
		{
			movements.Add("forward");
		}
		else if(Input.GetKeyDown(KeyCode.D) && canGiveDirections)
		{
 			movements.Add("right");
		}
		else if(Input.GetKeyDown(KeyCode.S) && canGiveDirections)
		{
			movements.Add("backward");
		}
		else if(Input.GetKeyDown(KeyCode.A) && canGiveDirections)
		{
			movements.Add("left");
		}

		if (Input.GetKeyDown("space") && movementCanStart == false)
		{
			canGiveDirections = false;
			movementCanStart = true;
			giveDirection ();
		}
	}

	void giveDirection (){
		
		if (movementCanStart) {

			for (int i = 0; i < movements.Count; i++) {
				
				print ("here1");
				StartCoroutine(move(movements *));*
  •  	}*
    
  •  }*
    
  • }*

  • IEnumerator move(string x) {*

  •  yield return new WaitForSeconds (2.0f);*
    
  •  if (x == "forward") {*
    
  •  	startingPos += new Vector3 (0, 0, 1);*
    
  •  	player.transform.localPosition = startingPos;*
    
  •  } else if (x == "right") {*
    
  •  	startingPos += new Vector3 (1, 0, 0);*
    
  •  	player.transform.localPosition = startingPos;*
    
  •  } else if (x == "backward") {*
    
  •  	startingPos += new Vector3 (0, 0, -1);*
    
  •  	player.transform.localPosition = startingPos;*
    
  •  } else if (x == "left") {*
    
  •  	startingPos += new Vector3 (-1, 0, 0);*
    
  •  	player.transform.localPosition = startingPos;*
    
  •  	print ("here2");*
    
  •  }*
    
  • }*
    }
    I have read through the manual for coroutines, and tried to move the “yield return new WaitForSeconds” statement to different places. I still don’t understand it. Any help would be much appreciated!

At the moment, you start n Coroutines at once (where n is the number of strings in your list) which all wait 2 Seconds and are then executed.

What you want is either

  • Start n Coroutines at once, where the first waits for 2 seconds, the second for 4 seconds, the third for 6 seconds and so on… OR:
  • Start n Coroutines sequentially, i.e. the second after the first has completed, the third after the second has completed and so on.

I personally would suggest using the second suggestion with a sort of recursive move()-method.

EDIT: Recursion is bad as @Bunny83 pointed out; changed the move()-method to use a loop instead. Also updated the name of the Move-Method according to naming standards pointed out by Bunny83.

Sorry if that’s not what you’ve been asking for, but I took the liberty to do some (imho) optimization in the rest of your code… :wink: First of all, I wouldn’t store the collected data (the movement-instructions) as strings; furthermore, this seems like the perfect use case for a Queue which works FirstInFirstOut: Queue.Enqueue() adds an element at the end of a Queue; Queue.Dequeue() returns the element from the beginning of the Queue and removes it from the Queue at the same time.

So I would change your Movement-Var to

private Queue<Vector3> movements = new Queue<Vector3>();

(You already import System.Collections.Generic, so Queue should already be available for you)

Then in your Update-Method:

if(canGiveDirections) {
	if(Input.GetKeyDown(KeyCode.W)) {
		movements.Enqueue(new Vector3(0,0,1));
	} else if(Input.GetKeyDown(KeyCode.A)) {
		movements.Enqueue(new Vector3(-1,0,0));
	}
	// and so on...

            if (Input.GetKeyDown("space") && movementCanStart == false)) {
                    canGiveDirections = false;
                    movementCanStart = true;
                    StartCoroutine(Move());
            }
}

I would not use the giveDirection() method because currently, you only check if movementCanStart == true, and you set movementCanStart to true immediately before calling it, so this check seems redundant.
Of course, if you have additional logic to perform / side effects to do, and want them rather in a different method than Update() for “tidier” code, you can re-introduce the giveDirection()-method at any time.

Then finally in Move():

Ienumerator Move(Vector3 movement) { // improved by Bunny83
            while(movements.Count > 0) {
	    yield return new WaitForSeconds (2.0f);
	    startingPos += movement;
	    player.transform.localPosition = startingPos;
	}

        print ("finished with movement");
}

Please Note: I did not write this code in an IDE and did not test it, so there might still be syntax errors in it or something; if so, apologies…

And: If you want to store the directions longer, so they can be read again later, my Queue-Approach is of course useless. But still in that case, I wouldn’t use strings but rather store the Vector3s (as in my Queue-Example) or introduce an Enum for the directions. Anyway, you can of course extract the relevant part for you (the Coroutines) and ignore everything else. :wink:

The statement yield return new WaitForSeconds is used to wait for a duration. So you programmed the result you experienced.

You should make a loop in your coroutine until the movement has ended. In the content of this loop, you put the statement yield return null that will wait for the next update and the modification on the transform.

IEnumerator Move(Vector3 targetPosition)
{
  while(transform.position != targetPosition)
  {
    yield return null;
    transform.position.MoveTowards(targetPosition...); // to fill with your data
  }
}