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

Improve CharacterBody script templates. #100662

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yosoyfreeman
Copy link
Contributor

@yosoyfreeman yosoyfreeman commented Dec 20, 2024

This PR improves the default CharacterBody template for both 2D and 3D. It fixes the following issues:

Input distortion:

The line:

var direction := (transform.basis * Vector3(input_dir.x, 0, input_dir.y)).normalized()

Performs the basis multiplication against a potentially non orthonormal basis. This is often the result of an object which doesn't have an scale of 1. However, because the normalization happens after the possibly wrong basis multiplication, the direction of the original input is now lost. This can be observed if you scale one of the axis to be bigger and move diagonally.

This has been fixed by removing the normalization (Which is not necessary because get_vector always reports at max a unit vector) and using an orthonormal basis instead. The scale no longer alters the input direction. This also allowed to use an smaller deadzone for the joystick, allowing the template to work with controllers too and illustrate how is done.

Acceleration had multiple issues:

It was accelerating instantly, but slowing down is done by moving the speed towards zero in an FPS dependent way, as there is no use of delta involved. Also, accelerating per axis is geometrically incorrect (Same issue as with non normalized vectors) and causes a very unnatural feeling. Also, the fps deceleration was the max speed.

if direction:
		velocity.x = direction.x * SPEED
		velocity.z = direction.z * SPEED
	else:
		velocity.x = move_toward(velocity.x, 0, SPEED)
		velocity.z = move_toward(velocity.z, 0, SPEED)

Now we isolate the horizontal component of our velocity and perform correct acceleration/deceleration

# Move the current horizontal velocity towards the target horizontal velocity by the acceleration factor multiplied by delta.
horizontal_velocity = horizontal_velocity.move_toward(target_horizontal_velocity, ACCEL * delta)

I have also taken into account the up vector of the CharacterBody node. We were ignoring it and assuming always an up vector of 0 1 0. Now everything is done having into account that the up vector is arbitrary.

I have tested it and works nicely. The c# versions are my first attempt with MONO, so i would appreciate some help in those to make sure i followed conventions properly.

@AThousandShips
Copy link
Member

@yosoyfreeman yosoyfreeman force-pushed the character_body_templates_fix branch from 9653611 to 29a36d0 Compare December 20, 2024 19:28
@yosoyfreeman
Copy link
Contributor Author

Hi! I seen the proposal, but it contains some gameplay specific choices like walk/run, modifying the velocity when changing direction for game feel purposes etc. As it's clear on the discussion, such things can be really specific and are hard to implement in a generic way. This proposal is meant to keep the same functionality we already have, just solving the input, acceleration and up direction issues. I also tried to make more clear how to properly handle acceleration along the horizontal plane and gravity-jump along the up direction.

So this proposal is more about fixing what we have and polishing it than it is about offering more features.

@AThousandShips
Copy link
Member

That's a PR with code changes to the same templates so good to get the context from those discussions

@yosoyfreeman
Copy link
Contributor Author

Thank you a lot for the review! I'll try to have it fixing as soon as possible!

PS: Yes i think the linked PR is relevant! I was just trying to add context to what this one aims for exactly.

Thank you a lot again!

@yosoyfreeman yosoyfreeman force-pushed the character_body_templates_fix branch from 29a36d0 to 28f8bab Compare December 20, 2024 19:49
@yosoyfreeman
Copy link
Contributor Author

I fixed the issues, i think everything is correct now. If that's not the case, tell me and i'll work on it. Thank you!

@AThousandShips AThousandShips changed the title Improved CharacterBody script templates. Improve CharacterBody script templates. Dec 20, 2024
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

I am not sure if the additional comments help. If anything they make things look more complicated than they are. A lot of this code is good and is readable on its own, it doesn't need the comments repeating themselves.

@yosoyfreeman yosoyfreeman force-pushed the character_body_templates_fix branch from 28f8bab to fd6d416 Compare December 21, 2024 11:42
@Gertkeno
Copy link

Gertkeno commented Dec 21, 2024

Really good fixes to the template. I think it's a great starting point now that it's corrected key issues.

Knockback has been challenging for new developers to make with the preivous templates, I see that as a common forum help post; this template very directly fixes those issues, allowing physics driven forces to affect the character bodies.

A lot of developers would slot animations into the if/else direction: and that remains easy to do, though not as obvious. Luckily any old tutorial using the last template should hint at that implementation.

Many do not even notice the old template does not support controllers despite how close it was, just removing .normalized() helps.

I am glad nothing else is added, this template should focus on legiblity and being extensible! I could see myself explaining this to a 10 year old easily, skipping over orthonormalized as rotates the input haha.

@yosoyfreeman
Copy link
Contributor Author

I just noticed that we are calling jump_velocity to a variable that represents the jump_speed, as speed is just a scalar (what the variable actually is) and velocity implies a vector, which is misleading for the user.

Also, i understand the intention of using constant for the parameters, but things as the speed, the acceleration and the jump speed should be exposed to the editor as i see it. It would also show the user how to export variables.

What do you think? i personally think that exporting them makes more sense, more having into account that constants in gdscript can be changed anyway.

Hearing your thoughts.

Thank you!

@yosoyfreeman yosoyfreeman force-pushed the character_body_templates_fix branch 2 times, most recently from 2e51f71 to 97faae4 Compare December 24, 2024 12:01
@yosoyfreeman
Copy link
Contributor Author

yosoyfreeman commented Dec 24, 2024

Ok, i pushed the latest changes and i think everything should be right!

This is the final work involved in this PR:

  • Fixed the input distortion issue.

  • support for controllers.

  • Implemented a correct way to accelerate without square root distortion due each axis using the max acceleration.

  • Implemented a variable for acceleration instead of using the max speed.

  • Fixed FPS dependency on the acceleration. This is a non trivial issue, but with the current code it's easy to visualize work with. As long as the acceleration is not wrongly manipulated by the user by using some kind of multiplier, we should be OK.

  • General refractor of the code.

  • Fixes some consistency errors with typed vars.

  • Change jump_velocity to jump_speed to reflect that is indeed a scalar, not a vector.

  • Exposes speed, accceleration and jump speed to the editor, showing the user how to do it and the correct annotation for both gdscript and c#

I think that's it. I personally like the result a lot and got good feedback, but of course, feel free to tell me if you think it needs further corrections.

@yosoyfreeman yosoyfreeman requested a review from Mickeon December 24, 2024 13:45
@yosoyfreeman yosoyfreeman force-pushed the character_body_templates_fix branch from 97faae4 to a97dee1 Compare December 24, 2024 13:50
@yosoyfreeman yosoyfreeman force-pushed the character_body_templates_fix branch from a97dee1 to bbf433f Compare January 2, 2025 11:16
@yosoyfreeman yosoyfreeman force-pushed the character_body_templates_fix branch from bbf433f to a851347 Compare January 7, 2025 19:06
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.

5 participants