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.

Sign up for GitHub

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

Jump to bottom

Updates EffEquip, CondIsWearing, and adds CondIsSaddled #6849

Merged
merged 24 commits into from
Dec 15, 2024
Merged

Updates EffEquip, CondIsWearing, and adds CondIsSaddled #6849

merged 24 commits into from
Dec 15, 2024

Conversation

cheeezburga
Copy link
Member

@cheeezburga cheeezburga commented Jul 2, 2024 •
edited
Loading

This PR originally was to only update the equip effect, but now does all the following:

  • General updates to the equip effect (horse and steerable stuff)
  • Adds wolf support to the equip effect
  • Updates the equip test (now tests with armour, horses, wolves)
  • Adds saddled condition, to test if a valid entity is saddled
  • Adds saddled test
  • Updates CondIsWearing to check for an item in the BODY slot of an entity as well

This resolves #6838

@cheeezburga cheeezburga changed the base branch from dev/feature to dev/patch July 2, 2024 03:58
@cheeezburga cheeezburga changed the base branch from dev/patch to dev/feature July 2, 2024 03:59
@sovdeeth sovdeeth added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Jul 2, 2024
sovdeeth
sovdeeth requested changes Aug 15, 2024
View reviewed changes
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.

looks good but needs tests

@cheeezburga cheeezburga requested a review from sovdeeth August 30, 2024 17:12
@cheeezburga
Merge branch 'dev/feature' into update-eff-equip
430a0ab
@cheeezburga
Adds saddle condition and changes some small EffEquip stuff
e50ec03
@cheeezburga
Adds saddle test and changes CondIsSaddled to avoid potential conflict
94af02a 
- Renames CondHasSaddle to CondIsSaddled
@cheeezburga
Just some tiny changes to CondIsWearing
9e42efe
@cheeezburga
Updates EffEquip and its test
d59ea69 
- The test now uses CondIsSaddled, and tries to test wolf armor as well
- Removed the Steerable check in EffEquip, as 1.19.4+ (2.10) will always have it
@cheeezburga cheeezburga changed the title Update EffEquip Updates EffEquip and adds CondIsSaddled Sep 3, 2024
@cheeezburga
Copy link
Member Author

cheeezburga commented Sep 3, 2024 •
edited
Loading

The 1.20.5+ tests will always fail due to something happening with the wolf armor section in the EffEquip test. Works fine with effect commands, I'm not sure what the problem is.
Fixed. Thanks @Fusezion

@cheeezburga
Fixes wolf armor test failing
f90975e 
- Due to CondIsWearing not accounting for the wolf armor in the BODY slot, which apparently isn't returned by EntityEquipment#getArmorContents()
- Moves wolf test to separate structure
@cheeezburga
Merge remote-tracking branch 'origin/update-eff-equip' into update-ef…
0ebb2ff 
…f-equip
@cheeezburga cheeezburga changed the title Updates EffEquip and adds CondIsSaddled Updates EffEquip, CondIsWearing, and adds CondIsSaddled Sep 3, 2024
Fusezion
Fusezion suggested changes Sep 3, 2024
View reviewed changes
Copy link
Contributor

@Fusezion Fusezion left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just some teeny tiny requests

src/main/java/ch/njol/skript/conditions/CondIsWearing.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsWearing.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsSaddled.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsSaddled.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsSaddled.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondIsWearing.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffEquip.java Show resolved Hide resolved
src/test/skript/tests/syntaxes/conditions/CondIsSaddled.sk Outdated Show resolved Hide resolved
src/test/skript/tests/syntaxes/effects/EffEquip.sk Outdated Show resolved Hide resolved
src/test/skript/tests/syntaxes/effects/EffEquip.sk Outdated Show resolved Hide resolved
@cheeezburga
Suggestions
90094ef 
- Adds horse armour tests
- Moves annotations in-line
- Only supports 'properly' in CondIsSaddled
Fusezion
Fusezion approved these changes Sep 6, 2024
View reviewed changes
Copy link
Contributor

@Fusezion Fusezion left a comment

Choose a reason for hiding this comment

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

I couldn't spot anything else that should need fixing, we'll see what everyone else thinks

@cheeezburga cheeezburga requested a review from Fusezion September 6, 2024 16:43
@sovdeeth sovdeeth added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Sep 13, 2024
@cheeezburga cheeezburga mentioned this pull request Oct 9, 2024
Merged
@sovdeeth sovdeeth merged commit 342d7e9 into SkriptLang:dev/feature Dec 15, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fusezion Fusezion Fusezion approved these changes

@Moderocky Moderocky Moderocky approved these changes

@sovdeeth sovdeeth sovdeeth approved these changes

@UnderscoreTud UnderscoreTud Awaiting requested review from UnderscoreTud

Assignees
No one assigned
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cheeezburga @sovdeeth @Moderocky @Fusezion @UnderscoreTud