Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

changes to default characterbody scripts #66723

Closed

Conversation

cbscribe
Copy link
Contributor

@cbscribe cbscribe commented Oct 1, 2022

Updates to default scripts for CharacterBody2D/CharacterBody3D.

  • Apply gravity always, not just when in the air.
  • Fix non-working deceleration
    • Using SPEED as move_toward() factor just means stopping instantly
    • Set a value of FRICTION consistent with other values - result: stops in ~0.5 s
    • Apply delta when decelerating
  • Remove static typing hints
    • Default in godot-docs is no type hints
    • "Type Hints" defaults off in Editor Settings
  • Minor clarifications to comments.

@cbscribe cbscribe force-pushed the default_charbody_scripts branch from bafc150 to 2a9359f Compare October 2, 2022 02:12
@aaronfranke aaronfranke requested a review from fabriceci October 2, 2022 02:31
@Chaosus Chaosus added this to the 4.0 milestone Oct 2, 2022
@cbscribe cbscribe force-pushed the default_charbody_scripts branch from 2a9359f to aaa8345 Compare October 2, 2022 16:45
@fabriceci
Copy link
Contributor

I currently have my PC under repair so I can't currently do tests :(

I would avoid to change the logic of gravity as long as it doesn't fix any problems, because there is a good chance that this change will introduce jitters or other issues. For example, you will never move only to the right/left, but always diagonally, I am not sure that in some corners/slopes this does not cause small problems.

The current template has been pretty well tested and since this is a character controller, not a rigid body simulation, it does not have to strictly obey the physical rules.

Copy link
Contributor

@fabriceci fabriceci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to what I wrote about the gravity change, there are the same templates for c#, they must be synchronized: https://github.com/godotengine/godot/tree/master/modules/mono/editor/script_templates

@cbscribe
Copy link
Contributor Author

cbscribe commented Oct 3, 2022

I would avoid to change the logic of gravity as long as it doesn't fix any problems, because there is a good chance that this change will introduce jitters or other issues.

If anything this is changing it back to how it's always worked. In 3.x not applying constant gravity is the number one source of is_on_floor() issues - I have answered that one a ton of times on the Discord. As far as I've been able to determine, this is still the case in master.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 10, 2023
@cbscribe
Copy link
Contributor Author

I'd like to revisit this. As we have a lot more new users coming in, it's really important that the default script they'll see be a simple and straightforward one. Are we waiting for the C# changes to commit? I can make those, but I'd need someone to review, because I'm not running the .net version to test it on.

aaronfranke
aaronfranke previously approved these changes Feb 12, 2023
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM. Still needs C# changes. Not critical for 4.0 though.

@YuriSizov
Copy link
Contributor

Are we waiting for the C# changes to commit?

Well, there are concerns from physics maintainers above, and no resolution on that front after that. Would be nice to reach some consensus before making the change. (Having parity between languages should be important too).

@fabriceci
Copy link
Contributor

@cbscribe Sorry, I had completely forgotten about this PR!

Apply gravity always, not just when in the air.

move and slide has been completely rewritten in 4.0, so it works differently from 3.x. I determined by test that it was better to apply gravity only to the ground. I don't remember exactly but it seems to me that in 3D it causes a bit of jitter. I think we should avoid changing this unless you have/there is a concrete case of a bug that it could fix.

Fix non-working deceleration

  • Using SPEED as move_toward() factor just means stopping instantly
  • Set a value of FRICTION consistent with other values - result: stops in ~0.5 s
  • Apply delta when decelerating

I don't quite understand the changes because this PR removes the deceleration (so the two bottom points are false).
I don't think that removing deceleration is an improvement. Is there a problem in the PR?

  • Using SPEED as move_toward() factor just means stopping instantly

You are right.

IMO two things can be done to improve the script:

  • add a speed for deceleration
  • add an acceleration instead of directly reaching SPEED which would become MAX_SPEED.

(For the second point, opinions are welcome.)

@cbscribe
Copy link
Contributor Author

@fabriceci Thanks for the clarification. If it's the case that it works better, then I guess that's the way we'll do it now. It just feels wrong to be effectively turning gravity on and off all the time.

I'll revise to just deal with the acceleration/deceleration code.

@fabriceci
Copy link
Contributor

fabriceci commented Feb 22, 2023

@cbscribe @aaronfranke here is my suggestion, I tried to keep it very simple like it was but I added some options to make it more flexible, more ready to be used. It is necessary to tweak a bit the default values, maybe the run is too much (I don't know, opinions are welcome)

extends CharacterBody2D


@export var WALK_MAX_SPEED := 500.0
@export var WALK_ACCELERATION := 1500.0
@export var RUN_MAX_SPEED := WALK_MAX_SPEED * 1.3
@export var RUN_ACCELERATION := WALK_ACCELERATION * 1.3
@export var FRICTION := 1000.0
@export var SLIDE_FRICTION := 1000.0
@export var JUMP_VELOCITY := -400.0

# Get the global gravity from the project settings. RigidBody2D nodes also use this value.
var gravity: float = ProjectSettings.get_setting("physics/2d/default_gravity")


func _physics_process(delta: float) -> void:
	# The run action is not defined by default in the project, you must add it yourself.
	var running = Input.is_action_pressed("run")
	var acceleration := RUN_ACCELERATION if running else WALK_ACCELERATION
	var max_speed := RUN_MAX_SPEED if running else WALK_MAX_SPEED
	
	# Add the gravity.
	if not is_on_floor():
		velocity.y += gravity * delta

	# Allow jumping only when on the floor.
	if Input.is_action_just_pressed("ui_accept") and is_on_floor():
		velocity.y = JUMP_VELOCITY

	# Apply friction.
	velocity.x = move_toward(velocity.x, 0, FRICTION * delta)
		
	# Get the input direction and handle the movement.
	# As good practice, you should replace UI actions with custom input actions.
	var direction := Input.get_axis("ui_left", "ui_right")
	if direction:
		# If the direction is opposite to the x-velocity, extra friction is applied for more reactivity.
		if direction + sign(velocity.x) == 0: 
			velocity.x += SLIDE_FRICTION * direction * delta
		velocity.x = clampf(velocity.x + acceleration * direction * delta, -max_speed, max_speed)
	
	move_and_slide()

@cbscribe
Copy link
Contributor Author

Superseded by #73873

@YuriSizov YuriSizov removed this from the 4.1 milestone Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants