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 usage of p_cancel_sliding and snapping refractoring. #100360

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

Conversation

yosoyfreeman
Copy link
Contributor

This addresses an issue with the usage of p_cancel_sliding inside of characterBody3D.

Explanation of the issue:

move_and_collide can place the body outside of the motion vector on the recovery phase. For this, the equivalent internal method has a parameter called p_cancel_sliding. This will try and place the body in a valid place as long as the penetration isn't too deep to be safely done. This is meant to alleviate the side effects of move_and_collide not being a true "sweep" when used for this purposes (Which is effectively always, the exposed GDScript method even has this parameter not exposed and set to true by default to match the expected behavior).

When p_cancel_sliding is not used, the body will "slide" out of it's intended path as a result of the recovery phase. Note that this "sliding" is not accurate to any real behavior. While it does kinematic resolution, it does it after we have tried to approximate as much as we could the exact position of the collision. This limits the penetration of the moving body into the another and therefore the kinematic resolution is not comparable to that of a complete solver. That's is the reason for the always slow but constant sliding it causes when not used.

How it affects move_and_slide:

There are multiple instances inside move_and_slide in which p_cancel_slide was set to false, straying the body from the intended motion path. Some instances of this are quite visible as it was the case with #99397 in which the body could stray various cm from it's intended position. Others may be less noticeable, but the same principle applies. There was an extra "Ghost" movement always presents which contributed to the body being stuck for example as a result of being resolved against an already collided planed, decreasing the stability of the algorithm. As @rburing pointed out the original intention was probably to use this kinematic resolution as part of the sliding movement, but as i already said this resolution is not physically accurate and even if it was, we would be adding unwanted motion to an algorithm who's only purpose is to manually simulate the movement and resolution of the body against the collided planes.

What this PR does:

It fixes all of the instances in which p_cancel_sliding was being set to false on calls to move_and_collide that have an impact on the motion of the body (All of them with the exception of on_floor_if_snapped, which never actually moves the body and therefore we can disable it to save a bit of computing).

This also allowed me to refactor apply_floor_snap to be simpler and easier to read. Note that this change is fully equivalent to the one of #99397 for the final user, which you can verify by using this pr and the mpr of the the other pr. The only difference is that p_cancel_sliding will make sure it's safe to perform this correction so the body doesn't get stuck.

This PR was tested by me with the 3º person demo and tested by @tracefree too. No regressions were observed and the body is slightly more precise while colliding against angled walls, for example. While this last part is hardly noticeable for itself it should contribute to less "random" and hard to identify situations as a result of a kinematic resolution we don't want and can't control.

…g why we don't need it in a particular case

- Refractor apply_floor_snap to use p_cancel_sliding and simplify the code
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.

2 participants