-
-
Notifications
You must be signed in to change notification settings - Fork 362
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
Rework entity visibility #6867
base: dev/feature
Are you sure you want to change the base?
Rework entity visibility #6867
Conversation
Can this just be put into the already existing CanSee condition and PlayerVisibility effect instead of making brand new ones? |
Not sure about that, since the entity one has only been around since Spigot 1.19, but idk |
You should just be able to change which kind of type is accepted in the pattern based on whether or not a certain method exists, if that makes sense. |
You can just change it entirely since 2.10 will be 1.19+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few little things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some small things I saw - I think some of them are just preference so up to you. Stuff like the star imports tho are pretty up there.
src/main/java/ch/njol/skript/expressions/ExprHiddenPlayers.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/ExprHiddenPlayers.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/ExprHiddenPlayers.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/ExprHiddenPlayers.java
Outdated
Show resolved
Hide resolved
src/main/java/ch/njol/skript/expressions/ExprHiddenPlayers.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: mock tests, you would need to provide your own logic implementation for canSee, since mocked objects have no inherent logic. I'm not sure if a junit test here is worth it.
Shouldn't be in the scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the status of the junit tests?
i think the EffEntityVisibility test is fine, but not really sure how to improve the one for CondCanSee. maybe adding an entity to a collection when it's hidden and then canSee checking that list? suggestions welcome :) |
That'd work! As long as there's some basic logic implementation. |
Description
Adds support for the hideEntity, showEntity and canSee(Entity) and removes the player-only equivalents, which are functionally equivalent. Also updates the Hidden Players syntax to match the current style guide and to improve the naming of the variable required for
reveal hidden players of {_p}
.I wasn't sure on how to do the test for the
can see
condition, as this will have to be done with JUnit to mock the player, but I couldn't get it to work properly withcanSee
. pls help :)Target Minecraft Versions: 1.19+ (Skript 2.10)
Requirements: none
Related Issues: #6795