I need help tidying this "Player" code up, removing unnecessary code or implement better more optimized versions of them and lastly convert them into state machines if possible. im confused about it all cuz my ladderclimb, wall jump & dash don'twork

Player Script :

extends CharacterBody2D

enum PlayerState {
	IDLE,
	WALK,
	WALL_JUMP,
	WALL_SLIDE,
	CLIMB,
}

@export var movement_data : PlayerMovementData

@onready var dash = $Dash
@onready var animated_sprite_2d = $AnimatedSprite2D
@onready var timer = $Timer
@onready var starting_position = global_position 
@onready var wall_jump_timer = $WallJumpTimer
@onready var wall_slide_timer = $WallSlideTimer
@onready var wallslide = $wallslide
@onready var laddercheck = $LadderCheck

const GRAVITY := 600
const FALL_GRAVITY := 900
const DASH_SPEED = 450
const DASH_LENGTH = 0.1
const wall_jump_pushback = 1500
const wall_slide_gravity = 10

var state = PlayerState.IDLE
var on_ladder:bool = false
var health = 100
var health_max = 100
var health_min = 0
var can_dash = true
var dashing = false
var air_jump = false
var is_wall_sliding = false
var just_wall_jumped = false
var was_wall_normal = Vector2.ZERO
var gravity = ProjectSettings.get_setting("physics/2d/default_gravity")

func _ready():
	Events.playerBody = self
	#on_ladder = false

func get_gravity(velocity: Vector2):
	if velocity.y < 0:
		return GRAVITY
	return FALL_GRAVITY 

func _physics_process(delta):
	match state:
		
		PlayerState.IDLE:
			handle_idle_state(delta)
		PlayerState.WALK:
			handle_walk_state(delta)
		PlayerState.WALL_JUMP:
			handle_wall_jump_state(delta)
		PlayerState.WALL_SLIDE:
			handle_wall_slide_state(delta)
		PlayerState.CLIMB:
			handle_climb_state(delta)
			
	#if is_on_ladder():
		
	if not is_on_floor():
		velocity.y += get_gravity(velocity) * delta
			
	if Input.is_action_just_released("ui_up") and velocity.y < 0:
		velocity.y = movement_data.jump_velocity / 4
	#apply_gravity(delta)
	handle_jump()
	wall_slide(delta)
	handle_wall_jump()
	handle_dash()
	var input_axis = Input.get_axis("ui_left", "ui_right")
	handle_acceleration(input_axis, delta)
	handle_air_acceleraion(input_axis, delta)
	apply_friction(input_axis, delta)
	apply_air_resistance(input_axis, delta)
	update_animations(input_axis)
	var was_on_floor = is_on_floor()
	var was_on_wall = is_on_wall_only()
	if was_on_wall:
		was_wall_normal = get_wall_normal()
	move_and_slide()
	var just_left_ledge = was_on_floor and not is_on_floor() and velocity.y >= 0
	if just_left_ledge:
		timer.start()
	just_wall_jumped = false
	var just_left_wall = was_on_wall and not is_on_wall()
	if just_left_wall:
		wall_jump_timer.start()
	if input_axis:
		if dashing:
			velocity.x = input_axis * DASH_SPEED
		else:
			velocity.x = input_axis * movement_data.speed

func is_on_ladder():
	if not laddercheck.is_colliding(): return false
	var collider = laddercheck.get_collider()
	if not collider is Ladder: return false
	return true

func _input(event : InputEvent):
	if(event.is_action_pressed("ui_down") && is_on_floor()):
		position.y += 1

func handle_idle_state(_delta):
	return
func handle_walk_state(_delta):
	return
func handle_wall_jump_state(_delta):
	return
func handle_wall_slide_state(_delta):
	return
func handle_climb_state(_delta):
	return

#func apply_gravity(delta):
	#if !is_on_floor():
		#velocity.y += gravity * movement_data.gravity_scale * delta
		#
func handle_dash():
	if Input.is_action_just_pressed("dash") and can_dash and !is_on_wall:
		dashing = true
		can_dash = false
		$dash_timer.start()
		$dash_again_timer.start()
	
func handle_wall_jump():
	if not is_on_wall_only() and wall_jump_timer.time_left <= 0.0: return
	var wall_normal = get_wall_normal()
	if wall_jump_timer.time_left > 0.0: 
		wall_normal = was_wall_normal
	if Input.is_action_just_pressed("ui_up"):
		velocity.x = wall_normal.x * movement_data.speed
		velocity.y = movement_data.jump_velocity
		just_wall_jumped = true
		
	if Input.is_action_just_pressed("ui_up") and wall_normal == Vector2.RIGHT:
		velocity.x = wall_normal.x * movement_data.speed
		velocity.y = movement_data.jump_velocity
		
