Advice for Improving a Script - Code Review

Hi all, I'm looking for a review of my code to get a second (or more) opinion, to see if it might be improved. I'm fairly new to scripting in Unity, so I'm not sure what the best way to do things is (mostly I am not yet very familiar with the built-in functionality). I made a script to tilt a platform using mouse movement, constraining it to within a certain angle; the idea being a game where you tilt the platform to roll a ball around and solve puzzles using physics.

This script took me a total of about 10 hours I think to get to its current state (about 5 hours to get it fully 'functional', and another 5 or so improving/streamlining it; to be perfectly clear, this script is working as intended). I'm not sure what that says about my scripting ability lol. Anyway, here it is; any feedback or advice as to how it might be improved would be welcome.

Code (CSharp):

1. using System.Collections;
2. using System.Collections.Generic;
3. using UnityEngine;
4.  
5. public class PlatformController : MonoBehaviour
6. {
7.     // Editor-accessible tilt speed variable; defaults to 20
8.     public float tiltSpeed = 20.0f;
9.     // Editor-accessible max tilt variable; defaults to 7.5
10.     public float maxTilt = 7.5f;
11.  
12.     // Member variable to hold the parent's rigidbody component for physical manipulation
13.     new Rigidbody rigidbody;
14.  
15.     // Start is called before the first frame update
16.     void Start()
17.     {
18.         rigidbody = GetComponent<Rigidbody>();
19.     }
20.  
21.     // FixedUpdate is called once per physics frame (50/s)
22.     void FixedUpdate()
23.     {
24.         // Variables for the mouse's x and y movement
25.         float mouseX;
26.         float mouseY;
27.         float mouseXInput;
28.         float mouseYInput;
29.  
30.         // Variable for the current maximum tilt, dependent on the mouse axis direction or current tilt angle (positive or negative)
31.         float currentMaxTilt;
32.        
33.         //Variables for the current x (forward/backward) and z (left/right) rotation values, as well as the y (up/down) rotation so we can negate it if there is any
34.         float rotX = rigidbody.transform.rotation.eulerAngles.x;
35.         float rotZ = rigidbody.transform.rotation.eulerAngles.z;
36.         float rotY = rigidbody.transform.rotation.eulerAngles.y;
37.  
38.         // Correct for negative angles (apparently the euler angles go 0-359, as in degrees, so if we want to get a number of degrees from "up" we have to adjust the value so we get negative degrees
39.         if (rotX > 180.0f) rotX -= 360;
40.         if (rotZ > 180.0f) rotZ -= 360;
41.  
42.         // Get the mouse input
43.         mouseXInput = Input.GetAxis("Mouse X");
44.         mouseYInput = Input.GetAxis("Mouse Y");
45.  
46.         // Figure out what the total rotation should be, limiting the parent rotation to the max tilt value
47.         if (rotX <= maxTilt && rotX >= -maxTilt && mouseXInput != 0.0f)
48.         {
49.             // Calculate the total rotation from the mouse movement and tilt speed, per frame
50.             mouseX = mouseXInput * tiltSpeed * Time.deltaTime;
51.             Debug.Log("Mouse X Input Accepted! Raw Input: " + mouseXInput + "; " + "Final Output: " + mouseX);
52.  
53.             // If the rotation would exceed the maximum allowed tilt angle, only let it rotate to that maximum
54.             if ((rotX + mouseX > maxTilt) || (rotX + mouseX < -maxTilt))
55.             {
56.                 currentMaxTilt = maxTilt * Mathf.Sign(mouseX);
57.                 Debug.Log("Clamping X to a Maximum of " + currentMaxTilt);
58.                 mouseX = currentMaxTilt - rotX;
59.             }
60.             //Debug.Log("Current Rotation: " + rotX + "; " + "Accepted mouseX: " + mouseX);
61.         }
62.         else
63.         {
64.             // If we got here because the current rotation is out of range, move it back to the maximum angle, otherwise we are here because we aren't trying to rotate, so set the rotation to 0
65.             if (mouseXInput != 0.0f)
66.             {
67.                 currentMaxTilt = maxTilt * Mathf.Sign(rotX);
68.                 mouseX = currentMaxTilt - rotX;
69.                 Debug.Log("Fixing X with Rotation: " + mouseX);
70.             }
71.             else
72.                 mouseX = 0.0f;
73.         }
74.  
75.         if (rotZ <= maxTilt && rotZ >= -maxTilt && mouseYInput != 0.0f)
76.         {
77.             // Calculate the total rotation from the mouse movement and tilt speed, per frame
78.             mouseY = mouseYInput * tiltSpeed * Time.deltaTime;
79.             Debug.Log("Mouse Y Input Accepted! Raw Input: " + mouseYInput + "; " + "Final Output: " + mouseY);
80.  
81.             // If the rotation would exceed the maximum allowed tilt angle, only let it rotate to that maximum
82.             if ((rotZ + mouseY > maxTilt) || (rotZ + mouseY < -maxTilt))
83.             {
84.                 currentMaxTilt = maxTilt * Mathf.Sign(mouseY);
85.                 Debug.Log("Clamping Y to a Maximum of " + currentMaxTilt);
86.                 mouseY = currentMaxTilt - rotZ;
87.             }
88.             //Debug.Log("Current Rotation: " + rotZ + "; " + "Accepted mouseY: " + mouseY);
89.         }
90.         else
91.         {
92.             // If we got here because the current rotation is out of range, move it back to the maximum angle, otherwise we are here because we aren't trying to rotate, so set the rotation to 0
93.             if (mouseYInput != 0.0f)
94.             {
95.                 currentMaxTilt = maxTilt * Mathf.Sign(rotZ);
96.                 mouseY = currentMaxTilt - rotZ;
97.                 Debug.Log("Fixing Y with Rotation: " + mouseY);
98.             }
99.             else
100.                 mouseY = 0.0f;
101.         }
102.  
103.         //Debug.Log("Final Rotation: X: " + mouseX + " Y: " + mouseY);
104.  
105.         // Rotate the parent by the tilt angle, making sure the Y (up/down) rotation remains 0
106.         rigidbody.transform.Rotate(mouseX, -rotY, mouseY);
107.     }
108. }
109.  

PS: I've noticed when testing there there are some possible hangs or slowdown occasionally, but I'm unsure if this is due to my code or simply a performance problem with the editor (or a performance issue due to my code lol). It's hard to tell if it's just my imagination, as they are very brief and seemingly random.

 

FixedUpdate is really just supposed to be for physics related code, since it runs immediately before the physics update independent of frame rate. So this comment in your code is incorrect, as some frames are being skipped and the same values from a single frame are going to be used multiple times, depending on current performance on the machine:

Code (csharp):

1.             // Calculate the total rotation from the mouse movement and tilt speed, per frame

I'd consider moving this all to Update or LateUpdate.

Just a knit pick, code where you're doing several evaluations in a single "if" statement it is a good idea to add additional parenthesis for clarity.

Code (csharp):

1.         if (rotZ <= maxTilt && rotZ >= -maxTilt && mouseYInput != 0.0f)

becomes

Code (csharp):

1.         if ((rotZ <= maxTilt) && (rotZ >= -maxTilt) && (mouseYInput != 0.0f))

Your use of "new" on line 13 is unnecessary. I would probably make it make it Public and drag the reference there instead of using GetComponent for it in Start, unless I was building this GameObject at runtime. Also, I'd rename "rigidbody" to something else, as you're using the same name as an obsolete Unity variable which automagically referenced the rigidbody on the same GameObject. It is just confusing, as it looks like you're using a variable commonly used back in Unity 4, but removed today. You might even be getting a warning from Unity about it. Not sure if that is still the case.

https://docs.unity3d.com/ScriptReference/Component-rigidbody.html

But ignoring all that, unless you're working on a team with specific coding standards, the really only important things are that the code does what you intend, and that it performs within your requirements. If you can say yes to both, then the code is fine Good luck on your project!

edit:
After some more looking, it doesn't look like you're using the Rigidbody for anything but accessing the transform reference. You could just remove all the Rigidbody stuff and use gameObject.transform instead.

Code (csharp):

1. gameObject.transform.Rotate(mouseX, -rotY, mouseY);

 

Hey and welcome (even if this is not your first post)

So here are the things that 'jumped' at me. Meaning there may be more but those are most obvious:

