-
-
Notifications
You must be signed in to change notification settings - Fork 377
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
Added %itemtypes% dyed %color%
expression & ExprColorOf Improvement
#7021
base: dev/feature
Are you sure you want to change the base?
Conversation
- Also fixed `color of` not working (getting and setting) with potions, maps and leather armor meta - Improved changers of `color of` expr
Co-authored-by: APickledWalrus <[email protected]>
Co-authored-by: APickledWalrus <[email protected]>
Co-authored-by: LimeGlass <[email protected]>
Co-authored-by: LimeGlass <[email protected]>
- RGB pattern now don't require a space after the commas - renamed couple variables - added a fail shortcut for dying blocks with RGB - Removed color-color comparator as it's not the proper fix, it should be fixed in CondCompare
kept as a band aid fix until we fix it properly due to it breaking this PR
…ript into feature-dyed # Conflicts: # src/main/java/ch/njol/skript/classes/data/DefaultComparators.java
src/main/java/ch/njol/skript/classes/data/DefaultComparators.java
Outdated
Show resolved
Hide resolved
if (mode == ChangeMode.ADD || mode == ChangeMode.REMOVE) | ||
return null; |
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.
this is a bit in the weeds but maybe we could do add blue to {_a leather chestplate}
and it would act how dying normally does in minecraft.
also above please fix the Nullable annotation on acceptChange
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.
I think that if they want to add a color to the chestplate they should just use ExprDyed, adding a color to a variable sounds like it would add it as a separate element
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.
that's fair well we have no way to partially dye items right now, just fully change the color. Which is ok, but not ideal. So even if this isn't the route you think is good I think we should support that behavior in some way.
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.
thought about [:partially] dye %itemtype% %color%
?
@@ -0,0 +1,61 @@ | |||
test "block colors": |
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.
needs to test what happens with beds, signs, banners, non-colourables, etc
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.
needs to test what happens with beds, signs, banners, non-colourables, etc
beds and banners bug out, tried to fix it but I couldn't, make a TODO somewhere about it, do you still want me to add them to the test?
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.
Well you need to handle that by not making them bug out. Either properly implement it or don't do anything if it's a bed/banner. So yes you still need to test for them.
# clean | ||
set block at {_loc} to {_originalBlock} | ||
|
||
test "item colors": |
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.
same as above
this seems super helpful especially for building purposes since you can just change the colour of blocks freely, a handy use for this would be for a tool for a player to like change colours of like glass panes they've placed down or any colourable block so they don't have to go and make separate all the dye colors needed so if this gets added in the next skript release, that would be quite big |
It would definitely be helpful, though proving to be a bit tricky to implement haha |
yeah for sure, I had a code setup to color some blocks with an item in skript and it seems to of recently broken, was sometime after updating from 1.20.4 to 1.21.1 but I only just noticed it and then came across this so this is my hope going forward lol |
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.
Good start.
Description
Ayham has kindly allowed me to take over his PR.
(Let's hope it can get approved)
Original Description:
This PR aims to add a new helpful expression to return itemtypes with colors directly using this pattern
%itemtypes% (dyed|with color [of]) %color%
It also comes with some improvements and fixes to
ExprColorOf
:✔️ Added support to return RGB colors of RGB colored items (see image below)
✔️ Fixed
color of
not working (getting and setting) with potions, maps and leather armor meta✔️ Fixed
color of
not working (getting and setting) with blocks in MC 1.14+I (hope) that I have at least addressed all suggested changes on Ayham's original PR.
I expect that a lot of things may need to be changed due to how old the original PR is, I hope I'm ready haha.
Target Minecraft Versions: any
Requirements: none
Related Issues:
PR #4335