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

Add OK HSV methods to Color API #76419

Closed
wants to merge 1 commit into from
Closed

Add OK HSV methods to Color API #76419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions core/math/color.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,20 @@ void Color::set_hsl(float p_h, float p_s, float p_l, float p_alpha) {
}
}

void Color::set_ok_hsv(float p_h, float p_s, float p_v, float p_alpha) {
ok_color::HSV hsv;
hsv.h = p_h;
hsv.s = p_s;
hsv.v = p_v;
ok_color new_ok_color;
ok_color::RGB rgb = new_ok_color.okhsv_to_srgb(hsv);
Comment on lines +329 to +330
Copy link
Member

Choose a reason for hiding this comment

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

@fire Why is ok_color a class? 🤔

godot/thirdparty/misc/ok_color.h

Line 25 in 4e1d5be

class ok_color

You've added it like this in #59786 even though in the original source (from this post) it is as namespace.

Suggestion: change ok_color back to be a namespace so the code in here (and similar code in other places) could be changed like:

Suggested change
ok_color new_ok_color;
ok_color::RGB rgb = new_ok_color.okhsv_to_srgb(hsv);
ok_color::RGB rgb = ok_color::okhsv_to_srgb(hsv);

Needing to create an instance like new_ok_color just to be able to call a method not operating on that instance makes no sense to me.
Alternatively (if for some reason class is preferred over namespace) such methods could be changed to be static.

Copy link
Contributor Author

@bonjorno7 bonjorno7 May 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

OK HSL conversion was already part of the engine before I came along.
I don't know why it's a class instead of a namespace, I just followed the conventions in the existing code.
I can change it if you'd like, but I'd have to change it in more places too then.
Edit: Oh you weren't talking to me. Perhaps this should be a separate cleanup then.

Copy link
Member

Choose a reason for hiding this comment

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

@bonjorno7 Indeed might be out of scope of this PR. Just catched my eye, hence the comment. 🙃

Color c = Color(rgb.r, rgb.g, rgb.b, p_alpha).clamp();
r = c.r;
g = c.g;
b = c.b;
a = c.a;
}

void Color::set_ok_hsl(float p_h, float p_s, float p_l, float p_alpha) {
ok_color::HSL hsl;
hsl.h = p_h;
Expand Down Expand Up @@ -669,13 +683,71 @@ Color Color::operator-() const {
1.0f - a);
}

Color Color::from_ok_hsv(float p_h, float p_s, float p_v, float p_alpha) {
Color c;
c.set_ok_hsv(p_h, p_s, p_v, p_alpha);
return c;
}

Color Color::from_ok_hsl(float p_h, float p_s, float p_l, float p_alpha) {
Color c;
c.set_ok_hsl(p_h, p_s, p_l, p_alpha);
return c;
}

float Color::get_ok_hsv_h() const {
if (r == g && g == b) {
return 0.0f;
}
ok_color::RGB rgb;
rgb.r = r;
rgb.g = g;
rgb.b = b;
ok_color new_ok_color;
ok_color::HSV ok_hsv = new_ok_color.srgb_to_okhsv(rgb);
if (Math::is_nan(ok_hsv.h)) {
return 0.0f;
}
return CLAMP(ok_hsv.h, 0.0f, 1.0f);
}

float Color::get_ok_hsv_s() const {
if (r == g && g == b) {
return 0.0f;
}
ok_color::RGB rgb;
rgb.r = r;
rgb.g = g;
rgb.b = b;
ok_color new_ok_color;
ok_color::HSV ok_hsv = new_ok_color.srgb_to_okhsv(rgb);
if (Math::is_nan(ok_hsv.s)) {
return 0.0f;
}
return CLAMP(ok_hsv.s, 0.0f, 1.0f);
}

float Color::get_ok_hsv_v() const {
if (r == g && g == b) {
// Workaround for edge cases causing NaN.
return get_ok_hsl_l();
}
ok_color::RGB rgb;
rgb.r = r;
rgb.g = g;
rgb.b = b;
ok_color new_ok_color;
ok_color::HSV ok_hsv = new_ok_color.srgb_to_okhsv(rgb);
if (Math::is_nan(ok_hsv.v)) {
return 0.0f;
}
return CLAMP(ok_hsv.v, 0.0f, 1.0f);
}

float Color::get_ok_hsl_h() const {
if (r == g && g == b) {
return 0.0f;
}
ok_color::RGB rgb;
rgb.r = r;
rgb.g = g;
Expand All @@ -689,6 +761,9 @@ float Color::get_ok_hsl_h() const {
}

float Color::get_ok_hsl_s() const {
if (r == g && g == b) {
return 0.0f;
}
ok_color::RGB rgb;
rgb.r = r;
rgb.g = g;
Expand Down
8 changes: 8 additions & 0 deletions core/math/color.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@ struct _NO_DISCARD_ Color {
float get_hsl_s() const;
float get_hsl_l() const;
void set_hsl(float p_h, float p_s, float p_l, float p_alpha = 1.0f);
float get_ok_hsv_h() const;
float get_ok_hsv_s() const;
float get_ok_hsv_v() const;
void set_ok_hsv(float p_h, float p_s, float p_v, float p_alpha = 1.0f);
float get_ok_hsl_h() const;
float get_ok_hsl_s() const;
float get_ok_hsl_l() const;
Expand Down Expand Up @@ -203,6 +207,7 @@ struct _NO_DISCARD_ Color {
static Color from_string(const String &p_string, const Color &p_default);
static Color from_hsv(float p_h, float p_s, float p_v, float p_alpha = 1.0f);
static Color from_hsl(float p_h, float p_s, float p_l, float p_alpha = 1.0f);
static Color from_ok_hsv(float p_h, float p_s, float p_v, float p_alpha = 1.0f);
static Color from_ok_hsl(float p_h, float p_s, float p_l, float p_alpha = 1.0f);
static Color from_rgbe9995(uint32_t p_rgbe);

Expand All @@ -225,6 +230,9 @@ struct _NO_DISCARD_ Color {
_FORCE_INLINE_ void set_hsl_h(float p_h) { set_hsl(p_h, get_hsl_s(), get_hsl_l(), a); }
_FORCE_INLINE_ void set_hsl_s(float p_s) { set_hsl(get_hsl_h(), p_s, get_hsl_l(), a); }
_FORCE_INLINE_ void set_hsl_l(float p_l) { set_hsl(get_hsl_h(), get_hsl_s(), p_l, a); }
_FORCE_INLINE_ void set_ok_hsv_h(float p_h) { set_ok_hsv(p_h, get_ok_hsv_s(), get_ok_hsv_v(), a); }
_FORCE_INLINE_ void set_ok_hsv_s(float p_s) { set_ok_hsv(get_ok_hsv_h(), p_s, get_ok_hsv_v(), a); }
_FORCE_INLINE_ void set_ok_hsv_v(float p_v) { set_ok_hsv(get_ok_hsv_h(), get_ok_hsv_s(), p_v, a); }
_FORCE_INLINE_ void set_ok_hsl_h(float p_h) { set_ok_hsl(p_h, get_ok_hsl_s(), get_ok_hsl_l(), a); }
_FORCE_INLINE_ void set_ok_hsl_s(float p_s) { set_ok_hsl(get_ok_hsl_h(), p_s, get_ok_hsl_l(), a); }
_FORCE_INLINE_ void set_ok_hsl_l(float p_l) { set_ok_hsl(get_ok_hsl_h(), get_ok_hsl_s(), p_l, a); }
Expand Down
1 change: 1 addition & 0 deletions core/variant/variant_call.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2003,6 +2003,7 @@ static void _register_variant_builtin_methods() {
bind_static_method(Color, from_string, sarray("str", "default"), varray());
bind_static_method(Color, from_hsv, sarray("h", "s", "v", "alpha"), varray(1.0));
bind_static_method(Color, from_hsl, sarray("h", "s", "l", "alpha"), varray(1.0));
bind_static_method(Color, from_ok_hsv, sarray("h", "s", "v", "alpha"), varray(1.0));
bind_static_method(Color, from_ok_hsl, sarray("h", "s", "l", "alpha"), varray(1.0));

bind_static_method(Color, from_rgbe9995, sarray("rgbe"), varray());
Expand Down
8 changes: 8 additions & 0 deletions core/variant/variant_setget.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,14 @@ void register_named_setters_getters() {
REGISTER_MEMBER(Color, hsl_h);
REGISTER_MEMBER(Color, hsl_s);
REGISTER_MEMBER(Color, hsl_l);

REGISTER_MEMBER(Color, ok_hsv_h);
REGISTER_MEMBER(Color, ok_hsv_s);
REGISTER_MEMBER(Color, ok_hsv_v);

REGISTER_MEMBER(Color, ok_hsl_h);
REGISTER_MEMBER(Color, ok_hsl_s);
REGISTER_MEMBER(Color, ok_hsl_l);
}

void unregister_named_setters_getters() {
Expand Down
4 changes: 4 additions & 0 deletions core/variant/variant_setget.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,10 @@ SETGET_NUMBER_STRUCT_FUNC(Color, double, hsl_h, set_hsl_h, get_hsl_h)
SETGET_NUMBER_STRUCT_FUNC(Color, double, hsl_s, set_hsl_s, get_hsl_s)
SETGET_NUMBER_STRUCT_FUNC(Color, double, hsl_l, set_hsl_l, get_hsl_l)

SETGET_NUMBER_STRUCT_FUNC(Color, double, ok_hsv_h, set_ok_hsv_h, get_ok_hsv_h)
SETGET_NUMBER_STRUCT_FUNC(Color, double, ok_hsv_s, set_ok_hsv_s, get_ok_hsv_s)
SETGET_NUMBER_STRUCT_FUNC(Color, double, ok_hsv_v, set_ok_hsv_v, get_ok_hsv_v)

SETGET_NUMBER_STRUCT_FUNC(Color, double, ok_hsl_h, set_ok_hsl_h, get_ok_hsl_h)
SETGET_NUMBER_STRUCT_FUNC(Color, double, ok_hsl_s, set_ok_hsl_s, get_ok_hsl_s)
SETGET_NUMBER_STRUCT_FUNC(Color, double, ok_hsl_l, set_ok_hsl_l, get_ok_hsl_l)
Expand Down
36 changes: 36 additions & 0 deletions doc/classes/Color.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,24 @@
[/codeblocks]
</description>
</method>
<method name="from_ok_hsv" qualifiers="static">
<return type="Color" />
<param index="0" name="h" type="float" />
<param index="1" name="s" type="float" />
<param index="2" name="v" type="float" />
<param index="3" name="alpha" type="float" default="1.0" />
<description>
Constructs a color from an [url=https://bottosson.github.io/posts/colorpicker/]OK HSV profile[/url]. The hue ([param h]), saturation ([param s]), and value ([param v]) are typically between 0.0 and 1.0.
[codeblocks]
[gdscript]
var color = Color.from_ok_hsv(0.58, 0.5, 0.79, 0.8)
[/gdscript]
[csharp]
var color = Color.FromOkHsv(0.58f, 0.5f, 0.79f, 0.8f);
[/csharp]
[/codeblocks]
</description>
</method>
<method name="from_rgbe9995" qualifiers="static">
<return type="Color" />
<param index="0" name="rgbe" type="int" />
Expand Down Expand Up @@ -520,6 +538,24 @@
<member name="hsl_s" type="float" setter="" getter="" default="0.0">
The HSL saturation of this color, on the range 0 to 1.
</member>
<member name="ok_hsl_h" type="float" setter="" getter="" default="0.0">
The OK HSL hue of this color, on the range 0 to 1.
</member>
<member name="ok_hsl_l" type="float" setter="" getter="" default="0.0">
The OK HSL lightness of this color, on the range 0 to 1.
</member>
<member name="ok_hsl_s" type="float" setter="" getter="" default="0.0">
The OK HSL saturation of this color, on the range 0 to 1.
</member>
<member name="ok_hsv_h" type="float" setter="" getter="" default="0.0">
The OK HSV hue of this color, on the range 0 to 1.
</member>
<member name="ok_hsv_s" type="float" setter="" getter="" default="0.0">
The OK HSV saturation of this color, on the range 0 to 1.
</member>
<member name="ok_hsv_v" type="float" setter="" getter="" default="0.0">
The OK HSV value (brightness) of this color, on the range 0 to 1.
</member>
<member name="r" type="float" setter="" getter="" default="0.0">
The color's red component, typically on the range of 0 to 1.
</member>
Expand Down
37 changes: 37 additions & 0 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/Color.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1062,6 +1062,32 @@ private static int ParseCol8(ReadOnlySpan<char> str, int index)
return ParseCol4(str, index) * 16 + ParseCol4(str, index + 1);
}

/// <summary>
/// Constructs a color from an OK HSV profile. The <paramref name="hue"/>,
/// <paramref name="saturation"/>, and <paramref name="value"/> are typically
/// between 0.0 and 1.0.
/// </summary>
/// <param name="hue">The OK HSV hue, typically on the range of 0 to 1.</param>
/// <param name="saturation">The OK HSV saturation, typically on the range of 0 to 1.</param>
/// <param name="value">The OK HSV value, typically on the range of 0 to 1.</param>
/// <param name="alpha">The alpha (transparency) value, typically on the range of 0 to 1.</param>
/// <returns>The constructed color.</returns>
public static Color FromOkHsv(float hue, float saturation, float value, float alpha = 1.0f)
{
return NativeFuncs.godotsharp_color_from_ok_hsv(hue, saturation, value, alpha);
}

/// <summary>
/// Converts a color to OK HSV values.
/// </summary>
/// <param name="hue">Output parameter for the OK HSV hue.</param>
/// <param name="saturation">Output parameter for the OK HSV saturation.</param>
/// <param name="value">Output parameter for the OK HSV value.</param>
public readonly void ToOkHsv(out float hue, out float saturation, out float value)
{
NativeFuncs.godotsharp_color_to_ok_hsv(this, out hue, out saturation, out value);
}

/// <summary>
/// Constructs a color from an OK HSL profile. The <paramref name="hue"/>,
/// <paramref name="saturation"/>, and <paramref name="lightness"/> are typically
Expand All @@ -1077,6 +1103,17 @@ public static Color FromOkHsl(float hue, float saturation, float lightness, floa
return NativeFuncs.godotsharp_color_from_ok_hsl(hue, saturation, lightness, alpha);
}

/// <summary>
/// Converts a color to OK HSL values.
/// </summary>
/// <param name="hue">Output parameter for the OK HSL hue.</param>
/// <param name="saturation">Output parameter for the OK HSL saturation.</param>
/// <param name="lightness">Output parameter for the OK HSL lightness.</param>
public readonly void ToOkHsl(out float hue, out float saturation, out float lightness)
{
NativeFuncs.godotsharp_color_to_ok_hsl(this, out hue, out saturation, out lightness);
}

/// <summary>
/// Encodes a <see cref="Color"/> from a RGBE9995 format integer.
/// See <see cref="Image.Format.Rgbe9995"/>.
Expand Down
6 changes: 6 additions & 0 deletions modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/NativeFuncs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,14 @@ internal static partial godot_variant godotsharp_callable_call(in godot_callable
internal static partial void godotsharp_callable_call_deferred(in godot_callable p_callable,
godot_variant** p_args, int p_arg_count);

internal static partial Color godotsharp_color_from_ok_hsv(float p_h, float p_s, float p_v, float p_alpha);

internal static partial void godotsharp_color_to_ok_hsv(in Color p_color, out float r_h, out float r_s, out float r_v);

internal static partial Color godotsharp_color_from_ok_hsl(float p_h, float p_s, float p_l, float p_alpha);

internal static partial void godotsharp_color_to_ok_hsl(in Color p_color, out float r_h, out float r_s, out float r_l);

// GDNative functions

// gdnative.h
Expand Down
24 changes: 24 additions & 0 deletions modules/mono/glue/runtime_interop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,13 +517,34 @@ void godotsharp_callable_call_deferred(Callable *p_callable, const Variant **p_a
p_callable->call_deferredp(p_args, p_arg_count);
}

godot_color godotsharp_color_from_ok_hsv(float p_h, float p_s, float p_v, float p_alpha) {
godot_color ret;
Color *dest = (Color *)&ret;
memnew_placement(dest, Color(Color::from_ok_hsv(p_h, p_s, p_v, p_alpha)));
return ret;
}

void godotsharp_color_to_ok_hsv(godot_color *p_color, float *r_h, float *r_s, float *r_v) {
Color *color_val = (Color *)p_color;
*r_h = color_val->get_ok_hsv_h();
*r_s = color_val->get_ok_hsv_s();
*r_v = color_val->get_ok_hsv_v();
}

godot_color godotsharp_color_from_ok_hsl(float p_h, float p_s, float p_l, float p_alpha) {
godot_color ret;
Color *dest = (Color *)&ret;
memnew_placement(dest, Color(Color::from_ok_hsl(p_h, p_s, p_l, p_alpha)));
return ret;
}

void godotsharp_color_to_ok_hsl(godot_color *p_color, float *r_h, float *r_s, float *r_l) {
Color *color_val = (Color *)p_color;
*r_h = color_val->get_ok_hsl_h();
*r_s = color_val->get_ok_hsl_s();
*r_l = color_val->get_ok_hsl_l();
}

// GDNative functions

// gdnative.h
Expand Down Expand Up @@ -1432,7 +1453,10 @@ static const void *unmanaged_callbacks[]{
(void *)godotsharp_callable_get_data_for_marshalling,
(void *)godotsharp_callable_call,
(void *)godotsharp_callable_call_deferred,
(void *)godotsharp_color_from_ok_hsv,
(void *)godotsharp_color_to_ok_hsv,
(void *)godotsharp_color_from_ok_hsl,
(void *)godotsharp_color_to_ok_hsl,
(void *)godotsharp_method_bind_ptrcall,
(void *)godotsharp_method_bind_call,
(void *)godotsharp_variant_new_string_name,
Expand Down
31 changes: 31 additions & 0 deletions tests/core/math/test_color.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,17 @@ TEST_CASE("[Color] Constructor methods") {
CHECK_MESSAGE(
green_rgba.is_equal_approx(green_hsla),
"Creation with HSL notation should result in components approximately equal to the default constructor.");

const Color red_rgba = Color(1, 0, 0, 0.25);
const Color red_ok_hsva = Color(0, 0, 0).from_ok_hsv(0.081205, 1, 1, 0.25);
const Color red_ok_hsla = Color(0, 0, 0).from_ok_hsl(0.081205, 1, 0.568085, 0.25);

CHECK_MESSAGE(
red_rgba.is_equal_approx(red_ok_hsva),
"Creation with OK HSV notation should result in components approximately equal to the default constructor.");
CHECK_MESSAGE(
red_rgba.is_equal_approx(red_ok_hsla),
"Creation with OK HSL notation should result in components approximately equal to the default constructor.");
}

TEST_CASE("[Color] Operators") {
Expand Down Expand Up @@ -123,6 +134,26 @@ TEST_CASE("[Color] Reading methods") {
CHECK_MESSAGE(
dark_blue.get_hsl_l() == doctest::Approx(0.25f),
"The returned HSL lightness should match the expected value.");

CHECK_MESSAGE(
dark_blue.get_ok_hsv_h() == doctest::Approx(0.733478f),
"The returned OK HSV hue should match the expected value.");
CHECK_MESSAGE(
dark_blue.get_ok_hsv_s() == doctest::Approx(0.999991f),
"The returned OK HSV saturation should match the expected value.");
CHECK_MESSAGE(
dark_blue.get_ok_hsv_v() == doctest::Approx(0.474967f),
"The returned OK HSV value should match the expected value.");

CHECK_MESSAGE(
dark_blue.get_ok_hsl_h() == doctest::Approx(0.733478f),
"The returned OK HSL hue should match the expected value.");
CHECK_MESSAGE(
dark_blue.get_ok_hsl_s() == doctest::Approx(0.999998f),
"The returned OK HSL saturation should match the expected value.");
CHECK_MESSAGE(
dark_blue.get_ok_hsl_l() == doctest::Approx(0.167343f),
"The returned OK HSL lightness should match the expected value.");
}

TEST_CASE("[Color] Conversion methods") {
Expand Down