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. Dismiss Notice

What is wrong with this code?

Discussion in 'Scripting' started by Kensei, Sep 23, 2014.

  1. Kensei

    Kensei

    Joined:
    Apr 26, 2013
    Posts:
    63
    Guys I'm writing a grid based movement code and I'm having some problems I cant iron out, mostly because I have no idea where they are. It seems to me I have no understanding on the execution order when it comes down to coroutines, as well as I can't grasp how the lerp function works. Here's my code, I will list my problems with it afterwards:
    Code (CSharp):
    1. using UnityEngine;
    2. using System.Collections;
    3.  
    4. public class Tile : MonoBehaviour
    5. {
    6.     public float itemScore;
    7.     public float smooth = 0.7f;
    8.  
    9.     private float h = 0;
    10.     private float v = 0;
    11.  
    12.     private bool moving = false;
    13.     void Update()
    14.     {
    15.         Control ();
    16.     }
    17.  
    18.     void Control ()
    19.     {
    20.         if (moving == false && Input.GetAxisRaw ("Horizontal") != 0) {
    21.             h = Input.GetAxisRaw ("Horizontal");
    22.             moving = true;
    23.             StartCoroutine (Move (h, v));
    24.             h = 0;
    25.             print ("horizontal" + h + "" + v);
    26.             Debug.Log (moving);
    27.         }
    28.         if (moving == false && Input.GetAxisRaw ("Vertical") != 0) {
    29.             v = Input.GetAxisRaw ("Vertical");
    30.             moving = true;
    31.             StartCoroutine (Move (h, v));
    32.             v = 0;
    33.             print ("vertical" + h + "" + v);
    34.             Debug.Log (moving);
    35.         }
    36.     }
    37.  
    38.     IEnumerator Move(float directionH, float directionV)
    39.     {
    40.         Vector3 currentPos = transform.position;
    41.         Vector3 targetPos = new Vector3 (directionH * GameController.GM.distanceDelta,directionV * GameController.GM.distanceDelta,0);
    42.         while(currentPos != targetPos)
    43.         {
    44.             transform.position = Vector3.Lerp(currentPos,currentPos + targetPos,smooth);
    45.             yield return null;
    46.         }
    47.         moving = false;
    48.         Debug.Log(moving + "after coroutine");
    49.         yield return null;
    50.     }
    51. }
    52.  
    Just to quickly summarize, the idea is to move an object a set amount of units in a specified by the player direction. He must not have control over movement while it is being executed, and he must see the movement instead of the items just appearing at the target position.

    Ok so here are the things I'm having trouble with. In the Move() coroutine, I never get as far as my debug.log. I was under the impression that a yield return statement returns control back to the system and then continues the code after it. I get what it does in a while loop, if I use yield return, the loop will continue looping without locking down the system. If I use yield break, the loop will break and stop, returning control to the system and never restarting until another call. Now, what exactly does yielding do in a coroutine? other than saying yield return new waitforseconds or startcouroutine, does a yield return inside a while loop also break out of a coroutine? How can I set the moving boolean to false, after the while loop, but still inside the coroutine?

    My other problem is with the Vector3.Lerp, I can't understand what the last parameter is supposed to be. In the documentation it says its some kind of fraction and is clamped between 0 and 1, but then I see in the tutorials section of the website that the guy just uses a smooth float multiplied by Time.deltaTime, however this didn't do the trick for me. So how can I make my object actually visibly move and not teleport to the specified location upon horizontal/vertical input? I used coroutines for it in order to take control away from the player so that he can't move things while they are still moving.

    I've been staring at this for 2 days straight now and I've tried a bunch of different approaches like moveTowards, Translate,Vector2.Lerps, Mathf.Lerp, changing positions in Update upon button press,Rigidbody2D.position and some other kinda stupid ideas. Any help is very much appreciated, especially on the coroutine execution order.
     
  2. Kogar

    Kogar

    Joined:
    Jun 27, 2013
    Posts:
    80
    not tested but i commented a part of your code where i directly am seeing trouble.
    currentPos does not get updated and has to be updated in your while loop
    Also GetAxisRaw produces code between -1 to 1. So you need to check and define your direction or you would multiply your distanceDelta with different values (0.1f, -0.3f and so on)
    Code (csharp):
    1. IEnumerator Move(float directionH, float directionV)
    2.   {
    3.   Vector3 currentPos = transform.position;
    4.   Vector3 targetPos = new Vector3 (directionH * GameController.GM.distanceDelta,directionV * GameController.GM.distanceDelta,0);
    5.   while(currentPos != targetPos)
    6.   {
    7.   transform.position = Vector3.Lerp(currentPos,currentPos + targetPos,smooth);
    8.        currentPos = transform.position; //needs to get updated or the while loop will never be ended.
    9.        //yield return null;  //not sure if needed. Try it with and without
    10.   }
    11.   moving = false;
    12.   Debug.Log(moving + "after coroutine");
    13.   yield return null;
    14.   }
    yield can be nice if a script part should wait before it continous.
    Code (csharp):
    1. yield return new WaitForSeconds(secondsToWaitFloat); //Be it a simple wait for a duration or
    2. yield return hasEventFinished(eventStart) //or waiting for a functiong to be true
    More to coroutines here
    http://unity3d.com/learn/tutorials/modules/intermediate/scripting/coroutines
     
  3. Eric5h5

    Eric5h5

    Volunteer Moderator Moderator

    Joined:
    Jul 19, 2006
    Posts:
    32,398
  4. JoeStrout

    JoeStrout

    Joined:
    Jan 14, 2011
    Posts:
    9,840
    I can help with this part. The tutorial you reference is misusing Lerp, and this is becoming one of my pet peeves — passing in some multiple of Time.deltaTime to any of the Lerp functions is just wrong wrong wrong, and causes a lot of confusion (as you so ably demonstrate).

    Lerp is very simple: it interpolates between two values (let's call 'em A and B). The third parameter, t, is how far from A towards B you want to go. When t=0, you get A. When t=1, you get B. when t=0.5, you get exactly halfway from A to B (i.e. the average or midpoint). That's all there is to it.

    So I don't blame you for being confused when you see people passing in something like "smooth * Time.deltaTime" for t. This is utter nonsense. It works, sort of, because they're also continually shifting A to the current value of the object. So what they're doing is basically saying "interpolate from my current position, towards the ending position, by some small amount I really can't justify but which appears to look nice on my machine under my testing conditions."

    What you should do instead is keep A and B fixed, and make t go from 0 to 1 over some amount of time.

    Unity provides other good ways of doing pretty much the same thing; there is Vector3.MoveTowards, for example, which is sometimes easier to use. Or there's Vector3.SmoothDamp, which is a little harder to use, but does nice easing-in and easing-out of the motion. With floats, I'm quite fond of Mathf.SmoothStep, which is as easy to use as Lerp but does automatic ease-in and ease-out. Sadly there is no Vector3.SmoothStep, but you can just use Vector3.Lerp, and wrap your t parameter in a Mathf.SmoothStep call.

    So you see, there are many excellent ways of smoothly moving something from one position to another... but what's in that @*#! tutorial isn't one of them.
     
    Kensei likes this.
  5. Kensei

    Kensei

    Joined:
    Apr 26, 2013
    Posts:
    63
    Thanks for the reply. I'm using GetAxisRaw because I don't want to influence the movement speed by the input. I just need to know the direction, and Raw does a great job. As for the yield statement in the while loop, it is very needed, since without it Unity freezes and I have to kill the process. Also, I have watched the coroutines tutorial here as well as a lot of other tuts on youtube, but I still fail to understand what does a yield statement yield when inside a while loop in a coroutine.
    If I have
    Code (CSharp):
    1. private IEnumerator Coroutine()
    2. {
    3. while(some logic)
    4. {
    5. yield return null;
    6. }
    7. }
    Would that restart the while loop until the condition is met, or will it restart the coroutine?

    Ultimately though I still have a problem with my moving boolean, which is never reset to false and stays true after the first time I move the object.

    @Eric5h5 and @JoeStrout, thanks for the informative posts. I finally understand what does the last parameter mean. Eric, I could just copy and paste the code from the wiki, but it would feel like I'm cheating myself. I'd like to figure out what's wrong with my own code, not because I'm stubborn, I understand that saving time and work is what code reuse is for, however I'd like to know that I would be able to solve such a problem on my own before I skip over it.
     
  6. Eric5h5

    Eric5h5

    Volunteer Moderator Moderator

    Joined:
    Jul 19, 2006
    Posts:
    32,398
    That's fine; I wasn't actually thinking you'd copypaste, but rather I figured you could use the code on the wiki as a reference in order to fix problems in your own code.

    --Eric
     
  7. Kogar

    Kogar

    Joined:
    Jun 27, 2013
    Posts:
    80
    If i remember correctly the difference between GetAxis and GetAxisRaw was that GetAxis smoothes the values before returning them. GetAxisRaw will give -1 Zero and 1 with a keyboard and maybe a gamepad. But an analogjoystick will still give intermediate values if the joystick has not reaches its borders.

    This is a good tutorial for coroutines
    http://unitypatterns.com/introduction-to-coroutines/
    http://unitypatterns.com/scripting-with-coroutines/
    In introduction to coroutines there is this textpassage in the yielding part.
    Code (csharp):
    1. NOTE: Yielding on 0 (zero) or null tells the Coroutine to wait until the next frame before continuing. But you can yield on other Coroutines as well, which I will cover in the next tutorial.
    In the second tutorial there is a nice explanation for "Move On Path".
     
    Kensei likes this.
  8. Kensei

    Kensei

    Joined:
    Apr 26, 2013
    Posts:
    63
    Ok I got it to work the way I want, however I still have one problem: When I move from origin, I can only do so once, and can't advance further in the same direction. If I move from any other position, as long as it's not 0 I have no problems.
    Code (CSharp):
    1. using UnityEngine;
    2. using System.Collections;
    3.  
    4. public class Tile : MonoBehaviour
    5. {
    6.     public float itemScore;
    7.     public float smooth = 0.7f;
    8.  
    9.  
    10.     private bool moving = false;
    11.     void Update()
    12.     {
    13.         Control ();
    14.     }
    15.  
    16.     void Control ()
    17.     {
    18.         //If the tile isn't moving, take the player input and store it in a 2D vector. If that vector
    19.         //is more than zero, start the movement coroutine.
    20.         if(!moving)
    21.         {
    22.             Vector2 input = new Vector2 (Input.GetAxisRaw("Horizontal"),Input.GetAxisRaw("Vertical"));
    23.             if(input != Vector2.zero)
    24.             {
    25.                 StartCoroutine(Move (input));
    26.             }
    27.         }
    28.     }
    29.  
    30.     IEnumerator Move(Vector2 input)
    31.     {
    32.         //Set the move bool to true, create a temporary variable for the lerp. Then store the current position and calculate
    33.         //the target position.
    34.         moving = true;
    35.         float t = 0;
    36.         Vector3 currentPos = transform.position;
    37.         Vector3 targetPos = new Vector3 (input.x * GameController.GM.distanceDelta,input.y * GameController.GM.distanceDelta,0);
    38.         //Lerp the object to the target position.
    39.         while(currentPos != targetPos && t < 1)
    40.         {
    41.             t += Time.deltaTime * smooth;
    42.             transform.position = Vector3.Lerp(currentPos,currentPos + targetPos,t);
    43.             yield return null;
    44.         }
    45.         //Once the object has arrived at the target position, make that the current one and set the moving bool to false.
    46.         if(transform.position == targetPos)
    47.         {
    48.             currentPos = targetPos;
    49.  
    50.             Debug.Log("current position reset to " + currentPos + ",movement bool is:" + moving);
    51.         }
    52.         moving = false;
    53.         yield return null;
    54.     }
    55. }
    56.  
     
  9. Kogar

    Kogar

    Joined:
    Jun 27, 2013
    Posts:
    80
    tried it. And had to look a while too.
    targetPos needs a better name. Currently it is targetDelta. After a first deltaMove from (0/0) you would check if the next delta is currentPosition(delta)==delta :)
    So your while should look like this
    Code (csharp):
    1. while(currentPos != (currentPos + targetPos) && t < 1)
    or you should directly give targetPos the final destination. targetPos = currentPos + targetPos; after you inputted your targetDirection with the distanceDelta.

    edit: I have adjusted it for myself to lookup at for a later time.
    I have seen that the while loop is always running for one second. Without regarding the currentPos.
    Instead of directly lerping into transform.position write it into currentPos. Then write currentPos to transform.position.
    Since the value in lerping seems to always be hard to maintain i'm writing my version down.
    Code (csharp):
    1. IEnumerator Move(Vector2 input)
    2. {
    3.   //Set the move bool to true, create a temporary variable for the lerp. Then store the current position and calculate
    4.   //the target position.
    5.   float timeForMovementInSeconds = 0.5f;
    6.   moving = true;
    7.   float t = 0;
    8.   Vector3 currentPos = transform.position;
    9.   Vector3 startPos = currentPos;
    10.   Vector3 targetPos = new Vector3 (input.x * GameController.GM.distanceDelta,input.y * GameController.GM.distanceDelta,0);
    11.   targetPos = currentPos + targetPos;
    12.   //Lerp the object to the target position.
    13.   while(currentPos != targetPos)
    14.   {
    15.   t += Time.deltaTime;  //t=elapsedTime
    16.   float normalisedTime = Mathf.Clamp((t / timeForMovementInSeconds), 0, 1); //Value between 0 and 1
    17.   //or define it by a speed value
    18.   //float normalisedTime = Mathf.Clamp((t * smooth), 0, 1); //Value between 0 and 1; 0=start, 1=target
    19.   currentPos = Vector3.Lerp(startPos,targetPos, normalisedTime);
    20.   transform.position = currentPos;
    21.   yield return null;
    22.   }
    23.   moving = false;
    24.   yield return null;
    25. }
     
    Last edited: Sep 27, 2014
    Kensei likes this.
  10. Kensei

    Kensei

    Joined:
    Apr 26, 2013
    Posts:
    63
    Omg I was staring at my code for a whole day and couldn't figure it out. Thanks for the help man! However it seems like I used the wrong choice here, lerping doesn't do anything with colliders so theres no way to restrict movement.

    I tried changing transform.position to rigidbody2d.position but it ain't working properly. Looks like I'll have to scrap this controller all together and think of another way to make a physics based 2D grid movement. Kinda sux. Anyway thanks for the help bro.
     
    Last edited: Sep 29, 2014