-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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
Improve Godot character controller template #73873
base: master
Are you sure you want to change the base?
Improve Godot character controller template #73873
Conversation
modules/gdscript/editor/script_templates/CharacterBody3D/godot_character_controller.gd
Outdated
Show resolved
Hide resolved
modules/gdscript/editor/script_templates/CharacterBody3D/godot_character_controller.gd
Outdated
Show resolved
Hide resolved
I think this makes sense to add, as this lets you test reduced walking speeds without having a controller (or tapping keyboard keys repeatedly). This has an impact when testing AnimationTree blending, for instance. |
modules/gdscript/editor/script_templates/CharacterBody3D/godot_character_controller.gd
Outdated
Show resolved
Hide resolved
|
||
func _ready() -> void: | ||
Input.set_mouse_mode(Input.MOUSE_MODE_CAPTURED) | ||
spring_arm.position = Vector3(0, 1, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The camera-offset could also be a exported (default Vector3(0, 1, 5)) to control the spring arm position
(xy) and spring_length
(z).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had done it at first, then I had deleted it because when you replace for your own camera (which will be ultimately the case), these properties will become useless.
I'm of mixed minds about the addition of the camera in 3D. For a lot of users, this is going to be the default script that they use for a character body. I feel like it should be as generic as possible, in order to allow for modification to suit different needs. On the plus side, it does reduce the number of steps needed to get a working 3d character up and running. |
modules/gdscript/editor/script_templates/CharacterBody2D/godot_character_controller.gd
Outdated
Show resolved
Hide resolved
modules/gdscript/editor/script_templates/CharacterBody3D/godot_character_controller.gd
Outdated
Show resolved
Hide resolved
Once we've locked in the code changes, let's make sure to take one last look at the comments to make sure they're clear and match the code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If my suggested changes are done and camera movement is a separate script, then I would suggest renaming the template back to basic movement.
modules/gdscript/editor/script_templates/CharacterBody3D/godot_character_controller.gd
Outdated
Show resolved
Hide resolved
modules/gdscript/editor/script_templates/CharacterBody3D/godot_character_controller.gd
Outdated
Show resolved
Hide resolved
# Moves the camera according to the movement of the cursor. | ||
elif Input.get_mouse_mode() == Input.MOUSE_MODE_CAPTURED and event is InputEventMouseMotion: | ||
rotate_y(-event.relative.x * mouse_sensitivity) | ||
spring_arm.rotate_x(-event.relative.y * mouse_sensitivity) | ||
spring_arm.rotation.x = clamp(spring_arm.rotation.x, deg_to_rad(camera_min_vertical_angle), deg_to_rad(camera_max_vertical_angle)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my previous comment here when the templates were initially added #53957 (review)
I think that camera movement should be a separate script that you attach to a child SpringArm node for the player's head. That way the camera movement is self-contained in a separate script. Bonus: We could make a separate version for Camera3D so that we have 3 scripts: a character movement script, an FPS camera script, and a TPS camera script. For left/right rotation, it could either rotate the parent character body, or it could rotate itself, we could pick one or maybe support both.
Thank you for your feedback. I see that the point of discussion is the camera. I think the best approach would be to have a scene as a template, unfortunately this is beyond the scope of the PR, but I think is the way to go ans should be implemented in the future. So the options for now:
I think that the first one has more advantage while waiting for a stage system. But as always, opinions are good to make a decision. |
7217ed4
to
01ba08a
Compare
updated according to comments |
fc0600e
to
7f94a2a
Compare
I never figured out how to use spring arm correctly, so I'd prefer if the template contained it. |
7f94a2a
to
381b0a1
Compare
modules/gdscript/editor/script_templates/CharacterBody3D/godot_character_controller.gd
Outdated
Show resolved
Hide resolved
modules/gdscript/editor/script_templates/CharacterBody3D/godot_character_controller.gd
Outdated
Show resolved
Hide resolved
381b0a1
to
1d1a016
Compare
@cbscribe Do you have any suggestions for comments? |
modules/gdscript/editor/script_templates/CharacterBody2D/godot_character_controller.gd
Outdated
Show resolved
Hide resolved
Co-authored-by: cbscribe <[email protected]>
1d1a016
to
1d86cb8
Compare
Doesn't applying gravity only when airborne break the property floor_stop_on_slope since gravity wouldn't apply any downward velocity while on a sloped floor? I think not applying gravity while grounded only sidesteps a greater issue with move_and_slide, #85971. |
var direction := (transform.basis * Vector3(input_dir.x, 0, input_dir.y)).normalized() | ||
if direction: | ||
_current_acceleration += acceleration | ||
_current_acceleration = clamp(_current_acceleration, 0, max_acceleration) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not clampf()
?
velocity.x = move_toward(velocity.x, 0, friction * delta) | ||
|
||
# Get the input direction and handle the movement. | ||
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
velocity.x = move_toward(velocity.x, 0, friction * delta) | |
# Get the input direction and handle the movement. | |
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 = move_toward(velocity.x, 0.0, friction * delta) | |
# Get the input direction and handle the movement. | |
var direction: float = Input.get_axis(&"ui_left", &"ui_right") | |
if direction != 0.0: | |
# If the direction is opposite to the x-velocity, extra friction is applied for more reactivity. | |
if direction + signf(velocity.x) == 0.0: |
Use float literals for floats, use float-specific functions over general ones, and follow the GDScript style guide of explicitly writing the type when it is not present on the same line.
@export var walk_acceleration := 15.0 | ||
@export var walk_max_acceleration := 300.0 | ||
@export var run_acceleration := walk_acceleration * 1.5 | ||
@export var run_max_acceleration := walk_max_acceleration * 1.5 | ||
@export var friction := 200.0 | ||
@export var jump_velocity := 5.0 | ||
@export_group("Camera") | ||
@export var mouse_sensitivity := 0.005 | ||
@export var camera_max_vertical_angle := 30 | ||
@export var camera_min_vertical_angle := -10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@export var walk_acceleration := 15.0 | |
@export var walk_max_acceleration := 300.0 | |
@export var run_acceleration := walk_acceleration * 1.5 | |
@export var run_max_acceleration := walk_max_acceleration * 1.5 | |
@export var friction := 200.0 | |
@export var jump_velocity := 5.0 | |
@export_group("Camera") | |
@export var mouse_sensitivity := 0.005 | |
@export var camera_max_vertical_angle := 30 | |
@export var camera_min_vertical_angle := -10 | |
@export var walk_acceleration: float = 15.0 | |
@export var walk_max_acceleration: float = 300.0 | |
@export var run_acceleration: float = walk_acceleration * 1.5 | |
@export var run_max_acceleration: float = walk_max_acceleration * 1.5 | |
@export var friction: float = 200.0 | |
@export var jump_velocity: float = 5.0 | |
@export_group("Camera") | |
@export var mouse_sensitivity: float = 0.005 | |
@export_range(-90, 90, 0.001, "radians_as_degrees") var camera_max_vertical_angle: float = 1.5 | |
@export_range(-90, 90, 0.001, "radians_as_degrees") var camera_min_vertical_angle: float = -1.5 |
There's no reason to prevent users from entering an angle of 30.5 degrees, and deg_to_rad
accepts a float anyway, so it's only used in a float context. Also, why were the default values so restrictive? You were only allowing the camera to look 10 degrees down before?
However, we should really use @export_range
here to store the value in radians, and avoid a runtime conversion step. https://docs.godotengine.org/en/stable/classes/[email protected]
_current_acceleration = move_toward(_current_acceleration, 0, friction * delta) | ||
velocity.x = move_toward(velocity.x, 0, friction * delta) | ||
velocity.z = move_toward(velocity.z, 0, friction * delta) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_current_acceleration = move_toward(_current_acceleration, 0, friction * delta) | |
velocity.x = move_toward(velocity.x, 0, friction * delta) | |
velocity.z = move_toward(velocity.z, 0, friction * delta) | |
_current_acceleration = move_toward(_current_acceleration, 0.0, friction * delta) | |
velocity.x = move_toward(velocity.x, 0.0, friction * delta) | |
velocity.z = move_toward(velocity.z, 0.0, friction * delta) |
var input_dir := Input.get_vector(&"ui_left", &"ui_right", &"ui_up", &"ui_down") | ||
var direction := (transform.basis * Vector3(input_dir.x, 0, input_dir.y)).normalized() | ||
if direction: | ||
_current_acceleration += acceleration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this acceleration change framerate dependent?
Here is And here is That's typically aligned with @expikr proposition in #81642 Edit: I just realized the original template doesn't use |
Superseed #66723 (as I added the comment changes)
This PR aims to improve the default template of the Character Controller by making it ready for use.
When I added the first template for the character body, the goal was to show how to use
move_and_slide
correctly, in a simple way, since it was often a source of issues.The script contains some problems:
Moreover, the script was too simple, It was not good enough to be used as it is. For example, I saw a video tutorial that explained how to make a character controller starting from the characterController template, this is what makes the template less interesting.
This version aims to have the best possible character controller while remaining simple and short.
New features:
Next steps:
Showcase:
Add a ground, a CharacterBody and attach the template:
showcase.mp4
@cbscribe & @aaronfranke you could be interested as you worked on the previous one.
Bugsquad edit:
move_toward
incorrectly #74216