func wall_slide(delta):
	if is_on_wall() and !is_on_floor():
		if Input.is_action_pressed("ui_left") or Input.is_action_pressed("ui_right"):
			is_wall_sliding = true
		else:
			is_wall_sliding = false
		if is_wall_sliding:
			velocity.y += (wall_slide_gravity * delta)
			velocity.y = min(velocity.y, wall_slide_gravity)

func handle_jump():
	if is_on_floor(): air_jump = true
	if is_on_floor() or timer.time_left > 0.0:
		if Input.is_action_just_pressed("ui_up"):
			velocity.y = movement_data.jump_velocity
			timer.stop()
	elif not is_on_floor():
		if Input.is_action_just_released("ui_up") and velocity.y < movement_data.jump_velocity / 2:
			velocity.y = movement_data.jump_velocity / 2
			
		if Input.is_action_just_pressed("ui_up") and air_jump and not just_wall_jumped:
			velocity.y = movement_data.jump_velocity * 0.8
			air_jump = false
			
func handle_acceleration(input_axis, delta):
	if not is_on_floor() : return
	if input_axis:
		velocity.x = move_toward(velocity.x, movement_data.speed * input_axis, movement_data.acceleration * delta)

func handle_air_acceleraion(input_axis, delta):
	if is_on_floor(): return
	if input_axis != 0:
		velocity.x = move_toward(velocity.x, movement_data.speed * input_axis, movement_data.air_acceleration * delta)
	
func apply_friction(input_axis, delta):
	if input_axis == 0 and is_on_floor():
		velocity.x = move_toward(velocity.x, 0, movement_data.friction * delta)
		
func apply_air_resistance(input_axis, delta):
	if input_axis == 0 and not is_on_floor():
		velocity.x = move_toward(velocity.x, 0, movement_data.air_resistance * delta)
		
func update_animations(input_axis):
	if input_axis != 0:
		animated_sprite_2d.flip_h = (input_axis < 0)
		animated_sprite_2d.play("run")
	else:
		animated_sprite_2d.play("idle")
	
	if not is_on_floor():
		animated_sprite_2d.play("jump")

func _on_hazard_detector_area_entered(_area):
	global_position = starting_position

func _on_dash_timer_timeout():
	dashing = false

func _on_dash_again_timer_timeout():
	can_dash = true

image_2024-06-17_181024666

Player Movement Script :

class_name PlayerMovementData
extends Resource

@export var speed = 100.0
@export var acceleration = 800
@export var friction = 1000.0
@export var jump_velocity = -300.0
@export var gravity_scale = 1.0
@export var air_resistance = 200.0
@export var air_acceleration = 400.0

Boomerang Weapon Script :

extends Node2D

enum {IDLE, FLY, STICK}
@export var throw_speed = 2 * 60
@onready var parent: = get_parent()
var state: int = IDLE
var velocity: = Vector2.ZERO
var pos: = Vector2.ZERO
var spin_speed: float = 4*360
var player

#func _ready()->void:
	#idle_position()

func _physics_process(delta):
	match state:
		IDLE:
			idle()
		FLY:
			fly(delta)
		STICK:
			stick(delta)

func idle()->void:
	look_at(get_global_mouse_position())
	if Input.is_action_just_pressed("Click"):
		throw()

func fly(delta:float)->void:
	pos += velocity*delta #variable for disconnecting from parent movement
	global_position = pos
	#spin
	rotation_degrees += spin_speed*delta

func stick(delta:float)->void:
	#place for your solution
	var target: = get_target()
	var dist = global_position.distance_to(target)
	if dist < throw_speed * delta:
		idle_position()
	else:
		pos = pos.lerp(target, (throw_speed * delta)/dist)
	global_position = pos
	#spin
	rotation_degrees += spin_speed*delta

func throw()->void:
	state = FLY
	$Timer.start()
	velocity = (get_global_mouse_position() - global_position).normalized() * throw_speed
	pos = global_position #variable for disconnecting from parent movement

func idle_position()->void:
	
	state = IDLE
	global_position = get_target()
	

func get_target()->Vector2:
	return parent.global_position + Vector2(0,-2)

