koobas.hobune.stream
Tips for having proper function structure
Hi there! I'm working on my first project currently and am having a lot of fun with it. I have been able to successfully get it to work like I want it to so far. The idea is that you're able to control a motorcycle by driving it forward/backward (Up/Down arrows), turn the bike left and right with those arrow keys, and go at a faster speed by holding down the shift button. The bike will also tilt if you're driving and turning at the same time. I have a separate script that is making the camera follow behind the bike, but here's the code for the bike itself:
using System.Collections;
using System.Collections.Generic;
using UnityEngine;
public class BikeScript : MonoBehaviour
{
public Transform bodyTransform;
public Transform wheelTransform1;
public Transform wheelTransform2;
public float moveSpeed = 30f;
public float normMoveSpeed = 30f;
public float turboMoveSpeed = 45f;
public float dirRotSpd = 80f;
public float normDirRotSpd = 80f;
public float turboDirRotSpd = 160f;
public float leanAmount = 30f;
public float leanSpeed = 10f;
// Start is called before the first frame update
void Start()
{
bodyTransform.parent = transform;
wheelTransform1.parent = bodyTransform;
wheelTransform2.parent = bodyTransform;
}
// Update is called once per frame
void Update()
{
PlayerMovement();
}
void PlayerMovement()
{
//Gets input
float hor = Input.GetAxisRaw("Horizontal");
float ver = Input.GetAxisRaw("Vertical");
//Checking for if the shift key is held down
if (Input.GetKey(KeyCode.LeftShift))
{
moveSpeed = turboMoveSpeed;
dirRotSpd = turboDirRotSpd;
}
else
{
moveSpeed = normMoveSpeed;
dirRotSpd = normDirRotSpd;
}
//Moves the player forward/backwards
Vector3 playerMovement = new Vector3(0f, 0f, ver) * moveSpeed * Time.deltaTime;
transform.Translate(playerMovement, Space.Self);
//Turns the player left/right
Vector3 playerTurn = new Vector3(0f, hor, 0f) * dirRotSpd * Time.deltaTime;
transform.Rotate(playerTurn, Space.Self);
//Leans the bike in the direction it's turning while moving
Quaternion currentLean = bodyTransform.rotation;
Quaternion normLean = transform.rotation;
Vector3 v = bodyTransform.rotation.eulerAngles;
if (hor != 0 && ver != 0)
{
if (hor == 1)
{
Quaternion desiredLean = Quaternion.Euler(v.x, v.y, -leanAmount);
bodyTransform.rotation = Quaternion.Slerp(bodyTransform.rotation, desiredLean, leanSpeed * Time.deltaTime);
} else {
Quaternion desiredLean = Quaternion.Euler(v.x, v.y, leanAmount);
bodyTransform.rotation = Quaternion.Slerp(bodyTransform.rotation, desiredLean, leanSpeed * Time.deltaTime);
}
} else {
bodyTransform.rotation = Quaternion.Slerp(bodyTransform.rotation, normLean, leanSpeed * Time.deltaTime);
}
}
}
Like I said, it works how I want it to. My concern at the moment is I feel like there's too much going on in the PlayerMovement function. I feel like ideally I would break it up into functions for PlayerInput, BikeMovement, and BikeLean. I wanted to ask some more experienced programmers how they would suggest breaking this code up? Do you think it's reasonable to have them in one function? Multiple functions? Maybe even separating them into there own scripts? If I were to separate the player input from the other stuff going on in the PlayerMovement function, I don't know how I would get those numbers to the other functions. I might try and have the PlayerInput function return those values to the other functions, but I don't know how I would return multiple numbers like that. Anyways, any input on this is appreciated! And if there's anything about the code that I could change about the code for the better, let me know about that as well. Thanks in advance!
Honestly, this is perfectly fine. When it comes to formatting, so long as it is still readable it just comes down to personal preference. In my case, I find it easier to read code in large blocks and better to separate code when functionality calls for it rather than needlessly, but others may find it easier to group specific parts. Hell, in your case I would even ask why the movement code needs to be in its own function at all - if it isn't going to be called from anywhere else you might as well just keep it all in the update function. Something to note is that, depending on the context, function calls can actually be of detriment to the performance of the program (negligibly in most cases, but still another reason not to split things up for the sake of it, imo). If you want to make separate sections for readability purposes, (at least in visual studio) you can use the #region directive - this doesn't actually change the code, it's just more of a note for the IDE to group things together.
https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/preprocessor-directives/preprocessor-region
Aside from that, the only thing really I would note in your code is this;
if (hor == 1)
{
Quaternion desiredLean = Quaternion.Euler(v.x, v.y, -leanAmount);
bodyTransform.rotation = Quaternion.Slerp(bodyTransform.rotation, desiredLean, leanSpeed * Time.deltaTime);
} else {
Quaternion desiredLean = Quaternion.Euler(v.x, v.y, leanAmount);
bodyTransform.rotation = Quaternion.Slerp(bodyTransform.rotation, desiredLean, leanSpeed * Time.deltaTime);
}
Both sides of the branch are identical aside from the lean amount, and there's no better programming than lazy programming, so we try not to repeat ourselves where we don't have to. You can replace the whole thing with a single ternary operator;
Quaternion desiredLean = Quaternion.Euler(v.x, v.y, (hor == 1) ? -leanAmount : leanAmount);
bodyTransform.rotation = Quaternion.Slerp(bodyTransform.rotation, desiredLean, leanSpeed * Time.deltaTime);
Also keep in mind that 'hor' is an axis input, meaning it can be anywhere between [-1,1], so if you are rotating based on left/right input, it might make more sense to use 'hor > 0' rather than 'hor == 1'.
Hope that helps, keep up the good work :)
Actually GetAxisRaw states on it's wiki page "keyboard input will always be either -1, 0 or 1".
For keyboard input yes, but the input system extends beyond just the keyboard, so it's always good practice to support as much as you can (especially when it's a simple change that doesn't break what already works).
Thinking about it, seeing as we are working with axis' you could also just do this and have it work better with more input systems too;
Quaternion desiredLean = Quaternion.Euler(v.x, v.y, -leanAmount * hor);
bodyTransform.rotation = Quaternion.Slerp(bodyTransform.rotation, desiredLean, leanSpeed * Time.deltaTime);
I generally agree with this. The only way I would take it a step further is if the code were meant to be more reusable and the Player$$anonymous$$ovement() method was separated into the concerns of both handling input and adjusting the transform separately.
Good call on folding the code; usually when I see several lines repeated with mainly the same parameters, but one change based on a logical condition, it is an indication that it is a candidate for reducing code size and still be readable.
@ShadyProductions is correct in the nitpick about the GetAxisRaw() returning -1, 0 or 1 for keyboard input, but your advice is still sound and will still work if the developer wants to use other input devices such as a game controller. Your suggestion handles a broader scope as opposed to one particular case.
Each script should be responsible for one task, in this case it's the bike movement (which is fine). Then each method should be responsible for one task that comes together to build the whole class.
The only real thing I can see that you could separate is the movement and the rotation. You could also split out the resetting of the speed if you wanted to but probably not needed in this case really. Currently, when you let go of Left Shift and the speed reaches normSpeed, it is constantly setting itself to norm speed.
private void Update()
{
if (Input.GetKey(KeyCode.LeftShift)
{
PlayerMovement();
}
// If the moveSpeed has reached the norm speed, stop setting it
// The 'return' means the code will stop there if the condition is met
if (moveSpeed <= normMoveSpeed && dirRotSpd <= normDirRotSpd)
return;
moveSpeed = normMoveSpeed;
dirRotSpd = normDirRotSpd;
}
You could separate the rotation to a BikeRotation method, then call this from the PlayerMovement method or in the Update after you call the PlayerMovement(). Whichever you think seems to make more sense.
using UnityEngine;
public class BikeScript : MonoBehaviour
{
[SerializeField] private Transform bodyTransform;
[SerializeField] private Transform wheelTransform1;
[SerializeField] private Transform wheelTransform2;
[SerializeField] private float moveSpeed = 30f;
[SerializeField] private float normMoveSpeed = 30f;
[SerializeField] private float turboMoveSpeed = 45f;
[SerializeField] private float dirRotSpd = 80f;
[SerializeField] private float normDirRotSpd = 80f;
[SerializeField] private float turboDirRotSpd = 160f;
[SerializeField] private float leanAmount = 30f;
[SerializeField] private float leanSpeed = 10f;
private void Start()
{
bodyTransform.parent = transform;
wheelTransform1.parent = bodyTransform;
wheelTransform2.parent = bodyTransform;
}
private void Update()
{
if (Input.GetKey(KeyCode.LeftShift))
{
PlayerMovement();
}
if (moveSpeed <= normMoveSpeed && dirRotSpd <= normDirRotSpd)
return;
moveSpeed = normMoveSpeed;
dirRotSpd = normDirRotSpd;
}
private void PlayerMovement()
{
moveSpeed = turboMoveSpeed;
dirRotSpd = turboDirRotSpd;
var hor = Input.GetAxisRaw("Horizontal");
var ver = Input.GetAxisRaw("Vertical");
var playerMovement = new Vector3(0f, 0f, ver) * (moveSpeed * Time.deltaTime);
transform.Translate(playerMovement, Space.Self);
PlayerRotation(hor, ver);
}
private void PlayerRotation(float horizontal, float vertical)
{
var playerTurn = new Vector3(0f, horizontal, 0f) * (dirRotSpd * Time.deltaTime);
transform.Rotate(playerTurn, Space.Self);
var normLean = transform.rotation;
var v = bodyTransform.rotation.eulerAngles;
if (horizontal != 0 && vertical != 0)
{
if (horizontal == 1)
{
var desiredLean = Quaternion.Euler(v.x, v.y, -leanAmount);
RotateBody(desiredLean);
}
else
{
var desiredLean = Quaternion.Euler(v.x, v.y, leanAmount);
RotateBody(desiredLean);
}
}
else
{
RotateBody(normLean);
}
}
private void RotateBody(Quaternion lean)
{
bodyTransform.rotation = Quaternion.Slerp(bodyTransform.rotation, lean, leanSpeed * Time.deltaTime);
}
}
Notes
It's best practice to have your fields private unless you need to be able to access them from other scripts. In this case, I don't think you should need to. By adding the [SerializeField] attribute, it means you can still see and change them in the inspector but hides them from other classes. If you do need to modify them from another class, you should probably introduce a public method to change their value.
The "currentLean" was not being used, which could have been a mistake? But I removed it.
You can use "var" instead of explicitly using each type, that's a personal preference.
You used the same line of code 3 times for the bodyTransform.rotation, when you are seeing repeated code, it's time to think about introducing a method for that line of code.
Added brackets around the playerMovement/playerTurn multiplication, which might not be necessary but it makes it much easier to read which multiplication is happening first
The rotation method feels like it could be better, the nested if statement specifically. I don't want to change it and break it though, but for the most part, I feel like there's some improvements there and maybe that's something to consider down the line.
