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

Add properties to CharacterBody for more move_and_slide options #51027

Merged
merged 1 commit into from
Aug 10, 2021

Conversation

fabriceci
Copy link
Contributor

@fabriceci fabriceci commented Jul 29, 2021

Implementation of the proposal 2982 that aim to fix and add more options to move and slide in 2D, see the proposal for details.

I made the following project https://github.com/fabriceci/move_and_slide_4.0 to test this PR easily.

Screenshot 2021-07-31 at 18 54 34

@fabriceci fabriceci requested review from a team as code owners July 29, 2021 17:11
@fabriceci fabriceci force-pushed the improve-move-and-slide branch from de2d8f1 to 222ef13 Compare July 29, 2021 17:25
@pouleyKetchoupp pouleyKetchoupp requested a review from a team July 29, 2021 17:27
@pouleyKetchoupp pouleyKetchoupp added this to the 4.0 milestone Jul 29, 2021
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

Great work!

I haven't had time to do extensive testing yet, but I've left some comments on the code and documentation. There might be a bit more later when I get deeper into the math :)

doc/classes/CharacterBody2D.xml Outdated Show resolved Hide resolved
doc/classes/CharacterBody2D.xml Outdated Show resolved Hide resolved
doc/classes/CharacterBody2D.xml Outdated Show resolved Hide resolved
doc/classes/CharacterBody2D.xml Outdated Show resolved Hide resolved
doc/classes/CharacterBody2D.xml Outdated Show resolved Hide resolved
scene/2d/physics_body_2d.cpp Outdated Show resolved Hide resolved
scene/2d/physics_body_2d.cpp Outdated Show resolved Hide resolved
scene/2d/physics_body_2d.cpp Outdated Show resolved Hide resolved
scene/2d/physics_body_2d.cpp Outdated Show resolved Hide resolved
scene/2d/physics_body_2d.cpp Outdated Show resolved Hide resolved
@fabriceci fabriceci force-pushed the improve-move-and-slide branch 5 times, most recently from e9ea589 to 9d136fc Compare July 30, 2021 18:52
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

I've made another batch of review comments about details of the algorithm. You changes generally look great, my comments are mostly about making things a bit more readable and adding comments to help future contributors.

I still haven't done the regression tests I wanted to, I'll probably do it next week after porting some example projects to 4.0.

scene/2d/physics_body_2d.cpp Outdated Show resolved Hide resolved
scene/2d/physics_body_2d.cpp Outdated Show resolved Hide resolved
scene/2d/physics_body_2d.cpp Show resolved Hide resolved
scene/2d/physics_body_2d.cpp Show resolved Hide resolved
scene/2d/physics_body_2d.cpp Outdated Show resolved Hide resolved
scene/2d/physics_body_2d.cpp Outdated Show resolved Hide resolved
scene/2d/physics_body_2d.cpp Outdated Show resolved Hide resolved
@fabriceci fabriceci force-pushed the improve-move-and-slide branch 3 times, most recently from aeb71e4 to e7d408a Compare August 1, 2021 10:45
@pouleyKetchoupp
Copy link
Contributor

pouleyKetchoupp commented Aug 3, 2021

Here are the tests I've made so far:

No regression spotted, everything seems to work well.

No regression.
Constant speed works well.

Possible issue: When setting motion on floor only (default move_max_angle) the character is able to climb a little bit on the curve at the 45 degree limit, and slide back a little bit, instead of just being stopped.
It's not a regression since it only affects the new settings, so not a requirement for this PR, but eventually it would be nice for curves to be handled with no glitch.
slide_slope_curve

Constant speed works well.

Possible regression: When setting move_max_angle to 180 degrees, the character is not able to climb the steeper slope anymore when moving right. It jitters on the edge instead. I would expect that it would work the same as before this PR in this case (the character was able to move on walls when their angle is low enough, and slide back when standing).

Before With this PR
slide_slope_before_PR slide_slope_PR

Edit: After discussion with @fabriceci and more tests, it makes sense that it's a case not supported by default. We can actually get rid of move_max_angle which is a confusing parameter (it allows walking on walls) and instead it's possible to change up_direction by script to achieve similar results. So the general idea can be that whatever is not floor is considered not walkable at all, it's much easier to understand and document properly.

Just for the record, here's a snippet of gdscript that can achieve something similar (before move_and_slide()):

if ((abs(linear_velocity.x) > 0) && is_on_wall() && get_slide_count() > 0):
	var last_collision = get_slide_collision(get_slide_count() - 1)
	var collision_normal = last_collision.get_normal()
	var collision_angle = abs(rad2deg(acos(collision_normal.dot(Vector2.UP))))
	if (collision_angle < 90.0):
		up_direction = collision_normal
		stop_on_slope = false
else:
	up_direction = Vector2.UP
	stop_on_slope = true

No regression.

Next I'm going to convert physics tests from https://github.com/godotengine/godot-demo-projects/tree/master/2d/physics_tests to 4.0 in order to do the last remaining regression tests.

@fabriceci fabriceci force-pushed the improve-move-and-slide branch 2 times, most recently from b7d823e to 1bdef5c Compare August 6, 2021 21:26
@fabriceci
Copy link
Contributor Author

fabriceci commented Aug 6, 2021

Possible issue: When setting motion on floor only (default move_max_angle) the character is able to climb a little bit on the curve at the 45 degree limit, and slide back a little bit, instead of just being stopped.
It's not a regression since it only affects the new settings, so not a requirement for this PR, but eventually it would be nice for curves to be handled with no glitch.

I struggled a lot on this. Note: there was a small problem in the test project you used, the gravity was not applied on the ground.

@fabriceci fabriceci force-pushed the improve-move-and-slide branch from 1bdef5c to 73dd986 Compare August 6, 2021 22:18
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

Just a few last things to adjust, otherwise it all works perfectly well!

Edit: Also tested with physics tests converted to 4.0, and no regression spotted on character controller tests.
(godotengine/godot-demo-projects#638)

scene/2d/physics_body_2d.cpp Show resolved Hide resolved
scene/2d/physics_body_2d.h Outdated Show resolved Hide resolved
scene/2d/physics_body_2d.cpp Outdated Show resolved Hide resolved
doc/classes/CharacterBody2D.xml Outdated Show resolved Hide resolved
@fabriceci fabriceci force-pushed the improve-move-and-slide branch from 73dd986 to 7a37ccd Compare August 10, 2021 13:31
@fabriceci fabriceci force-pushed the improve-move-and-slide branch from 7a37ccd to c6666cc Compare August 10, 2021 13:57
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

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

Amazing work, thanks!

@akien-mga akien-mga merged commit ac1dab5 into godotengine:master Aug 10, 2021
@akien-mga
Copy link
Member

Thanks!

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.

3 participants