• I believe those comments are mostly for yourself, since you are still learning. Which is fine, of course. However, since this is a review, i gotta mention it: comments are fine, if they contain useful information. For a variable called 'tiltSpeed' to which you assign a value of 20f, a comment saying that this is a tild speed variable that defaults to 20f is, to most people, a huge waste of space. That said, it only defaults to 20f unless you put a different value in the inspector. Then it defaults to that.
• Think about whether you want attributes to be public or private. This is not the same as making them appear on the inspector. You can have a [SerializeField] private variable, which makes it appear on the inspector. In fact, this should be the default. A variable should only be public, from an architectural viewpoint, if it actually needs to be accessed from a different script. And for the most part, in C# we use properties for this instead.
• For my taste you are using a bit too many (single) variables, inflating the length and thus reducing the readability of the code. For example, mouseXInput and mouseYInput could be put into a Vector2. More importantly, rotX, rotY, rotZ could be put into a Vector3. This also reduces the amount of assignments, since it's was a Vector in the first place.
• I believe you did not mean to write 'new' at line 13.
• Major beginner mistake: do not use FixedUpdate() for anything other than applying physics. Normally this means that you would get the inputs (and do calculations on them if possible) in Update(), and only apply forces and other physics in FixedUpdate. The main reason for this is performance. FixedUpdate is supposed to run at a fixed timestep. This can cause exponential slowdown if you are not careful. If your code in Update is slow, sure, you also lose FPS. But if your code in FixedUpdate is slow, it takes up more time, leaving less time for Update() before the next FixedUpdate needs to be calculated. As an example, the default fixed timestep is 0,02s, so 50x a second. If your code in FixedUpdate took 0.018s, you would have little to no time for Updates. On the other hand, if your Update code is slow, it only reduces the amount of frames you draw per second, while the amount of physics updates (FixedUpdate) stays more or less the same. If i did not make this clear, look it up. It's quite an important difference.
  
• That said, you are not actually using physics. You dont apply a force or anything. You could just replace your 'Rigidbody' with 'GameObject' and the code should run the same. You are manually updating the transform after all. This also means, in regard to the above point, that you should be able to just move the entire code from FixedUpdate to Update, in this specific case.
• Debug.Log, or writing to the console in general, is a bit costly. Normally this should not cause performance problems right off the bat, but since you are 'spamming' it so much in FixedUpdate, it may actually be the culprit. Generally speaking, you can use the Profiler to figure out where you are having performance issues. It's decently easy, as it was made for this purpose, but you should look it up since it's a bit much to explain here.
• I'm pretty sure the length of this code can be reduced drastically, but it works and that's all that counts. For future reference tho, i probably would have approached this by adding up the total change of rotation on the tracked axis (ie pitch and roll), clamp this value to the desired maximum tilts, and then set the rotation to this value each frame (plus potential smoothing). This would also be a lot shorter, most likely. An example for what i'm talking about can be found here. Even if it's about a camera, the concept is nearly identical.
  Spoiler: Video about Camera Clamp

All in all the code is definitely acceptable. I like that you are using the correct naming conventions.
Just to make this clear, a lot of the above is nitpicking. I've seen a lot worse coding.

Edit: Also, worth mentioning. I applaud your effort to improve your coding!
I really shouldnt be taking these things for granted

 

Thanks for the feedback so far

As a quick first note, the use of 'new' in the rigidbody declaration is to avoid a warning that it's an inherited member, so it's not strictly necessary, I know, but it suggested using the 'new' keyword, so I did. I didn't realize it was a bad idea to use that variable name in general; it was used in one of the tutorials I followed before; I guess it must have been an old tutorial.

To get straight to what seems to be the biggest issue, though, if I'm understanding right, with the current implementation of the rotation, I shouldn't use FixedUpdate at all. I had been unsure if this all qualified as physics, since I suspected it would only be so if I was, as Yoreki said, applying forces. However, my main reason for going with FixedUpdate was that I wanted it to interact with a ball, which is a purely physics-driven object, and I wasn't sure that updating the platform out of sync with physics would result in collisions between the two being wonky. This might also explain why the ball doesn't seem to accept forces from the platform correctly (like launching into the air when the platform is rapidly tilted; it does a little bit, but not much); though that may also just be a matter of the mass/gravity and the rate at which the platform can rotate. If there is a better, physical way to apply rotation that might be better for my intentions, I'd love to hear about it or at least be pointed to somewhere that I could learn more about it on my own.

I'm not sure I understand what you said, Yoreki, about the public variables. How would I write a private variable into the code that would still show up in the inspector? I assume there is a special way to declare it. I guess I could probably also look up [SerializedField], because I have no idea what that means haha.

In regard to the comments, I admit I may have been a bit overzealous; my goal was to make it as easy to read as possible, even if the reader had no idea how to read code. I guess it's probably not a good idea to let non-programmers even view the code in the first place, though, so maybe I shouldn't even bother with making it that obvious. I'm trying to make this game as if I was working with others, just to develop good practices if I ever do make a project in a group.

Also, I considered condensing the float variables (especially the current rotation ones) into vectors, but I had already used them individually with their current setup so figured it would be a hassle to change them now. As far as applying the rotation directly, I think I initially tried doing it that way but couldn't get it to work. I may have just been doing it wrong. As I was writing it, I thought to myself at several points that being able to use the Clamp function directly on the final rotation would have been a lot easier, but wasn't sure how I'd be able to do that, since I couldn't figure out how to apply the rotation directly.

