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

Make Player Sprint + Camel Is Dashing #7365

Open
wants to merge 5 commits into
base: dev/feature
Choose a base branch
from

Conversation

TheAbsolutionism
Copy link
Contributor

@TheAbsolutionism TheAbsolutionism commented Jan 2, 2025

Description

This PR allows users to make players sprint and check if a camel is dashing.


Target Minecraft Versions: any
Requirements: none
Related Issues: none

@sovdeeth
Copy link
Member

sovdeeth commented Jan 2, 2025

I don't think it's appropriate to merge these. Dashing is very different from sprinting, it's more like a jump or a lunge, not a toggled state.

Further conversation revealed this to be a confusion in how the bukkit method is named and how the camel's jump ability is named. The dashing methods do in fact refer to a sprinting state the camel can be in, not its dash ability.

@Efnilite Efnilite added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Jan 3, 2025
Copy link
Member

@Efnilite Efnilite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm after these and @cheeezburga's changes

src/test/skript/tests/syntaxes/effects/EffDashing.sk Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffDashing.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsDashing.java Outdated Show resolved Hide resolved
@sovdeeth
Copy link
Member

sovdeeth commented Jan 3, 2025

re: the existing reviews
isn't this pr intended to be reverted to the initial commit?

@TheAbsolutionism TheAbsolutionism changed the title Camel Sprinting Make Player Sprint + Camel Is Dashing Jan 4, 2025
@Name("Sprinting")
@Description({
"Make a player start or stop sprinting.",
"If the player is not moving when this effect is used, they will be put in sprint mode for a tick and then stops (Causes FOV change). "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"If the player is not moving when this effect is used, they will be put in sprint mode for a tick and then stops (Causes FOV change). "
"If the player is not moving when this effect is used, they will be put in sprint mode for a tick and then stopped (this causes the FOV to change). "

" - The player will not exit the sprinting state if they stop moving.",
" - Restrictions like low hunger will not prevent the player from sprinting",
" - The player pressing shift will stop them sprinting, and pressing sprint will re-assert normal sprinting behavior",
"Using this effect two or more consecutive times on a stationary player produces undefined behavior and should not be relied on."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is so unreliable, should it even be featured in Skript? seems like all this would do is create bug reports for stuff we cant fix

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already got the go ahead from Sovde. He was the one to help note down all the stuff that happens and even rewrote the description.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that was about the difference between dashing and sprinting though, or was this done in dms?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok if we have the quirks documented but it's a fair point, some other opinions would be welcome

import org.bukkit.entity.LivingEntity;

@Name("Camel Is Dashing")
@Description("Checks whether a camel is dashing.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@Description("Checks whether a camel is dashing.")
@Description("Checks whether a camel is currently using its dash ability.")

just want to clarify a bit more

" - The player will not exit the sprinting state if they stop moving.",
" - Restrictions like low hunger will not prevent the player from sprinting",
" - The player pressing shift will stop them sprinting, and pressing sprint will re-assert normal sprinting behavior",
"Using this effect two or more consecutive times on a stationary player produces undefined behavior and should not be relied on."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok if we have the quirks documented but it's a fair point, some other opinions would be welcome

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants