Here would be my corrected code (and my explanation is below):
using CustomInspector;
using UnityEngine;
public class PlayerController : MonoBehaviour
{
[SelfFill] public BoxCollider2D boxDetector;
[SelfFill] public Rigidbody2D playerBody;
//Layers
[SerializeField, Layer] private int groundLayer;
[SerializeField, Layer] private int wallLayer;
//Speeds
[Header("Speeds")]
[SerializeField, Min(0.001f)]
float maxSpeed = .7f;
[SerializeField, Min(0), Tooltip("How fast maxWalkSpeed will be reached")]
float walkHardness = 1;
[SerializeField, Range(1, 4), Tooltip("how much is running faster than walking")]
float runMultiplier = 2;
[SerializeField, Range(.1f, .9f), Tooltip("gets multiplied on speed if he is mid air")]
float airMultiplier = .25f;
[SerializeField]
float jumpForce = 10;
//Collide infos
[Min(0)] const float safeSpace = .5f;
[Min(0)] const float wallSpace = .2f;
// -
bool wallIsGrabbed = false; //rename from wallGrabbing
void Update()
{
Movement();
}
void Movement()
{
bool hitWall = Physics2D.BoxCast(boxDetector.bounds.center, boxDetector.bounds.size, 0f, Vector2.left, wallSpace, wallLayer).collider != null
|| Physics2D.BoxCast(boxDetector.bounds.center, boxDetector.bounds.size, 0f, Vector2.right, wallSpace, wallLayer).collider != null;
if(hitWall && (wallIsGrabbed || TryGrabWall()))
{
//Vertical Movement
wallIsGrabbed = true;
playerBody.velocity = Vector2.zero;
if(!EndGrabWall())
{
playerBody.gravityScale = 0;
}
else
{
wallIsGrabbed = false;
playerBody.velocity = new Vector2(0, -.8f);
}
}
else
{
bool hitGround = Physics2D.BoxCast(boxDetector.bounds.center, boxDetector.bounds.size, 0f, Vector2.down, safeSpace, groundLayer).collider != null;
//Horizontal Movement
playerBody.gravityScale = 1;
//Add force
float currentForce = hitGround ?
walkHardness
: walkHardness * airMultiplier;
float movDir = Input.GetAxis("Horizontal");
if (Mathf.Abs( playerBody.velocity.x ) < maxSpeed)
playerBody.AddForce(new Vector2( movDir * currentForce, 0));
//SpriteHandling
Debug.LogWarning($"Sprite should change its facing based on {nameof(movDir)}");
//Jump
if (TryJump())
{
playerBody.AddForce(new Vector2(0, jumpForce));
}
//Crouch
if(Input.GetKey(KeyCode.S))
{
Debug.LogException(new System.NotImplementedException("Crouching is not implemented yet"));
}
}
}
bool TryJump() => Input.GetKeyDown(KeyCode.W);
bool TryGrabWall() => Input.GetKey(KeyCode.Space);
bool EndGrabWall() => Input.GetKey(KeyCode.S);
}
this would look in the inspector like this (you can edit there only variables you can edit):
explanation:
first what a good compiler would tell you:
- You don’t need line 42, because after that you test whether w was just pressed
- line 16 float test; public GameObject myPlayer; both are unused variables
- Line 19, 20 you can make const/readonly
- Line 46 you accidentally used Vector3 instead of Vector2
I would work on the readability, so you too will find mistakes faster and think in a more structured way: Comments that explain what you do or at least you capsulate your requirements in functions. e.g. in the update function you call a method named MoveAround(). In this method I would build on an if query,
which is called
if(OnWall()) { <wallbehaviour> } else { <horizontal movement> }
because they are two completely different behaviors
I would not invert the transform to just invert the sprite. Make a right-looking and a left-looking sprite - then you don’t have such problems with the transform direction. Changing the sprites is still missing with you anyway.
in line 77 you test whether left and right wall touches - i think you mean an ‘or’
magic numbers in the code is an anti-pattern
better assign them above. you write eg
speed = .5f or playerBody.velocity = new Vector2(0, -.8f);
You could also create a variable which is wallSlideSpeed just like you did for jumpspeed
You should take a look at the unity attributes:
If wallSlideSpeed should then be in the range of .5f to .8f, then write that down as well.
Attributes are important for hints. [Range(.5f, .8f)] float wallSlideSpeed only allows special values in the inspector
I would also do things like Speed with
[Min(.001f)] float speed = 1;
limit because you don’t want negative values.
as a question for you: Do your public fields really have to be public? Wouldn’t a ‘[SerializeField] BoxCollider2D boxDetector’ do the same thing?
You should only set stuff public if you want to get it from other scripts.
Why do you set jumpSpeed to 10 in Start() and not right at the top of the field like you did with eg wallSpace.
Anything you do at runtime is performance technically worse.
@AndreaMar has already referred to character controllers. I personally think there are a lot of unnecessary features in it, which is why I personally don’t use it. What I
The new input system is often used instead. This already has an axis evaluation of the keyboard and you get a direction.
An alternative is of course to move the character with animations and root motion anyway,
but that is certainly going too far.
He also addressed trigger. I like the idea of looking at unity’s collision detection. Your normal BoxCollider already has an OnCollisionEnter, which gives you information about all collisions.
In this you can also check the collider object for a tag instead of a layer. You can also read out the direction of the collisions and much more.
But I will ignore that in the following to explain to you further what I would improve on the code if you go the same way
Since you don’t actually use the physical casts, just checking if they took place, I would prefer to store a boolean value as well
I can also recommend ‘Custom Inspector’:
So I would set playerBody = GetComponent() beforehand. GetComponent is slow and
you can also assign the variable in the inspector. I see your point though (which is probably why you did that),
that afterwards it is unclear in the inspector which values belong in there.
For example, I can [SelfFill] recommend, this searches in its own object automatically in the editor and not runtime with GetComponent<> for the reference
Then you never have to worry about these types of assignments again
LayerMask wallMask is definitely only one layer and not a mask, i.e. several:
So you can really only select one layer with [Layer] int wallMask in the Inspector per enum selection.
If you want jumpSpeed always to be a 10 you can set it readonly by attribute, too.
[ReadOnly] public float jumpspeed = 10;
Then you know, at least in the inspector, that changes are not applied
Custom Inspector attributes are part of Asset (Custom Inspector | Utilities Tools | Unity Asset Store)