#func _on_Timer_timeout()->void:
	#state = STICK


func _on_timer_timeout()->void:
	state = STICK

ALSO : there is like 3 different functions that handle fall acceleration after jump but it should be only one…

Please surround your code with <code> to make it possible to read it properly and for the indentation to show up

1 Like

is it possible now?

Much more readable!

EDIT: These ended up out of order due to moderation. Please read my other reply first, then come back to this.

Oh, I should also mention, an individual state doesn’t have to be complicated.

Like I may have a DEAD state that in the state transition (changeState) method just does:

    match newState:
        [... other states ...]
        PlayerStates.DEAD:
            global_position = starting_position
            changeState(PlayerStates.IDLE)

Or something.
Or maybe it stays DEAD for one frame, and the handle_dead_state method just looks like:

    func handle_dead_state(_delta):
       changeState(PlayerStates.IDLE)

Where being dead unconditionally goes back to idle. Now you can transition to DEAD wherever, and not have to worry about what it means to be dead (deep, I know). But it also means that in the future you may want a dead animation or something. That’s fine, you would just play("dead") in the changeState method instead of transitioning to idle, and then have the animation finished callback change you to idle or respawn state or something. And all the places where you die stay the same. They don’t have to care how you get out of being dead, they just know this is how you get in.

Similarly, your wall-jump state may just reflect the player based on which way they’re facing, or which way the wall is, or whatever, and then transition right away into the normal jump state to handle the actual logic. Because once you’ve launched off the wall, it doesn’t matter why you jumped anymore. Or maybe it does, and it’s a similar but different state to normal jump. Again, both are options, and it’s up to what your game is like.

All I wanted to add was that you can use states that are really simple now, and take advantage of the state-transition concept, without feeling like you have to put a bunch of logic there to justify the state. It can be clear sometimes to see “Ah, right. Being dead is simply this and nothing else”.

1 Like

Okay! I’m not going to do the re-arranging for you, but I will give you some tips that may help.

The most important thing to note is that coding style is personal and situational, so the things I say here are just how I would do things if I was refactoring this, but other people may have completely different opinions. Things like State patterns are even more personal, so it’s even more likely you’ll get different suggestions.

Okay, so the first thing I’ve noticed is you have a lot of instance variables that could have might tighter scopes. Like, your is_wall_sliding is set in one conditional, and then used right after that in the same method, but nowhere else. This would be much clearer if the variable was local to that method, so we didn’t have to worry about where these things get used.

The next thing is the number of conditionals. Sometimes they can’t be avoided, but one of the advantages of the state pattern is that your code can be more focused, which is what helps it to be clear. So, here’s a technique that can be used to figure out why stuff isn’t working. We’re going to bring things back to nothing, and then rebuild. Take all the code in your _physics_process after the match state and comment them out. (If you didn’t know, you can highlight lines and hit Ctrl-K to comment lines out). Oh, also if you’re using version control maybe make sure you’ve got a commit before you start this refactor, or if you’re not using version control, make a copy of the file before refactoring it. (You should probably use version control though. Something to look into for later perhaps)

Now your character won’t do anything, but the code’s still there! It’s not gone, just inactive. Just because the character isn’t doing things anymore, it doesn’t mean we’ve lost progress or that we’re going backwards. We’re just moving forwards by temporarily turning bits off. Okay, so now they’re in the Idle state, and we want them to be able to walk. So in the handle_idle_state method, you’ve got to figure out if you should be walking or not. Looks like you do that by checking the input_axis, but then that’s it! You were idle, now you’re walking. You could be done this frame right here, or you could manually call handle_walk_state to get a head start on it before next frame, where your _physics_process will do that for you. Honestly, I’d probably just be done here and see if it makes a noticeable difference, just because it’s simpler. If you’re not walking, then you should play your idle animation from update_animations. Now everything related to being idle is in one place, the handle_idle_state method.

Now, you want walking to actually do something, and that’s good because we have the handle_walk_state for that! First thing to check would probably be if we’re still moving, and if not we go back to the idle state. Done! But if we are still moving, this is where the logic from handle_acceleration seems to go. Also the walking animation from update_animations can be copied in here. At this point you should have a character that can start walking, walk around, and stop walking again.

Now I’d go through and add some of your other logic back in, like jumping and dashing and falling. But try to add them in full chunks like we did here. Say you’re going to add jumping, add only the bits for jumping, testing along the way, until you feel like jumping is good. Then move on to dashing.

