Feedback Organizing a PlayerController script

I am making a FPS game and something I just realized is that I have way too many variables in my PlayerController script. In fact, this whole script is a mess... I might have to redo it.

This script was initially made for a CharacterController and then I tried to use a rigidbody instead, since character controllers are just too limited.

So I thought you guys would want to see what could I improve here? I know there is a lot wrong here but I can't pinpoint what exactly and there might be special methods and calls that I didn't think of that can make this much better. I will divide the script into sections so each section can be analized individually, think of this as a multi-post.

First, there is just way too many global variables. I thought of using arrays, but I thought those would be even more confusing, especially when assiging values in the inspector. What could I do to use less variables, yet have all these values easily assignable (or at least make it more organized)?

Code (CSharp):

1. public class PlayerControl : MonoBehaviour
2. {
3.     [Header("Noise Properties")]
4.  
5.     [SerializeField]
6.     private float StandingNoise; //How much noise the player makes when standing still//
7.     [SerializeField]
8.     private float FallingNoise; //How much noise the player makes when hitting the ground with enough speed//
9.     [SerializeField]
10.     private float WalkingNoise; //How much noise the player makes while walking//
11.     [SerializeField]
12.     private float JumpingNoise; //How much noise the player makes when jumping//
13.     [SerializeField]
14.     private float SprintingNoise; //How much noise the player makes while sprinting//
15.     [SerializeField]
16.     private float CrouchingNoise; //How much noise the player makes while crouching//
17.     private float CurrentNoise; //How much noise the player is making at the moment, changes if player is crouching, sprinting, etc//
18.  
19.     [Header ("Spread Properties")]
20.  
21.     [SerializeField]
22.     private float StandingSpreadMultiplier; //How accurate the player gun is while it is standing still//
23.     [SerializeField]
24.     private float FallingSpreadMultiplier; //How accurate the player gun is while it is falling//
25.     [SerializeField]
26.     private float WalkingSpreadMultiplier; //How accurate the player gun is while it is walking//
27.     [SerializeField]
28.     private float JumpingSpreadMultiplier; //How accurate the player gun is while it is jumping//
29.     [SerializeField]
30.     private float SprintingSpreadMultiplier; //How accurate the player gun is while it is sprinting//
31.     [SerializeField]
32.     private float CrouchingSpreadMultiplier; //How accurate the player gun is while it is crouching//
33.     private float CurrentSpreadMultiplier; //How accurate the player gun is at the moment, changes if player is crouching, sprinting, etc//
34.  
35.     [Header("Speed Properties")]
36.  
37.     public bool AllowAirControl = true;
38.     public float WalkingSpeed; //The player speed for when the player is just walking//
39.     public float SprintingSpeed; //The player speed for when the player is sprinting//
40.     public float CrouchingSpeed; //The player speed for when the player is crouching//
41.     private float CurrentSpeed = 400f; //The player speed variable being used at the moment, changes if player is crouching, sprinting, etc//
42.     private Vector3 GeneralVelocity;
43.     [SerializeField]
44.     private float HarmfulVelocity = 500f; //Any values of velocity higher than this are going to harm the player//
45.     [SerializeField]
46.     private float VelocityDamage; //How much damage the player gets for each + 10 m/s of harmful velocity//
47.  
48.     [Header("Stamina Mechanics")]
49.  
50.     [SerializeField]
51.     private float MinStamina; //The minimum stamina necessary to start running or jumping//
52.     [SerializeField]
53.     private float StandingStaminaGain; //How much stamina is gained while not sprinting//
54.     [SerializeField]
55.     private float WalkingStaminaGain; //How much stamina is gained while walking//
56.     [SerializeField]
57.     private float CrouchingStaminaGain; //How much stamina is gained while crouching//
58.     [SerializeField]
59.     private float SprintingStaminaGain; //How much stamina is gained while sprinting//
60.     [SerializeField]
61.     private float JumpingStaminaGain; //How much stamina the player gains when jumping//
62.     [SerializeField]
63.     private float FallingStaminaGain; //How much stamina is gained while falling//
64.  
65.     [HideInInspector]
66.     public float Stamina;
67.     private float CurrentStamina;
68.  
69.     [Header("Dimensions")]
70.  
71.     [SerializeField]
72.     private float Radius = 0.6f;
73.     [SerializeField]
74.     private float Height = 3f;
75.     [SerializeField]
76.     private float CrouchingHeight = 1.8f; //Height of the player while crouching//
77.     [SerializeField]
78.     private float JumpingHeight = 1f; //How high the player jumps//
79.  
80.     [SerializeField]
81.     private float WalkingSlopeLimit = 45.0f; //How low the angle of a slope can be before the player cant climb it anymore//
82.     [SerializeField]
83.     private float JumpingSlopeLimit = 100.0f;
84.     private float CurrentSlopeLimit = 45.0f; //The player slope limit variable being used at the moment, changes if player is jumping, sprinting, etc//
85.  
86.     [SerializeField]
87.     private float WalkingStepLimit = 0.2f; //How high a step can be before the player cant climb it anymore//
88.     [SerializeField]
89.     private float JumpingStepLimit = 0.4f;
90.     private float CurrentStepLimit = 0.2f; //The player step limit variable being used at the moment, changes if player is jumping, sprinting, etc//
91.  
92.     private Vector3 LastPosition; //This player last position//
93.  
94.     [Header("Ground Checking")] //These variables are used for checking if player is grounded//
95.  
96.     public Transform GroundCheck;
97.     public LayerMask GroundMask;
98.  
99.     [Header("Main References")]
100.  
101.     public Rigidbody PlayerRigidbody;
102.     public Transform PlayerBody;
103.     public PlayerStats Player;
104.     public Guns PlayerGun;
105.     public Melee PlayerMelee;
106.  
107.     public enum PlayerState
108.     {
109.         Standing, Walking, Sprinting, Crouching, Falling, Jumping
110.     }
111.     [SerializeField]
112.     private PlayerState PlayerStatus;

Secondly, there is this portion of

Update()

made to assign fall damage, I am not sure if it's ok and nothing stands out as wrong here, but some kind of feedback of it would be appreciated:

Code (CSharp):

1.     void Update ()
2.     {
3.         if (Player.Dead)
4.         {
5.             return;
6.         }
7.  
8.         bool Grounded = Physics.CheckSphere (GroundCheck.position, Radius, GroundMask); //Checks if player is currently on the ground or airborne//
9.  
10.         if (Grounded && PlayerRigidbody.velocity.y < 0f)
11.         {
12.             if (-PlayerRigidbody.velocity.y >= HarmfulVelocity)
13.             {
14.                 float FallDamage = VelocityDamage * (-PlayerRigidbody.velocity.y / 10f);
15.                 Player.Hurt (FallDamage);
16.  
17.                 if (Player.NoiseLevel < FallingNoise)
18.                 {
19.                     Player.NoiseLevel = FallingNoise;
20.                 }
21.  
22.                 Debug.Log ("Player hits the ground with high speed;");
23.             }
24.         }

This part is plain out broken, it is responsible for the movement. But it's ok, I already asked help earlier about this and I have an idea of what should be done to make this better (including putting it in

FixedUpdate()

instead of

Update()

).

Code (CSharp):

1.         //Player main movement shenanigans//
2.         float X = Input.GetAxisRaw ("Horizontal") * CurrentSpeed;
3.         float Z = Input.GetAxisRaw ("Vertical") * CurrentSpeed;
4.  
5.         Vector3 MovePosition = transform.right * X + transform.forward * Z;
6.         PlayerRigidbody.AddForce(new Vector3 (MovePosition.x, 0f, MovePosition.z), ForceMode.Force);
7.         //Player main movement shenanigans//

Then, there is this, which is just ridiculous. It is a mess of ifs and elses that burns your eyeballs, made to check inputs and call methods to deal with them (things like jumping, crouching, sprinting, etc) inside of

Update()

. I tried putting a switch there but I couldn't find anywhere it would fit, except to assign the player the right stamina according to their movement state. Isn't there any better way of dealing with this?

Code (CSharp):

1.         if (Grounded && !Input.GetKey (KeyCode.LeftControl) && !Input.GetKey (KeyCode.LeftShift) && PlayerStatus != PlayerState.Jumping)
2.         {
3.             Normal();
4.         }
5.  
6.         if (Input.GetKey(KeyCode.Space))
7.         {
8.             if (Grounded && CurrentStamina > MinStamina && PlayerStatus != PlayerState.Jumping && PlayerStatus != PlayerState.Falling)
9.             {
10.                 Jump();
11.             }
12.         }
13.  
14.         if (Input.GetKey (KeyCode.LeftControl) && !Input.GetKey (KeyCode.LeftShift))
15.         {
16.             if (Grounded)
17.             {
18.                 Crouch();
19.             }
20.         }
21.  
22.         if (Input.GetKey (KeyCode.LeftShift) && !Input.GetKey (KeyCode.LeftControl))
23.         {
24.             if (Grounded && CurrentStamina > MinStamina)
25.             {
26.                 Sprint ();
27.             }
28.             else if (CurrentSpeed >= SprintingSpeed && PlayerStatus != PlayerState.Sprinting)
29.             {
30.                 PlayerStatus = PlayerState.Sprinting;
31.             }
32.         }
33.  
34.         if (!Grounded && PlayerRigidbody.velocity.y < 0f)
35.         {
36.             PlayerStatus = PlayerState.Falling;
37.             CurrentSpreadMultiplier = FallingSpreadMultiplier;
38.         }
39.  
40.         switch (PlayerStatus)
41.         {
42.             case PlayerState.Standing:
43.                 CurrentStamina += Time.deltaTime * StandingStaminaGain;
44.             break;
45.  
46.             case PlayerState.Walking:
47.                 CurrentStamina += Time.deltaTime * WalkingStaminaGain;
48.             break;
49.  
50.             case PlayerState.Crouching:
51.                 CurrentStamina += Time.deltaTime * CrouchingStaminaGain;
52.             break;
53.  
54.             case PlayerState.Sprinting:
55.                 CurrentStamina += Time.deltaTime * SprintingStaminaGain;
56.             break;
57.  
58.             case PlayerState.Falling:
59.                 CurrentStamina += Time.deltaTime * FallingStaminaGain;
60.             break;
61.              
62.             case PlayerState.Jumping:
63.                 CurrentStamina += Time.deltaTime * FallingStaminaGain;
64.             break;
65.         }
66.         if (CurrentStamina >= Stamina)
67.         {
68.             CurrentStamina = Stamina;
69.         }
70.         else if (CurrentStamina <= 0f)
71.         {
72.             CurrentStamina = 0f;
73.             if (Grounded && PlayerStatus != PlayerState.Jumping)
74.             {
75.                 Normal();
76.             }
77.         }
78.         Player.CurrentStamina = CurrentStamina;
79.  
80.         LastPosition = transform.position;
81.  
82.         if (Player.NoiseLevel < CurrentNoise)
83.         {
84.             Player.NoiseLevel = CurrentNoise;
85.         }
86.  
87.         if (PlayerGun != null)
88.         {
89.             PlayerGun.CurrentSpread *= CurrentSpreadMultiplier;
90.         }
91.         else if (PlayerMelee != null)
92.         {
93.             PlayerMelee.CurrentSpread *= CurrentSpreadMultiplier;
94.         }

Lastly, each aforementioned movement state has their own

void method()

and I wanted to make sure I am doing this the right way. Again, I am aware the "jumping MATH" is broken but like I said earlier I think I have an idea on how to fix it.

Code (CSharp):

1.     void Normal ()
2.     {
3.         CurrentSpeed = WalkingSpeed;
4.  
5.         if (transform.position == LastPosition)
6.         {
7.             PlayerStatus = PlayerState.Standing;
8.             CurrentNoise = StandingNoise;
9.             CurrentSpreadMultiplier = StandingSpreadMultiplier;
10.         }
11.         else
12.         {
13.             PlayerStatus = PlayerState.Walking;
14.             CurrentNoise = WalkingNoise;
15.             CurrentSpreadMultiplier = WalkingSpreadMultiplier;
16.         }
17.     }
18.  
19.     void Jump ()
20.     {
21.         CurrentStepLimit = JumpingStepLimit;
22.         CurrentSlopeLimit = JumpingSlopeLimit;
23.  
24.         if (Player.NoiseLevel < JumpingNoise)
25.         {
26.             Player.NoiseLevel = JumpingNoise;
27.         }
28.  
29.         PlayerRigidbody.AddForce(new Vector3(PlayerRigidbody.velocity.x, PlayerRigidbody.velocity.y + Mathf.Sqrt(JumpingHeight * -2f * Physics.gravity.y), PlayerRigidbody.velocity.z), ForceMode.Impulse); //Jumping MATH//
30.         CurrentStamina += JumpingStaminaGain;
31.         PlayerStatus = PlayerState.Jumping;
32.         Debug.Log (this.name + " jumps;");
33.     }
34.  
35.     void Crouch ()
36.     {
37.         CurrentSpeed = CrouchingSpeed;
38.         CurrentNoise = CrouchingNoise;
39.         CurrentSpreadMultiplier = CrouchingSpreadMultiplier;
40.  
41.         if (PlayerStatus != PlayerState.Crouching)
42.         {
43.             PlayerStatus = PlayerState.Crouching;
44.             Debug.Log (this.name + " crouches;");
45.         }
46.     }
47.  
48.     void Sprint ()
49.     {
50.         CurrentSpeed = SprintingSpeed;
51.         CurrentNoise = SprintingNoise;
52.         CurrentSpreadMultiplier = SprintingSpreadMultiplier;
53.  
54.         if (PlayerStatus != PlayerState.Sprinting)
55.         {
56.             PlayerStatus = PlayerState.Sprinting;
57.             Debug.Log (this.name + " sprints;");
58.         }
59.     }

So what do you think? How spaghetti is this script, and what can I do to improve it? I really want help with this because I don't want to do it the wrong way just to find out I could do it better!

 

First principle is separation of concerns.

https://en.wikipedia.org/wiki/Separation_of_concerns

You have a player controller concerned with moving, noise, weapons, guns, melee, stamina, I mean that's real kitchen sink stuff.

Some quick improvements:

Remove the noise stuff, remove the weapon spread stuff.

Now over in your noise decision code, observe the player's velocity (perhaps through a getter function in the player control) and decide how noisy he is.

Over in your shooter code, observe the player's velocity and/or crouching and decide what his weapon spread is. For instance you might end up with 47 different weapons each with different properties based on the player speed... no need for all that to go in the player controller!

Remove the stamina stuff... make the player query a stamina manager to see if running is possible.

Conversely the stamina manager should observe the player and burn down stamina accordingly.

That way when you get the uber-espresso-shot-commando powerup and can run all day long, you just replace the stamina controller with an unlimited stamina controller for a period of time, no change at all to the player.

That stamina controller might also even be a more generic "player stats controller," such that it can say "I can run this fast right now, but when I drink a lot of coffee it gets way faster!"

 

How do I do that?

 

Just like every other interoperation between scripts:

- give the stamina script a reference to the player
- give the player script a reference to the stamina
- make methods in each that can give the other what it needs

"Can I run?"

"I am running, make me feel more tired."

"I am stationary and resting."

"Can I jump?"

etc.

It's your API to design, however complex or simple you like. I always like to start simple.

For extra bonus points you can even abstract it to an interface and have each side implement the interface the other expects, but that's just icing on the cake really. Just get the basics working first.

 

Wait, so you are suggesting a script just for the stamina??

 

Your original post actually observed, and I quote:

I suggested considering separation of concerns.

- Input gathering is one concern
- Input processing is another concern
- player state affecting input processing is yet another concern
- weapon accuracy is a farther and even less-connected concern

etc.

You can also write all of your games the way we did in the early days, all in one script.

But you see what happens with that approach. It's entirely up to you. It's your project.

 

Are you suggesting putting the stamina as it's own script or not?

 

Ok, I was able to make the basic parts of the controller work, but I have one problem, that is the player can stick to walls to be grounded. How can I make so that doesn't happen (does it require a capsule collider)?

 

How do I make so the player can't stick to walls?

 

I suppose you could try making the player or the walls really slippery with a PhysicMaterial.

 

That didn't work. The problem remains the same

 

Is it the "Grounded" variable check causing you not to fall?

If so , try reducing the radius of the OverlapSphere call while simultaneously putting a larger sphere collider on the object in such a way to keep the OverlapSphere check for succeeding anywhere but below you.

If it is not, then find out why you're not falling, perhaps by printing out values in this at runtime.

 

I used your script, and in it you have this instead to check for grounded:

Code (CSharp):

1.     void OnCollisionStay()
2.     {
3.         Grounded = true;
4.     }

So I understand now what is wrong. "OnCollisionStay" will obviously return true when colliding with the walls. Is it possible to make it work only downwards?

 

What is the opposite of OnCollisionStay? I want to check if the player is falling.

 

If you look at the top of the script it is not my script.

Code (csharp):

1. // Originally obtained from Unity Wiki:
2. // https://wiki.unity3d.com/index.php/RigidbodyFPSWalker
3. //
4. // Hacked up a bit by @kurtdekker to support more feechurz

To keep it from sticking to walls I just used the a zero-friction PhysicMaterial and that works fine in my test level.



If that doesn't work for your level layout / design / expectation, you may wish to try another RigidbodyFPS script. This one is just one I found years ago and I've never used it in any game.