Fix player's vertical velocity taking too long to update when landing on ground

[JimmyThrush]

This PR seeks to fix #143, a problem that causes strange behavior when the player lands on the very edge of a corner while holding down to roll. The bug is caused by the line "parent.movement.y = min(parent.movement.y,0)" taking too long to trigger because it's in the physics_process function, which isn't typically an issue but causes problems in this one case. This PR makes this code execute in the state_activated function of the normal and rolling player states, setting the player's Y movement to 0 on the same frame as the landing and fixing this problem.

[Renhoex]

Just want to say we'll test this as soon as one of us have time. this has been sitting here a few days and I just want to make it clear this isn't being ignored, life just gets in the way

[DimensionWarped]

My immediate inclination is that this is the wrong approach to fixing #143 - There is something that has to kick off the transition to state normal / state rolling before the logic of activating the state can take effect, so whatever that is should be responsible for setting y velocity to 0 ahead of changing the state. The way this is, we would never be able to change to normal state without zeroing y velocity and I can't help but imagine that there are legitimate reasons to do that, particularly in the design of certain gimmicks or enemies.

[JimmyThrush]

The way this is, we would never be able to change to normal state without zeroing y velocity and I can't help but imagine that there are legitimate reasons to do that, particularly in the design of certain gimmicks or enemies.

Y velocity was zeroed when changing to the normal state before, since it's in the physics_process function of the state script. I can't imagine any kind of mechanic that would warrant delaying the change in velocity by a couple of imperceptible frames.

There is something that has to kick off the transition to state normal / state rolling before the logic of activating the state can take effect, so whatever that is should be responsible for setting y velocity to 0 ahead of changing the state.

To my understanding, the normal state is entered from the air state based on the ground bool set by PhysicsObject.gd, which is true if the object's raycasts are colliding or if move_and_slide determines that the object is on the ground. I don't think you'd want that script to determine this sort of thing, since interactions with the terrain should be decided by the object extending from the PhysicsObject class and not the class itself.

Perhaps, rather than having the ground and rolling states make this determination, it would be cleaner to put the logic in the Air state's state_exit function? Something like, "if parent.ground, then parent.movement.y = min(parent.movement.y,0)". I suppose it is quite messy to add activation functions to Normal.gd and Air.gd for this one problem, and I believe this would have the same effect.

[DimensionWarped]

I may have been mistaken about normal.gd being a state that you can be in while airborne. Not 100% positive on that still, it's not documented the best.

As far as I'm aware, PhysicsObjects don't normally use these states, it's only the player objects specifcially that do -- in which case we should be fine using a universal behavior for connecting to ground like this in the state rather than with the specific physic object. Am I mistaken?

[JimmyThrush]

As far as I'm aware, PhysicsObjects don't normally use these states, it's only the player objects specifcially that do -- in which case we should be fine using a universal behavior for connecting to ground like this in the state rather than with the specific physic object. Am I mistaken?

From what I know, you're right, the PhysicsObject class just sets flags that are used by the Player object to decide when to switch to some states; the PhysicsObject class doesn't have a state machine of its own.

I may have been mistaken about normal.gd being a state that you can be in while airborne. Not 100% positive on that still, it's not documented the best.

For this reason I named an equivalent state "Ground" in my own project. I'm almost certain that the Normal state is only for ground movement.

Soon I'll go ahead and make the change I suggested to this PR and then hopefully everything should be good from there.

[JimmyThrush]

Sorry if those commits are sloppily done, I've never edited a PR before. But I removed the changes to Normal.gd and Roll.gd and moved this fix to Air.gd's state_exit function, as I suggested. Everything should be good from here.

[DimensionWarped]

Looks minimally invasive and like it targets the problem directly. Approved.