godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.14k stars 21.19k forks source link

Apply_floor_snap() method of CharacterBody3D does not update the velocities after the snapping. #87946

Open yosoyfreeman opened 9 months ago

yosoyfreeman commented 9 months ago

Tested versions

4.2.1 stable. reproducible since the method was introduced.

System information

Fedora workstation

Issue description

When the discussions about the new CharacterBody3D were taking place, the necessity to snap the character regardless of the up velocity resulted in the apply_floor_snap()method by fabriceci (https://github.com/godotengine/godot/pull/73749). Now that we are working on a full refractioning of our game Liblast, i had the time to test it's functionality.

After quite a lot of head scratching, I think I isolated the problem.

The method is meant to force an snapping of the body to the floor even if there is vertical velocity applied. The body snaps correctly, but it's velocity never takes that into account. As a result the body will remain snapped, but it's vertical velocity will continue increasing as if the snapping never happened. This leads to an accumulation of vertical velocity on the body, making it jump when the delta velocity on the vertical axis scape from the snapping vector.

Steps to reproduce

I made my best to reproduce the issue in the following MPR. The character body starts above the floor and falls down due to gravity until it reaches the floor. Once we have reached the floor, an small vertical force is applied. The character remains still until it scape and jump after around 4 seconds.

Minimal reproduction project (MRP)

Apply_floor_snap bug.zip

yosoyfreeman commented 9 months ago

Okay, it is my bad. The documentation reads as follows:

Allows to manually apply a snap to the floor regardless of the body's velocity. This function does nothing when is_on_floor() returns true.

It may be just me, but with this description, and knowing that is on floor is updated once move and slide is performed, i think it gives the idea that you should use it after move and slide. Move the body, and then snap regardless of velocity.

But it works as a kind of parameter for the next move and slide, so in fact you should use apply_floor_snap before calling move and slide and it will be taken into account for the next move and slide.

I think the documentation should clarify that it must be used before move and slide, or otherwise it could lead to unexpected behaviours.

yosoyfreeman commented 9 months ago

I been testing more in deep and i do think there is a problem with this method. It does not make sense to use it before, so i checked what was happening. Is on floor is alternating between true and false. Which means that the problem is still happening, is just that using it before allow it to detach during move and slide. The result is a heavy stutter really noticeable in a first person character.

Using it after messes the velocity, and even when it seems to be attached on the floor while it increases, it still stutters quite a lot with both godot physics and godot jolt.

This method was introduced with the specific user case of using the real velocity of the body to alter is trajectory while keeping it snapped. After various days, i can not achieve is intended use. Is not a documentation issue.

Sorry to bother you @fabriceci but i can not grasp my head around what could be happening. If you could take a peek of it it would be really useful as you were the one who made it.

fabriceci commented 9 months ago

@yosoyfreeman I haven't been too active for a while now, as I'm taking a master's degree in evening classes in addition to my work, which leaves me very little time at the moment.

If I remember correctly, this method was requested because move and slide no longer modifies velocity on slopes as in Godot 3, and only calls snapping if velocity is inverse to up direction. Consequently, if you want to apply the real velocity, you lose the snap, hence this function.

I don't quite understand the problem you're encountering, to move the conversation forward:

yosoyfreeman commented 9 months ago

Thank you for your time, I'll try to make the situation more clear.

As you said, this method was introduced because move and slide no longer causes a change on trajectory on the body by default and you must use get_real_velocity(), losing the snapping functionality if any vertical velocity is produced as a result. This is important because in most games you indeed want to change the trajectory as a result of the sliding, otherwise you will get a simpler movement.

The problem that I'm encountering is that the method do not fulfil is intended usage in the specific scenario is was designed for.

Do you want velocity Y to be removed by snap of floor? If so, why do you want to apply the actual velocity? Because if you delete the Y velocity, it will cancel the real velocity...

Is not about deleting the Y velocity, is about limiting the velocity to the floor plane once the snap happens. If an object is snapped to a floor (Or whatever other plane really) the velocity must be clipped against the plane. Otherwise the body is not being snapped, just moved towards the plane. This will always create a discrepancy between the body actual movement (which remains static due to the "snapping") and the actual velocity, which can keep increasing freely until it is able to scape the snap vector, resulting in big jumps and extremely unstable behaviour. Furthermore, even if the snap is applied a single time, because the velocity is not clipped against the floor, you will have upwards velocity the next frame that will launch the character anyway. This issue can be seen in the MPR, when the apply_floor_snap method is unable to keep the body snapped and causes an enormous discrepancy between the velocity and the actual movement (Again, note that a body snapped to the floor should not present, by definition, any velocity outside the floor plane).

snap_bug

Can you illustrate the problem in a real-life situation to understanding the limitation?

The problem is already being illustrated in a real life scenario. The problem as described above is giving us severe issues to create reliable movement in Liblast. But for more information, let me illustrate another case in which this result is even more unexpected and problematic:

On the original MPR we are applying gravity until we reach the floor, then applying an small acceleration upwards to show that the method is unable to produce the intended results. If we change the code on the CharacterBody3D to NOT contain any acceleration at all, not even gravity, disable stop on slopes, and place the character on the floor since the beginning, just using the real velocity and apply_floor_snap the precision error will accumulate so quickly that it will launch the character to the air in a few frames.

snap bug 2

The only code on the demonstration above:

extends CharacterBody3D

func _physics_process(delta):
    move_and_slide()
    velocity = get_real_velocity()
    apply_floor_snap()
    print(velocity)

MPR: Apply_floor_snap_2.zip

Can you describe the behaviour you think is correct?

When a CharacterBody3D is snapped to the floor, the velocity should be correctly limited (projected ) to the floor plane.

Also, I found that this method is not consistent with the automatic snapping: Snapping is applied only if the character was on the floor the last frame, while apply_floor_snap will do it regardless of it. As a result, the body is Teletransportated to the floor when it gets near to it even if the character was floating. This results in having to re-integrate this part to prevent this.

snap  bug 3 (The body snaps as soon as it reaches 5 meters to the floor)

I hope the picture is more clear now and that i provided enough information to understand the problem and it's implication in real use scenarios such as our FPS game.

fabriceci commented 9 months ago

If we change the code on the CharacterBody3D to NOT contain any acceleration at all, not even gravity, disable stop on slopes, and place the character on the floor since the beginning, just using the real velocity and apply_floor_snap the precision error will accumulate so quickly that it will launch the character to the air in a few frames.

get_real_velocity() returns a difference on frame 2 and frame 3. If you pass these frames, the problem no longer arises. This is due to the safe margin taken into account.

(0, 0, 0) <-- Frame 1
(0, 0.05604, 0) <-- Frame 2
(0, 0.000386, 0) <-- Frame 3
(0, 0, 0) <-- Frame 4 and next

I found that this method is not consistent with the automatic snapping: Snapping is applied only if the character was on the floor the last frame, while apply_floor_snap will do it regardless of it.

It's deliberate, move_and_slide makes "intelligent" use of the snap. This method applies the snap (I was hesitating about the name with force_floor_snap), it's up to you to decide when to apply it. When you call it, it applies a snap.

When a CharacterBody3D is snapped to the floor, the velocity should be correctly limited (projected ) to the floor plane. Is not about deleting the Y velocity, is about limiting the velocity to the floor plane once the snap happens. If an object is snapped to a floor (Or whatever other plane really) the velocity must be clipped against the plane.

So in the case of flat ground, you'd like the Y velocity to be suppressed, I should have said according to the collision normal.

What's stopping you from doing this after calling apply snap?

Can you come over to the chat, it might be easier to try and solve the problem.

yosoyfreeman commented 9 months ago

Hi! Excuse the delay, I'm working right now on a controller system and wanted to take the time to fully experiment with it in a real use case.

I do now understand better the choices you made to this method. In fact, i found it useful for one thing that was not intended for:

Because you made it to not alter the velocity, it is useful even if you don't want to snap the body when there is vertical velocity. Because it will update the is_on_floor() parameter, it can be use to update it after custom modifications to the position are made. For example: I'm currently using it for stair stepping, if the character ends up in a ramp or a wall after move and slide i will place it where it was, call apply_floor_snap() if it was on floor before attempting the stair step and reset it's velocity, so it's like nothing happened. Something it was not possible before and was asked a lot of times, so i think this use case should be explicitly documented.

In resume, I think a better documentation is what is needed here, because the use of this method is not as trivial from the user perspective as it may appear at first glance. This are my two recommendations after working a lot with this method:

As for documentation proposals i must recognize i do not know how they work. If you think i should propose this changes in other place, please tell me and I'll happily do it!

Thanks for your time helping me clear this, i really appreciate it and i think if properly documented this method will be of extreme value for the user.

Hope you have a great day!