Help with 2D crouching

Hey guys, I have been working on this for about a half hour and I cannot seem to get it to work as intended. Can any of you tell me what I am doing wrong here?

Code (CSharp):

1.  
2.         if(!crouching && Input.GetButtonDown("Crouch"))
3.         {
4.             maxSpeed += crouchSpeed;
5.             player.GetComponent<BoxCollider2D>().enabled = false;
6.             crouchCollider.GetComponent<BoxCollider2D>().enabled = true;
7.             crouching = true;
8.  
9.         }
10.  
11.         if (crouching && Input.GetButtonDown("Crouch"))
12.         {
13.  
14.             maxSpeed -= crouchSpeed;
15.             player.GetComponent<BoxCollider2D>().enabled = true;
16.             crouchCollider.GetComponent<BoxCollider2D>().enabled = false;
17.             crouching = false;
18.  
19.         }

The code is in the Update() function and the crouching variable is initially set to false. For some reason the boolean isn't flipping back and forth or something. Thank you for in advance and I hope we can figure this out together!

 

You are setting it true, then most likely in the next if it gets set to false right away..?

 

Change you code to something like this:

Code (csharp):

1. if(Input.GetButtonDown("Crouch"))
2. {
3.     maxSpeed = crouching ? crouchSpeed : normalSpeed;
4.     player.GetComponent<BoxCollider2D>().enabled = !crouching;
5.     crouchCollider.GetComponent<BoxCollider2D>().enabled = crouching;
6.     crouching = !crouching;
7. }

Obviously you would have to define the normalSpeed variable, but for me at least that is much more readable code. I'm not sure what is wrong with your code, but it was hard for me to read so I didn't bother.

Basically you just toggle all your settings based on the crouching boolean.

EDIT: Sorry, obviously it has to be like this or your crouching boolean will show the wrong thing if you inspect it:

Code (csharp):

1. if(Input.GetButtonDown("Crouch"))
2. {
3.     crouching = !crouching;
4.     maxSpeed = crouching ? crouchSpeed : normalSpeed;
5.     player.GetComponent<BoxCollider2D>().enabled = !crouching;
6.     crouchCollider.GetComponent<BoxCollider2D>().enabled = crouching;
7. }

 

Yeah, after doing some debug logs in both set of brackets they are both triggering. How would you go about fixing this?

 

Interesting. I will try this when I get home tonight. Thank you, I will let you know if I have any problems.

 

I realize my last answer was a bit lazy, but as for your original code. If you add something like print("is here") in the first if and then print("is here as well") in the second I'm sure you'll see what your problem is. And this is the reason why:

Code (csharp):

1. // Let's say crouching is false originally.
2. crouching = false;
3.  
4. // Because crouching is false this evaluates to true when we click the button.
5. if(!crouching && Input.GetButtonDown("Crouch"))
6. {
7.     maxSpeed += crouchSpeed;
8.     player.GetComponent<BoxCollider2D>().enabled = false;
9.     crouchCollider.GetComponent<BoxCollider2D>().enabled = true;
10.  
11.     // Crouching is set to true.
12.     crouching = true;
13. }
14.  
15. // Because crouching now is changed to true this also evaluates to true.
16. // Input.GetButtonDown("Crouch") still equals true
17. if (crouching && Input.GetButtonDown("Crouch"))
18. {
19.     maxSpeed -= crouchSpeed;
20.     player.GetComponent<BoxCollider2D>().enabled = true;
21.     crouchCollider.GetComponent<BoxCollider2D>().enabled = false;
22.  
23.     // Crouching is set to false again.
24.     crouching = false;
25. }

And the reason for this behavior is that Update() is run every frame and GetButtonDown() returns true during the frame the user pressed the given mouse button. So Input.GetButtonDown("Crouch") will be true for the entire Update() call. There's no reason why Input.GetButtonDown("Crouch") will suddenly not be true for your second if-statement. It will only be false in the next call to Update().

Basically, if the user clicks the "Crouch"-button then Input.GetButtonDown("Crouch") will be equal to true for the entire Update() for that frame. That is where you went wrong. You must have thought of it like a trigger or something that was only true for that one if-statement, but as far as I know there's no code that works like that.

This is how you you would have to write it if you wanted to keep your original code:

Code (csharp):

