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

Feature/pathfinding to be double #4915

Merged
merged 20 commits into from
Jul 18, 2022
Merged

Feature/pathfinding to be double #4915

merged 20 commits into from
Jul 18, 2022

Conversation

TheLimeGlass
Copy link
Collaborator

Description

Makes the pathfinding a double rather than an integer

@TheLimeGlass
Copy link
Collaborator Author

TheLimeGlass commented Jul 16, 2022

Pikachu is missing in action, so taken it on. Just a simple change of int to double. Already approved and merged. Caught this last minute. Maybe Paper changed at one point from int to double, or it was missed.

Paper handles negative numbers with an absolute.

@TheLimeGlass TheLimeGlass changed the title Feature/pathfinding Feature/pathfinding to be double Jul 16, 2022
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

LGTM. Just one little thing

@Examples({
"make all creepers pathfind towards player",
"make all cows stop pathfinding",
"make event-entity pathfind towards player"
"make event-entity pathfind towards player at speed 1"
})
@Since("INSERT VERSION")
public class EffPathfind extends Effect {
Copy link
Member

Choose a reason for hiding this comment

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

Since you're editing, the check at L49 is no longer needed since it's 1.13+

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a Paper Spigot exclusive effect.

Copy link
Member

Choose a reason for hiding this comment

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

Then that class needs @RequiredPlugins annotation instead of the description note

@AyhamAl-Ali AyhamAl-Ali added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Jul 17, 2022
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

LGTM - make sure to address Ayham's comment

@TheLimeGlass TheLimeGlass requested a review from AyhamAl-Ali July 18, 2022 07:28
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

LGTM, just one last thing, keep the required plugins to just Paper (we don't use Paper Spigot and they stopped using that name)
image

@TheLimeGlass TheLimeGlass requested a review from AyhamAl-Ali July 18, 2022 07:42
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

One last last thing 😄 for Skript 2.7 we no longer specify 1.13+ since Skript will only work on 1.13+ see comment

@TheLimeGlass
Copy link
Collaborator Author

One last last thing 😄 for Skript 2.7 we no longer specify 1.13+ since Skript will only work on 1.13+ see comment

This makes sense to be at 1.13+

@AyhamAl-Ali
Copy link
Member

This makes sense to be at 1.13+

Why?

@TheLimeGlass
Copy link
Collaborator Author

This makes sense to be at 1.13+

Why?

2.7 only supports 1.13+

@TheLimeGlass TheLimeGlass merged commit 7186ed1 into master Jul 18, 2022
@TheLimeGlass TheLimeGlass deleted the feature/pathfinding branch July 18, 2022 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants