-
Notifications
You must be signed in to change notification settings - Fork 324
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
Added multi-jumps to jump method/can_jump interval test #1961
Conversation
It's based on ticks
Removed the Optional typing to rely on the default input instead
I believe that there is more work to be done on this. Specifically making it so players can jump off ladders. In the meantime, I would like some critiques on how it is currently coded so that we can ensure that this is inline with the rest of the library. |
This is integrated into can_jump, but also can be used independently of it so devs can do checks that allow them to determine how to jump off ladders.
@FriendlyGecko are you still interested in working on this? |
Yes, I am. However, I am unsure if it is ready/what do we need in order to determine if it is ready. Items I am considering
Is there anything else you can think of that would be necessary? |
I changed the default jump_delay to 0 so that the test will properly see a true value for can_jump with multijump. I argue that 10 ticks is a better number than 0 as a default but that will require updating the test.
This request also adds an is_on_ground test which we can use to differentiate cases where we want altering behavior based on if the player is on a ladder vs the ground. NOTE: In order to test this properly in regards to jumping while on a ladder, it is imperative that you alter the default jump_delay time to a number greater than 0 (10 is a good number). Otherwise player will launch like a rocket up the ladder with infinite jumps. The delay functions to limit the player from instantaneously jumping repeatedly and prevents them from just jumping up the entire ladder unchecked. To further support this, I believe that if we want to merge this, then we need to update the test AND change the default delay_time to 10. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance to add some unit tests for this? Documentation? Examples?
air_jump = air_jump_velocity | ||
else: | ||
air_jump = velocity | ||
if air_jump_style == "additive": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to use a constant/enum for this?>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the current version of the code better fulfill your intent?
Perhaps not in near-near future, but early July my schedule should be a little more open. By documentation do you mean within the code or within the rest of the project's documentation? And by examples do you mean like example code showing/explaining how one could use it? If so, then absolutely. |
After some discussion: This feature can easily be implemented on the user side. I don't think we want more complexity in these simple physicsengines. We will probably brush them up a bit in 3.x. At that point we could look at something like this. |
This is an attempt to start resolving #1848 .
As of current this only has added multi-jump capabilities to the jump method. I added in the ability to set various types of multi-jump: set, additive, and limited (additive to a point).
I also added a jump interval timer delay based on ticks of the physics engine being updated.