Add slope support #38
Conversation
The character used to slide down slopes with a low enough incline to be the floor. The character used to run up (or slide up if in the Air state) slopes that are too steep to be the floor. New bad behaviour: - On slopes with an incline equal to max_floor_angle, the character makes a slight hop after running up and looses their grounding when running down them. - When the character runs into a steep slope they perform short hops due to sliding upwards. Ideally the player would stay on the ground (no sliding up the slope). This can be exploited to jump higher. A new ramp obstical (Rampy) was added to the main Game scene to test the slope behaviour. Closes: gdquest-demos#25
NathanLovato
changed the title
There was a problem hiding this comment.
Thanks for your contribution. I wouldn't merge the code with the current bugs that come with the feature. There are also some other changes required, see the review comments.
Note this isn't a fix, but a new feature. The current stable version of Mannequiny not supporting slopes is intended. As you can see, smooth and bug-free slope support is not that easy to add.
| [node name="Playground" parent="." instance=ExtResource( 5 )] | ||
| transform = Transform( -4.37114e-08, 0, -1, 0, 1, 0, 1, 0, -4.37114e-08, 0, 0, -16 ) | ||
|
|
||
| [node name="Rampy" parent="." instance=ExtResource( 9 )] |
There was a problem hiding this comment.
Could you name it something like Ramp or TestRamp ? The names should make sense in the project. Mannequiny is a particular case as it's the name of the character
There was a problem hiding this comment.
I was trying my luck here, will do.
| _parent.enter() | ||
|
|
||
|
|
||
| func exit() -> void: | ||
| _parent.snap = Vector3.DOWN |
There was a problem hiding this comment.
I would rather override the _parent.physics_process(delta) call in physics_process and use move_and_slide in the Air state than mutate that snap vector in different places. At least, instead of this value, we should use a constant in the Move class, so there's only one place to change the default snap vector length. 1m is pretty long and I've had the character go down abruptly on the ground due to the current value.
There was a problem hiding this comment.
The problem is the parent class has more logic in the physics_process related to inputs and rotating the character.
Perhaps if I make a constant as you suggested and make a public method to change the snap direction? There could also be a default snap direction (down) if no argument is given.
There was a problem hiding this comment.
I'm not sure what's best. I was thinking of something like that to start with:
It was in that sense.
# Move.gd
const SNAP_DEFAULT := Vector2.DOWN
...
# Air.gd
func enter(...):
_parent.snap = _parent.SNAP_DEFAULTIt's just a matter of not duplicating the values, as that's a guaranteed source of silent bugs in my experience.
And I'd rather have us stick to properties, as it's more in line with the rest of Godot 3.0+
| @@ -8,7 +8,7 @@ func unhandled_input(event: InputEvent) -> void: | |||
|
|
|||
| func physics_process(delta: float) -> void: | |||
| _parent.physics_process(delta) | |||
| if player.is_on_floor() or player.is_on_wall(): | |||
| if player.is_on_floor() : | |||
There was a problem hiding this comment.
Space left before the :. You can use a code formatter to address these things for you: https://github.com/Scony/godot-gdscript-toolkit/
There was a problem hiding this comment.
Thanks for the suggestion, I'll check it out.
| @@ -3,21 +3,27 @@ extends PlayerState | |||
|
|
|||
|
|
|||
| func physics_process(delta: float) -> void: | |||
| var velocity_previous_y = _parent.velocity.y | |||
There was a problem hiding this comment.
Please use type hints. Here, you can use type inference var velocity_previous_y := _parent.velocity.y
If I get it correctly, the changes here prevent the player from sliding up steep slopes in the Air state? If so, this is not the way to handle this. When walking against a slope that's too steep, the player should not enter the Air state in the first place.
There was a problem hiding this comment.
I was being a bit lazy, inference wasn't working in this case. I'll fix it.
|
@NathanLovato Thank you very much for your feedback on all of this. I have a technical question related to pushing further commits. The guidelines state that the commits should be squashed. Should I push further commits to this branch and squash it later, or should I squash it each time before pushing and force it? |
As you prefer, I can also use the squash and merge feature from GitHub at the end so you don't have to rebase yourself. We have this for the team, as we generate lots of commits. |
The character used to slide down slopes with a low enough incline to be
the floor (this is now fixed).
The character used to run up (or slide up if in the Air state) slopes
that are too steep to be the floor (this is now fixed).
New bad behavior:
makes a slight hop after running up and looses their grounding when
running down them.
to sliding upwards. Ideally the player would stay on the ground (no
sliding up the slope). This can be exploited to jump higher.
A new ramp obstacle (Rampy) was added to the main Game scene to test the
slope behavior.
Closes: #25
Please check if the PR fulfills these requirements: