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

Some armour stand syntax #6613

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

Some armour stand syntax #6613

wants to merge 17 commits into from
+558 −0

Conversation

cheeezburga
Copy link

@cheeezburga cheeezburga commented Apr 26, 2024 •
edited

Description

Adds some armour stand syntax. So far, this PR includes a couple of effects and conditions for extremities (arms, base plate), behaviour (canTick, canMove) and properties (size, marker). Would love any feedback.

Probably won't do in this PR:

  • Disabled/locked slots
  • Poses
  • Rotations

Target Minecraft Versions: any (so far)
Requirements: none
Related Issues: #6597

@cheeezburga
Adds armour stand extremities condition
5025686 
- Allows checking of an armour stand's arms or base plate
- Not set on the pattern. Should there be two instead of one with the [:no]?
- I feel like the check method could be more efficient
@cheeezburga
Adds armour stand properties condition
3614fe4 
- Allows checking of an armour stand's properties
- The pattern seems fine, but should it be changed to have a second one which allows for `can` for ticking and moving?
- Again, feel like the check method could be better
@cheeezburga
Adds armour stand extremities effect
530d416
@cheeezburga
Adds armour stand properties effect
554d587
@ShaneBeee
Copy link
Contributor

Just a note, the tick/move methods for ArmorStand are part of PaperAPI not Bukkit API, so you will have to do checks for that to prevent errors on Spigot

@cheeezburga
Copy link
Author

cheeezburga commented Apr 26, 2024 •
edited

Just a note, the tick/move methods for ArmorStand are part of PaperAPI not Bukkit API, so you will have to do checks for that to prevent errors on Spigot

Oh ty, I missed that when I was adding them. Thinking about moving them to their own separate class coz they feel kinda out of place in the Properties class imo (make %armor stands% move really doesnt get across what its doing ya know). Would make that check a bit easier to do as well.

@cheeezburga
Split EffArmorStandProperties and changes
6d24dae 
- Split the properties and behaviour into their own syntax so the patterns weren't weird to accommodate for that
- Changed LivingEntity to Entity to prevent ClassCastException being thrown (see issue SkriptLang#6612 and PR SkriptLang#6614)
@cheeezburga
Created EffArmorStandBehaviour
a11d5ea 
- The result of splitting the properties syntax
@cheeezburga
Split CondArmorStandProperties
77c8f48 
- Split the properties and behaviours into their own syntax
@cheeezburga
Changes to CondArmorStandExtremities
2faf493 
- Just changed the patterns to be better
@cheeezburga
Added methodExists checks for behaviour syntax
0157cc3
cheeezburga and others added 2 commits May 8, 2024 13:34
@cheeezburga @sovdeeth
Apply suggestions from code review
ef4e9a4 
Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>
@cheeezburga
Just small changes from suggestions
cb3a6ec
@cheeezburga cheeezburga marked this pull request as ready for review May 8, 2024 04:16
@cheeezburga cheeezburga requested a review from sovdeeth May 8, 2024 04:24
sovdeeth
sovdeeth requested changes May 8, 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.

All that's left is tests!

src/main/java/ch/njol/skript/conditions/CondArmorStandExtremities.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/conditions/CondArmorStandProperties.java Outdated Show resolved Hide resolved
@sovdeeth sovdeeth added the feature Pull request adding a new feature. label May 9, 2024
@cheeezburga cheeezburga requested a review from sovdeeth May 9, 2024 13:18
sovdeeth
sovdeeth requested changes May 13, 2024
View reviewed changes
Copy link
Member

@sovdeeth sovdeeth left a comment •
edited

Choose a reason for hiding this comment

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

just some formatting
A few edge case tests would also be good, eg put in a null variable, or a non-armor stand entity, etc.

You should also be using delete entity within {_e} tbh

src/test/skript/tests/syntaxes/conditions/CondArmorStandBehaviour.sk Outdated Show resolved Hide resolved
src/test/skript/tests/syntaxes/conditions/CondArmorStandExtremities.sk Outdated Show resolved Hide resolved
src/test/skript/tests/syntaxes/conditions/CondArmorStandProperties.sk Outdated Show resolved Hide resolved
cheeezburga and others added 2 commits May 14, 2024 05:35
@cheeezburga @sovdeeth
Apply suggestions from code review
792e337 
Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>
@cheeezburga
Added edge case tests
26794b1
@cheeezburga cheeezburga requested a review from sovdeeth May 13, 2024 19:51
@cheeezburga
Copy link
Author

just some formatting A few edge case tests would also be good, eg put in a null variable, or a non-armor stand entity, etc.

You should also be using delete entity within {_e} tbh

Added some edge case tests (the two that were mentioned). Let me know if there are any more I should worry about, or if the ones that were added are done wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sovdeeth sovdeeth sovdeeth approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
feature Pull request adding a new feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@cheeezburga @ShaneBeee @sovdeeth