The last thing I wanted to address, for clarity, is Joe's comment on the rigidbody being assigned in the Start function. The reason I do it that way is that this script is attached to the platform, so it will always be the rigidbody component of the platform that the script uses, so I figured it would just add an extra step if I made it public and filled it in the inspector, whereas I can just assign it in the code and know that it is going to get the right one every time and not depend on it being assigned in the inspector, which I might forget to do; and if anyone else was working on the project with me, it would be something I'd have to explain. Again, though, if I'm not understanding something or this reasoning doesn't make sense (if it doesn't necessarily work that way), I'll gladly take additional feedback.

I'm still very early (obviously) in the development of this project, so I will very likely revisit this script to make it as good as it can be before going much further, just to get a better understanding of working with Unity scripting before blundering through more scripts that I'd want/need to fix later. It is probably obvious, but I have a pretty firm grasp of syntax, but the actual functionality behind it is still a bit foggy to me.

 

When you rotate the platform gameobject, you also rotate the collider which is what is used in the physics calculation. As a rule of thumb, only use FixedUpdate if you actually do something specific to rigidbodies (ie, AddForce, ..). Everything else, especially Inputs and so on, belong in Update, or rarely LateUpdate (in case of pure visual updates like UI, sometimes Camera, ..).

SerializeField does just that. Or well, it's part of what it does. In reality it serialized the field (mindblow, i know ). The inspector shows fields that are being serialized. Public just does the same thing, since all public variables get serialized by Unity by default. However, if all you want is for the variable to show up in the inspector, using 'public' is incorrect from an architectural viewpoint. Instead use [SerializeField] private.

In case you are unfamiliar with what private and public actually does, they are access modifiers. They tell the compiler from where these attributes / fields / variables (methods, classes, properties, ...) can be accessed. A script can always access its own variables, no matter what. The most secure option is private. The least secure one is public, where it can be accessed by anybody from the outside. There are also others, like protected, which basically means 'private for me and classes deriving from me', which is used in inheritance. In case you did not look into OOP (object oriented programming) too much yet, i would recommend doing that as well. It's very useful for creating underlying data structures.

All you said here is true. First of all, yes comments should be written to be understood by programmers, or at the very least by you. If you are still learning and thus want over-obvious comments: that's absolutely fine! Obviously, if the code were to server for some kind of beginner tutorial, comments like these may be beneficial as well.
However, as a rule of thumb, you want comments to be as short and precise as possible, while as common as necessary, but not more. There is a saying that you cannot have too many comments, and technically that's true, but eventually you wont be able to find code between the comments anymore. Go for a balance that makes it (at least for you) as fast as possible to navigate your code, as well as to understand it.

I personally like to mark blocks of code with 'headline comments', that tell me which the following couple lines of code are meant to achieve. This helps me to nagivate my code pretty efficiently. On top of these navigation comments, everything that is not obvious, should be commented. For example, if you wrote something in a less-obvious format due to performance reasons.
Commenting as if you were working on it in a group is a good idea. Or rather, commenting at all is a good habit. The thing is, you will eventually come back to some code after not working on it for weeks. Be this because you paused the project, or simply worked on other parts and need to redo an old part. The future-you will have trouble understanding some / most of your code, no matter how 'speaking for itself' you believe the code to be right now. Future-you would like to find the part of the code it is looking for quickly, as well as to understand what non-obvious parts of the code are supposed to do, in case it needs to rework some parts of it.
And this is, for single-people projects, the main use of comments.

Take a look at the linked video (in the Spoiler). On that note, the same guy made an excellent gamedev tutorial series, where he goes over all the things you need to know about C# and Unity to get started. While there is probably not a whole lot of new information for you in the first couple episodes, he also offers challenges to apply the learned information, and i would generally say it's worth checking out.

I personally would do it the same way. Why would you assign it through the inspector if it's a component of the same gameobject @Joe-Censored? Unless i'm missing something it's just a tradeoff between a miniscule performance loss (outside the Update loop) vs. more values in the inspector. I'm personally not a huge fan of the latter.

 

Of course using GetComponent in Start is just fine for the one instance. Minuscule performance hit. It adds up to a bigger performance issue once you add more and more scripts each doing the same thing, especially on the same GameObject, or if you're instantiating multiple of the same prefab at the same time. I've run into performance issues like this before in more extreme cases.

Not in the above case, but sometimes you want to call a method on a script attached to a just instantiated prefab. It can be easy to forget that none of your component references are set yet because Start will not run until after the current frame. (yes you can use Awake instead, but sometimes you have reasons to not use Awake)

But if you're unconcerned with either of these, I can't argue with your position.