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

Allay Support #7358

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

Conversation

TheAbsolutionism
Copy link
Contributor

@TheAbsolutionism TheAbsolutionism commented Jan 2, 2025

Description

This PR aims to add support for Allays. Allowing users to duplicate allays, set if allays can duplicate naturally, make them dance, duplication cooldown, and their target jukebox.


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

@Efnilite Efnilite added the feature Pull request adding a new feature. label Jan 2, 2025
Copy link
Member

@cheeezburga cheeezburga left a comment

Choose a reason for hiding this comment

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

Some things here and there, but looking good. I'm not sure I love the class names being specific to allays, not a big deal though

Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

looking good

src/main/java/ch/njol/skript/effects/EffDancing.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffDancing.java Outdated Show resolved Hide resolved
@@ -0,0 +1,15 @@
test "allay dancing":
Copy link
Member

Choose a reason for hiding this comment

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

piglin tests?

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 left a comment on your initial request for piglins explaining why there were no tests

Copy link
Member

Choose a reason for hiding this comment

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

Well, it failing is a bit concerning. How were you testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exact same test as the allay. Just spawn a piglin instead and change the fail messages. I've tested manually, the exact same way as done through the test and it works.

Copy link
Member

Choose a reason for hiding this comment

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

By manually, do you mean running that exact trigger? or running a similar trigger? or using effect commands?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

effect commands and exact code within a /test command

Copy link
Member

@sovdeeth sovdeeth Feb 10, 2025

Choose a reason for hiding this comment

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

The exact code in a test command didn't fail? If that doesn't fail, the test shouldn't either. Perhaps it's a chunk loading thing like we saw a while back

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants