Feedback

[dreamy dune]

Your variable names are very inconsistent and there's a lot of unnecessary whitespace making the script bigger than it needs to be

[fervent igloo]

Yes ik i am not home rn but its not fully ninished its just in a working state

I stil have a lot of fine tuning to do

[dreamy dune]

I'm going to fork it and make some stylistic changes so you can see maybe how you could format it better

[fervent igloo]

That great thanx

[dreamy dune]

I haven't seen anything bad/logically incorrect yet though so thats good

[fervent igloo]

Yay

[fervent igloo]

Actually what is fork it?

Found it

Didn't know that was a thing

[dreamy dune]

why do you start these identifiers with _

[charred merlin]

I think thats the default naming convention made by microsoft

[dreamy dune]

I don't know of any naming convention that asks you to start identifier names of _

[charred merlin]

According to them, private fields should start with underscore. Unity devs tend to just skip that

[dreamy dune]

hm

not sure why

seems kind of messy

[charred merlin]

Ik and thats why we don’t use that 😄

[dreamy dune]

ig something with intellisense?

so you don't accidentally use a private variable ig

[fervent igloo]

Actually a friend helped me make some parts and he renamed it with _ bc they where all public before i made them private bc i like that more

[dreamy dune]

well then that makes even less sense

[fervent igloo]

Well ill change it when i get home hehe

[dreamy dune]

    [SerializeField] private bool isGrounded = false;





    // private statements

there's also like a 6 line gap between the last field and the comment

there shouldn't ever be a more than one line gap

[fervent igloo]

Oh not all the _ where made by him its bc he did it i thought it was necessary to do or something

[dreamy dune]

no, the names of your variables dont ever affect what they do

unless they use reserved keywords

[fervent igloo]

I need 2 line gap so it is organized i have a reading disability

[charred merlin]

Its just easier to read code if everyone uses same type of naming convention

[dreamy dune]

There's also just a bunch of comments and things with inconsistent capitalization which just looks kinda bad

but ig it doesn't really go against a standard

however the variable names with inconsistent capitalization is bad

[fervent igloo]

Well it isnt finished it are just notes for myself

[dreamy dune]

what about the tooltips though

Headers*

[fervent igloo]

What about it?

[dreamy dune]

they have inconsistent capitalization, just a nitpick though

[fervent igloo]

I thought they looked ok in the unity

[dreamy dune]

"G" in "Ground checker" wasn't capitalized

like i said, just a nitpick

[fervent igloo]

Where is inconsistent capitalisation

[dreamy dune]

[Header("ground c...
vs
[Header("Fall speed...

[fervent igloo]

Unity automatically does a capital at the start in the editor thingy

[dreamy dune]

well ig it just looks bad in the code then lol

[fervent igloo]

And i like that the first letter in not capital if i have to write it a bunsh of times

[dreamy dune]

the double newline thing ig is fine if you keep it consistent (which you dont)

[fervent igloo]

Well i made the code in a way so you never have to look in the cofr

[fervent igloo]

Ik ik

Hey just to be fair i only learned cs 4 days ago ok so i think i did pritty well

Buttttttt ill ajust the problems jou just said so it looks better and all

[dreamy dune]

       if (_movementDirection > 0 && !_isFacingRight)
        {
            FlipPlayer();
        }
        else if (_movementDirection < 0 && _isFacingRight)
        {
            FlipPlayer();
        }

this is redundant

you only need one if statement for this

[fervent igloo]

And what is the # reagon debug bc idk what it means tbh

[dreamy dune]

       if ((_movementDirection > 0 && !isFacingRight) || (movementDirection < 0 && isFacingRight))
        {
            FlipPlayer();
        }

[fervent igloo]

Yes while that looks better now i do not understand that piece of code

[dreamy dune]

its literally the same thing but with an OR gate

[fervent igloo]

I dont know how to ise ||

[dreamy dune]

|| means "or"

[fervent igloo]

Oh ok

[dreamy dune]

so if the movement direction is more than 0 and not facing right OR movement direction is less than 0 and is facing right

[fervent igloo]

Oh ill make sure to use it

Oh gtg thanx for the help btw :)))

[dreamy dune]

I don't think the code I'll push will be useable out of the gate so you'll probably have to manually make changes

There's no point to initializing fields with their default values

bool foo = false is unnecessary

since booleans are false by default

Vector3 velocity does not need to be initalized as Vector3.Zero

        if (isGrounded)
        {
            hasDoubleJump = true;
        }
``` is redundant

hasDoubleJump = isGrounded makes more sense

ok yeah

isJumping = Input.GetButtonDown("Jump") && isGrounded;

[charred merlin]

@fervent igloo only logic issue I found by fast glance was this: currently you’re checking for all colliders inside the box area and only checking whether atleast one of those is not the player itself. If WhatIsGround layermask is set up correctly, you should never get the player itself as result. Checking the ground using single OverlapBox (you can check whether the result is null or not) should be enough. OverlapBoxAll just makes useless memory allocation every time you call it and iterating through the list of colliders adds some small overhead too. Same applies for the OverlapCircleAll.

[dreamy dune]

oh yes thats gonna be slow

just got to that part

for loops on update are not good

especially when you're creating arrays

[charred merlin]

Well I can’t say for loops are automatically bad but ofc every pointless calculations should be avoided

[dreamy dune]

        if (groundCheckObject != null && boxCheck == true)
        {
            Gizmos.color = Color.red;
            Gizmos.DrawWireCube(groundCheckObject.position, new Vector3(boxWidth, boxHeight, 0f));

        }
        if (groundCheckObject != null && cirleCheck == true)
        {
            Gizmos.color = Color.red;
            Gizmos.DrawWireSphere(groundCheckObject.position, circleSize);

        }

this could be a style problem but comparing with == true is redundant

setting the gizmoz.color to red inside of the if statements twice is also redundant

gizmos*

[dreamy dune]

#region is used for organization

its so you can collapse and navigate through code quickly

[charred merlin]

I can’t just imagine how many for loops unity is using per every single frame intenrally… even OverlapBox is most certainly using for loop to iterate through list of bounds and colliders to get the result. Some of the unitys internal for loops may happen on c++ side so it may be bit faster but my point is that you don’t need to be afraid of using for loops on update. You can use them always you have to but massive loops (which this obviously isn’t, the memory allocation is probsbly the biggest issue here) are obviously not good for performance

[fervent igloo]

I dont understand lol ill look in to it

So not boxall but just box?

[fervent igloo]

Cool

[charred merlin]

yes. that gives you whether the box hits any object on WhatIsGround (which would be better named as groundLayerMask or something like that) layermask and if it does, it gives the first object it hits as a result. if *OverlapBox gives you null, it didn't hit any object on those layers

[fervent igloo]

Ah i like the way it is ill optimize a couple things but some words like groubdLayerMask doesn't make sense in my head so i avoid those words

[fervent igloo]

I have some trouble reading this could you rephrase it please? My language is not english

[charred merlin]

mine is neither which may make my messages nothing but clear

[fervent igloo]

Ohhhk?

[charred merlin]

well, let's start by this: what is the point of this checking? (the gameObject itself should never be on that collider array because I assume WhatIsGround layermask doesn't include the players layer) ```cs
if (colliders[i].gameObject != gameObject)
{
_isGrounded = true;
}

[fervent igloo]

My friend made the ground check

Ill ask him

How do you make it in one of those boxes the code i mean

In discord

[charred merlin]

you just use 3 backticks on both sides of the code + cs to indicate that you want c# syntax highligting

this is called code block

[fervent igloo]

backticks?

[charred merlin]

`

[fervent igloo]

'''
if (colliders[i].gameObject != gameObject)
{
_isGrounded = true;
}
'''

[charred merlin]

` not '

[fervent igloo]

where on my keyboard i am blind :)

found it

thangs

[fervent igloo]

if i remove All it gives me an error

[charred merlin]

you can't just remove it

[fervent igloo]

oh ok

so how do i fix it

[charred merlin]

So this ⬇️ check is pointless because colliders[i].gameObject will never be same as gameObject due to the fact your OverlapBoxAll uses layermask (WhatIsGround) which doesn't include the player itself.

if (colliders[i].gameObject != gameObject)
{
    _isGrounded = true;
}

Because colliders[i].gameObject != gameObject is always true, the whole if check is worth nothing and therefore the code is same as this:

for (int i = 0; i < colliders.Length; i++)
{
    _isGrounded = true;
}

That means _isGrounded is true always except when colliders.Length is 0 and therefore the for loop doesn't run once. That means your code is exactly same as this ⬇️

Collider2D[] colliders = Physics2D.OverlapBoxAll(GroundCheckObject.position, new Vector2(boxWidth, boxHeight), 0f, WhatIsGround);
if (colliders.Length != 0)
{
    _isGrounded = true;
}

So basically what your code is doing, is to check whether OverlapBoxAll hits any colliders at all. You don't need OverlapBoxAll to check that because OverlapBox exists. OverlapBoxAll returns list (it's technically an array) of all colliders the given box hits. OverlapBox on the other hand returns only the first collider it hits and null (basically in c# null means same as "nothing") in case it doesn't hit anything. Sooo finally you could just simplify your code to this:

Collider2D collider = Physics2D.OverlapBox(GroundCheckObject.position, new Vector2(boxWidth, boxHeight), 0f, WhatIsGround);
//checks whether OverlapBox hit something on WhatIsGround layer. if so, grounded can be set true
if (collider != null)
{
    _isGrounded = true;
}

[fervent igloo]

oh lord

thats a lot of text hehe

the friend who made it is explaining it to me now ill show you how it looks when i am finished

thanx for the long message it helps a lot