-
-
Notifications
You must be signed in to change notification settings - Fork 373
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
Colours Are Cool 🌈 #7246
base: dev/feature
Are you sure you want to change the base?
Colours Are Cool 🌈 #7246
Conversation
- Also fixes blending util method using old, wrong scale
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.
nice work
src/main/java/org/skriptlang/skript/misc/colours/ColourModule.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/misc/colours/ColourModule.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/misc/colours/ColourModule.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/misc/colours/ColourUtils.java
Outdated
Show resolved
Hide resolved
public static float[] rgbToHsl(Color color) { | ||
float r = color.getRed() / 255f; | ||
float g = color.getGreen() / 255f; | ||
float b = color.getBlue() / 255f; | ||
float max = Math.max(r, Math.max(g, b)); | ||
float min = Math.min(r, Math.min(g, b)); | ||
float h, s, l = (max + min) / 2f; | ||
if (max == min) { | ||
h = s = 0f; | ||
} else { | ||
float delta = max - min; | ||
s = l > 0.5f ? delta / (2f - max - min) : delta / (max + min); | ||
if (max == r) { | ||
h = ((g - b) / delta + (g < b ? 6f : 0f)) / 6f; | ||
} else if (max == g) { | ||
h = ((b - r) / delta + 2f) / 6f; | ||
} else { | ||
h = ((r - g) / delta + 4f) / 6f; | ||
} | ||
} | ||
return new float[]{ h, s, l }; | ||
} |
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.
Why not return a new Color?
src/main/java/org/skriptlang/skript/misc/colours/ExprBlend.java
Outdated
Show resolved
Hide resolved
/* return new SyntaxStringBuilder(event, debug) | ||
.append(this.colours) | ||
.append("blended with") | ||
.append(this.blendWith) | ||
.append("by a factor of") | ||
.append(this.amount) | ||
.toString(); */ |
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.
can uncomment now
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 relies on #7244
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.
merged now
src/main/java/org/skriptlang/skript/misc/colours/ExprComplement.java
Outdated
Show resolved
Hide resolved
static { | ||
register(ExprHex.class, String.class, "hex [code]", "colors"); | ||
} | ||
|
||
@Override | ||
public @Nullable String convert(Color from) { | ||
return ColourUtils.toHex(from); | ||
} | ||
|
||
@Override | ||
protected String getPropertyName() { | ||
return "hex code"; | ||
} | ||
|
||
@Override | ||
public Class<? extends String> getReturnType() { | ||
return String.class; | ||
} |
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'm not sure about this
I think i'd rather see it implemented as taking in a number and returning a string, rather than being restricted to 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.
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.
colour
is used everywhere, and i think skript generally uses color
, as do most people. you should probably replace all colour
mentions with color
src/main/java/org/skriptlang/skript/misc/colours/ColourModule.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/misc/colours/ColourModule.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/misc/colours/ColourModule.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/misc/colours/ColourModule.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/misc/colours/ExprComplement.java
Outdated
Show resolved
Hide resolved
src/main/java/org/skriptlang/skript/misc/colours/ExprComplement.java
Outdated
Show resolved
Hide resolved
Yeah already discussed briefly on Discord - I'll change them all over in my next round of commits 👍 |
- Works for ColorRGBs but not SkriptColors
@@ -0,0 +1,144 @@ | |||
package org.skriptlang.skript.misc.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.
package org.skriptlang.skript.misc.colors; | |
package org.skriptlang.skript.common.colors; |
For native/non-Bukkit dependent syntax, we had sort of informally agreed on using common
as the package name for, which I think would work better here.
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 it would be good to move the Color ClassInfo registration to the module
@@ -81,12 +57,17 @@ public enum SkriptColor implements Color { | |||
|
|||
private ChatColor chat; | |||
private DyeColor dye; | |||
@Nullable | |||
private Adjective adjective; | |||
private @Nullable Adjective adjective; |
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.
Is this field still Nullable now that the setter is gone?
import org.jetbrains.annotations.NotNull; | ||
|
||
/** | ||
* Utility class for colour manipulation and conversion. |
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.
* Utility class for colour manipulation and conversion. | |
* Utility class for color manipulation and conversion. |
*/ | ||
public class ColorUtils { | ||
|
||
public static Color fromInt(int asInt) { |
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.
javadocs
public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed, ParseResult parseResult) { | ||
this.colours = (Expression<Color>) exprs[0]; | ||
this.blendWith = (Expression<Color>) exprs[1]; | ||
this.amount = (Expression<Number>) exprs[2]; |
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.amount = (Expression<Number>) exprs[2]; | |
this.amount = (Expression<Number>) exprs[2]; |
"%colors% (blended|mixed) with %colors% [by [a[n] (factor|amount) of] %-number%]", | ||
"blend of %colors% (and|with) %colors% [by [a[n] (factor|amount) of] %-number%]", | ||
"%colors% and %colors% blen(ded|t) together [by [a[n] (factor|amount) of] %-number%]"); |
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.
"%colors% (blended|mixed) with %colors% [by [a[n] (factor|amount) of] %-number%]", | |
"blend of %colors% (and|with) %colors% [by [a[n] (factor|amount) of] %-number%]", | |
"%colors% and %colors% blen(ded|t) together [by [a[n] (factor|amount) of] %-number%]"); | |
"%colors% (blended|mixed) with %colors% [by [(a factor|an amount) of] %-number%]", | |
"blend of %colors% (and|with) %colors% [by [(a factor|an amount) of] %-number%]", | |
"%colors% and %colors% blen(ded|t) together [by [(a factor|an amount) of] %-number%]"); |
I think this is somewhat easier to read (and a bit more correct!)
|
||
@Override | ||
protected String getPropertyName() { | ||
return "complementary color"; |
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.
return "complementary color"; | |
return (hsl ? "hsl " : "") + "complementary color"; |
} | ||
|
||
@Override | ||
protected String getPropertyName() { |
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.
Should be placed after getReturnType
@Override | ||
public Class<? extends String> getReturnType() { | ||
return String.class; | ||
} |
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.
} | |
} | |
Description
This PR adds a few colour manipulation expressions and functions. There might be more to come in another PR, because there's plenty left which can still be included, but these seemed (to me) like the important-ish ones. Find below a list of new syntax:
shade
function: shades a colour by a given amount, optionally using HSL adjustmentstint
function: tints a colour by a given amount, optionally using HSL adjustmentscolourBrightness
function: adjusts the brightness of a colour by a given amount. This is similar to, but not the same as, theshade
andtint
functionsgrayscale
function: converts a colour to its grayscale equivalentsepiatone
function: converts a colour to its sepiatone equivalentThis PR also adds
channel
as an option in ExprARGB.I used this script to do some basic testing in the early stages of the PR, so feel free to try it:
Target Minecraft Versions: any
Requirements: none
Related Issues: none