1. if(Input.GetButtonDown("Crouch"))
2. {
3.     if(!crouching)
4.     {
5.         maxSpeed += crouchSpeed;
6.         player.GetComponent<BoxCollider2D>().enabled = false;
7.         crouchCollider.GetComponent<BoxCollider2D>().enabled = true;
8.         crouching = true;
9.     }
10.     else {
11.         maxSpeed -= crouchSpeed;
12.         player.GetComponent<BoxCollider2D>().enabled = true;
13.         crouchCollider.GetComponent<BoxCollider2D>().enabled = false;
14.         crouching = false;
15.     }
16. }
17.  

But like I said. I think my version is cleaner and it does the same thing. It's just a matter of preference.

Edit: If this code is the last code in your Update() function then you could also add return; to the end of the first if-statement. This would ensure the second if-statement was never evaluated for that frame. But this is a very hacky solution to a non-problem.

 

This information was incredibly valuable to me, thank you. I really appreciate you taking the time .

 

I am trying to understand how your way works. I want to be more efficient, but at the same time I want to understand how the code works in my game. Do you have time to give a slight explanation? Thanks again.

 

You got both examples to work?

Basically my way does the exact same thing as the last example I gave, but in a more compact way. It takes some time to wrap your head around it, but when you do I find it both easier to read and it's less code to write, which means less that can go wrong or less to change if you decide to tweak it later.

I've tried commenting in the code to explain what the code does.

First your original code, just with those changes I mentioned so that it actually works:

Code (csharp):

1. // Check if we clicked the crouch-button.
2. if(Input.GetButtonDown("Crouch"))
3. {
4.     // An if/else to decide what to do depending on whether or not crouching is true or false.
5.     if(!crouching)
6.     {
7.         // Increase maxSpeed when we are not crouching.
8.         maxSpeed += crouchSpeed;
9.         // Disable/enable colliders for when we are not crouching.
10.         player.GetComponent<BoxCollider2D>().enabled = false;
11.         crouchCollider.GetComponent<BoxCollider2D>().enabled = true;
12.         // Change the crouching variable from false to true.
13.         crouching = true;
14.     }
15.     else {
16.         // And this code is almost identical to the code above. It's just opposite.
17.         maxSpeed -= crouchSpeed;
18.         player.GetComponent<BoxCollider2D>().enabled = true;
19.         crouchCollider.GetComponent<BoxCollider2D>().enabled = false;
20.         crouching = false;
21.     }
22. }

Then my compact form (which should do the exact same thing, apart from the maxSpeed thing which I mentioned you would have to change, but I felt it was for the better):

Code (csharp):

1. // Check if we clicked the crouch-button.
2. if(Input.GetButtonDown("Crouch"))
3. {
4.     // The if/else here is gone. Instead we set crouching to the oppsite of what it was.
5.     // So if crouching is true it will be set to false and if crouching is false it will be set to true.
6.     crouching = !crouching;
7.    
8.     // Here I set the max speed depending on whether crouching is true or false.
9.     // This is a shorthand form of an if else. This code is exactly the same as this:
10.     // if (crouching)
11.     // {
12.     //     maxSpeed = crouchSpeed;
13.     // }
14.     // else {
15.     //     maxSpeed = normalSpeed;
16.     // }
17.     maxSpeed = crouching ? crouchSpeed : normalSpeed;
18.    
19.     // This collider we set to the oppsite value of what crouching currently is.
20.     // So if crouching is true this will be set to false and if crouching is false this will be set to true.
21.     player.GetComponent<BoxCollider2D>().enabled = !crouching;
22.    
23.     // This collider we set to the value that crouching is.
24.     // So if crouching is true this will be set to true and if crouching is false this will be set to false.
25.     crouchCollider.GetComponent<BoxCollider2D>().enabled = crouching;
26. }

To explain my compact form some more. The first line changes crouching to the opposite of what it was. Meaning that when we click the crouch-button we'll toggle between the 2 "states". And then I just set all the values to be either the same as the crouching-variable or the opposite of it.

And the reason I prefer setting the value of maxSpeed instead of increasing/decreasing it like you did in your original code is that I feel it's easier to read and less error prone. It's easier for me to read it and be like "Ahh, now my max speed is my crouching speed" or "Now it is my normal speed" instead of "Hmm, my max speed is my max speed subtracted by my crouch speed. What was my max speed? What is it now?". And if for whatever reason some freak thing should happen and the code in Update() ended up being run 2 times in a row with the same "state" then you could end up decreasing max speed below or above your intended value. If you always just set it to a specified value then it can never become anything else than that.

 

Wow, I didn't even know there was a shorthand for an if/else statement. I can see why this would be less error prone. Thank you again!