If you have some logic that’s in more than one state that’s either more than a few lines, or likely to change in the future, you could put that in a helper and then call it from whichever handle_ methods need it. The only things I’d put directly in _physics_process are things that are truly truly global to all the states. Like maybe gravity, but even that I could see being just a helper like apply_gravity() that gets called where it’s relevant. Like probably you don’t need to apply gravity if you’re idle, and depending on what kind of game you’re making, you may want to avoid it if you’re dashing, for example.

Sometimes you have to setup some stuff before you can process a state, like starting your dash timer. Doing that in more than one place kinda sucks. A pattern I use for that sometimes is something something like:

    func changeState(new_state):
        if new_state == state:
             return
    
        match new_state:
            [... other states ...]
            PlayerState.DASH:
                $dash_timer.start()
            [... other states ...]

        state = new_state

So now I never edit the state variable directly in the handle_ methods. They detect if I should be dashing, and then do changeState(PlayerState.DASH) to make the transition. And it’s always safe to call it, because it will just do nothing if we were already dashing. For example, at least.

I’ve noticed you don’t actually have a dash state. I might think about it. Again, one of the ways State machines can help the code is by replacing a nest of conditional statements with straightforward methods. Like, if your handle_dash_state doesn’t check if the dash action is pressed, then you can’t dash. You don’t need a dashing or can_dash variable, because it’s implied by the state you’re in. And then your timer is the way you exit that state. The timer method would probably check if you’re moving and then either changeState(PlayerState.IDLE) or changeState(PlayerState.WALK) or whatever. And since it seems like you can Dash while dashing, so maybe you could still have can_dash and conditionally re-dash, or maybe you could even have two states, DASH and REDASH or something. The first timer could change from DASH to REDASH, and the second timer could do what I said before.

That’s up to you and what you’re planning. Sometimes more states is actually more work and more confusing, and sometimes less. It’s unclear if this can_dash part is one or the other. If there’s a different animation that plays when a dash starts, or if the speed changes over time across the dash, that feels more likely to be a different state to me, because you want it to “restart” in some way, which feels like a state transition, but I could be wrong.

I may also have a state for jumping and falling, given that you have two different gravity values for those. So things like walking or idle would transition to falling if they’re not on the floor, but jumping if the jump button was pressed. I would set your initial upward velocity in the state transition to the jumping state, so you don’t have to copy that wherever. Just transition to jumping and the state machine will take care of the rest. And then the jumping state would transition to falling when the upward speed hits zero. And that again might be accompanied by a line in the changeState state-transition method that for example plays the falling animation, so the character looks different when going up versus down. Again, sometimes this fixes mess and cleans things up, and sometimes it’s too much State-pattern and makes it more complicated and harder to make changes. Up to your taste and comfort level.

And in general, the thing with states that you should be focused on is “how do I get into this state” and “how do I get out of it”, right? We’ve got a dash, and we’ve wrapped up all the dashing logic in one place. The thing I should be thinking is “when can I start dashing”, and I put changeState(PlayerState.DASH) there. And then once I’m in the dashing state, how can I get out of it? The timer is the obvious way. If I stop holding a direction, do I stop dashing, or keep dashing? Both are the right option for different kinds of games. Which kind of game is yours? Can I jump while I’m dashing? Does it stop dashing and do a normal jump? Or does it do some kind of dash jump?

But yeah, that’s how I’d approach it. Comment things out, go back to “basics”, and then build each state as a self-contained unit, one at a time. And try to keep the logic together when you can. You’ve got handle_acceleration and handle_air_acceleration right now, and those seem like prime candidates for just becoming specialized in the various handle_ methods. apply_friction and apply_air_resistance too. And then try to think through your variables. Do I really need this? If I do need this, do I need this up here, or can it be localized to just the one method that does need it. Or is this actually more of a state concern. That kind of thing.

All of that having been said, no pattern is perfect. It’s good practice, but try not to let perfect be the enemy of good. Sometimes a single boolean could save you from an entire refactor of the whole state model, and that’s worth it. The player doesn’t know about your state machines, just if you have bugs or not. It’s mostly for your own benefit, so keep that in mind as well.

Good luck!

1 Like

Thank you so much, I’m doing the back to basics now and things are looking good, just implemented the FSM. the player can move, jump, wall_jump and wall_slide. the animations and flip_h work well. still working on one way platform drop down and tweaking the numbers and adding accelerations to make it feel good.

This topic was automatically closed 30 days after the last reply. New replies are no longer allowed.