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

🚀 Added %itemtypes% dyed %color% expression & ExprColorOf Improvements #4335

Closed

Conversation

AyhamAl-Ali
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali commented Sep 17, 2021

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+

Quick showcase of this PR's results

image
image
in the 2nd image the held item is the 2nd hotbar slot and the new given item is the 4th hotbar slot


Target Minecraft Versions: Any (HEX/RGB 1.16+)
Requirements: None
Related Issues:

- Also fixed `color of` not working (getting and setting) with potions, maps and leather armor meta
- Improved changers of `color of` expr
@TPGamesNL TPGamesNL added enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature Pull request adding a new feature. labels Sep 19, 2021
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Nothing too major. I also wonder if it would be beneficial to have some sort of dye utility methods (since there is a lot of repetition when it comes to the instanceof checks and such)

@AyhamAl-Ali AyhamAl-Ali marked this pull request as draft June 24, 2022 12:16
@AyhamAl-Ali AyhamAl-Ali changed the title 🚀 Added %itemtypes% dyed %color% expression 🚀 Added %itemtypes% dyed %color% expression & ExprColorOf Improvements Dec 24, 2022
@TheLimeGlass TheLimeGlass mentioned this pull request Jan 6, 2023
1 task
@AyhamAl-Ali
Copy link
Member Author

AyhamAl-Ali commented May 12, 2023

This PR now fixes the issue of grabbing block colors, see #5682
Changers support will be added as well

@AyhamAl-Ali AyhamAl-Ali marked this pull request as ready for review May 13, 2023 22:59
@AyhamAl-Ali AyhamAl-Ali added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label May 14, 2023
@sovdeeth
Copy link
Member

sovdeeth commented Apr 7, 2024

Closing due to 7+ months of inactivity

@sovdeeth sovdeeth closed this Apr 7, 2024
@AyhamAl-Ali
Copy link
Member Author

AyhamAl-Ali commented Apr 8, 2024

I AM BACK!

@AyhamAl-Ali AyhamAl-Ali reopened this Apr 8, 2024
- 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
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'm not the best at reviewing so I'll leave the more important reviews to the team, this was just something I noticed right away

kept as a band aid fix until we fix it properly due to it breaking this PR
@AyhamAl-Ali AyhamAl-Ali requested a review from sovdeeth April 9, 2024 01:52
@AyhamAl-Ali AyhamAl-Ali added the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Apr 23, 2024
String hand = offHand ? "off hand" : "";
return String.format("%s tool of %s", hand, getExpr().toString(e, debug));
public String toString(final @Nullable Event event, final boolean debug) {
return offHand ? "off hand " : "" + "tool of " + getExpr().toString(event, debug);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return offHand ? "off hand " : "" + "tool of " + getExpr().toString(event, debug);
return (offHand ? "off hand " : "") + "tool of " + getExpr().toString(event, debug);

// TODO FIX
// this approach has couple issues, users can't use multiple types in source like
// 'broadcast colors of ((trailing burst firework colored blue and red) and targeted block)' and second
// this check doesn't work with variables as it's not converted yet
Copy link
Member

Choose a reason for hiding this comment

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

what do you mean by not converted yet?

protected Color[] get(Event e, Object[] source) {
protected Color[] get(Event event, Object[] source) {
// TODO FIX
// this approach has couple issues, users can't use multiple types in source like
Copy link
Member

Choose a reason for hiding this comment

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

would this be resolved by instead using a SimplePropertyExpression here? is there a reason it couldn't be one?

public class ExprColorOf extends PropertyExpression<Object, Color> {

private static final boolean MAPS_AND_POTIONS_COLORS = Skript.methodExists(PotionMeta.class, "setColor", org.bukkit.Color.class);
Copy link
Member

Choose a reason for hiding this comment

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

return ColorRGB.fromBukkitOrRgbColor(mapMeta.getColor());
} else if (meta instanceof PotionMeta && MAPS_AND_POTIONS_COLORS) {
PotionMeta potionMeta = (PotionMeta) meta;
if (potionMeta.getColor() != null)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (potionMeta.getColor() != null)
if (potionMeta.hasColor())

You need to use this check per the javadoc of getColor

Comment on lines +178 to 185
switch (mode) { // items/blocks/entities only accept these change modes
case RESET:
case DELETE:
case SET:
return CollectionUtils.array(Color.class);
}

return null;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
switch (mode) { // items/blocks/entities only accept these change modes
case RESET:
case DELETE:
case SET:
return CollectionUtils.array(Color.class);
}
return null;
switch (mode) { // items/blocks/entities only accept these change modes
case RESET:
case DELETE:
case SET:
return CollectionUtils.array(Color.class);
default:
return null;
}

I think you can do this (can eventually be converted to a switch expression in the future)

"since different colored beds are different materials. " +
"Instead, set the block to right material, such as a blue bed."); // Let's just assume it's a bed
} catch (Exception ex) {
// TODO remove this as it's too old now
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason not to remove it in this PR?

Material newBlock = setMaterialColor(block.getType(), color);
if (newBlock == null)
continue;
block.setType(newBlock);
Copy link
Member

Choose a reason for hiding this comment

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

how does this affect the BlockData (e.g. bed half)

Comment on lines +48 to +51
"give player leather chestplate dyed red",
"give player potion of invisibility dyed rgb 200, 70, 88",
"give player filled map colored rgb(20, 60, 70)",
"give player wool painted red"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"give player leather chestplate dyed red",
"give player potion of invisibility dyed rgb 200, 70, 88",
"give player filled map colored rgb(20, 60, 70)",
"give player wool painted red"
"give player leather chestplate dyed red",
"give player potion of invisibility dyed rgb 200, 70, 88",
"give player filled map colored rgb(20, 60, 70)",
"give player wool painted red"

I think one line is okay for these annotations

@Since("INSERT VERSION")
public class ExprDyed extends SimpleExpression<ItemType> {

private static final boolean MAPS_AND_POTIONS_COLORS = Skript.methodExists(PotionMeta.class, "setColor", org.bukkit.Color.class);
Copy link
Member

Choose a reason for hiding this comment

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

@sovdeeth sovdeeth added 2.9 Targeting a 2.9.X version release and removed feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. 2.9 Targeting a 2.9.X version release labels Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature Pull request adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants