Repository: godotengine/godot (language: cpp)

Issue:
Relocate Color8 and ColorN methods from @GDScript class to Color class
Should functions of similar functionality not belong to the same class?


Discussion / hints:
They cannot be in the `Color` class while providing `Color8()` and `ColorN()` sugar constructors. I'm not against merging their functionality into `Color`, but the API would have to be different, and I don't think we can have static methods on `Color` either, so it could be awkward:
```gdscript
var color = Color()
color.set_8bit(255, 255, 255, 0)
color.set_named("rubicon")
```

CC @bojidar-bg who implemented those IIRC.
`ColorN(NAME)` already exists as `Color.NAME`, while `Color8` is there since the earliest days of Godot FOSS.
So it's a documentation issue, not a code issue?

On Thu, 11 Jul 2019 at 13:45, Bojidar Marinov <notifications@github.com>
wrote:

> ColorN already exists as Color.NAME, while Color8 is there since the
> earliest days of Godot FOSS.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/godotengine/godot/issues/30499?email_source=notifications&email_token=ABVAJRE5C6WUCGYSOIMRS2LP64FMFA5CNFSM4H7R6ZHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZWJV4I#issuecomment-510434033>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABVAJRGPYSFT5KLZCRNIXQLP64FMFANCNFSM4H7R6ZHA>
> .
>

Now that static methods can be bound to engine types in `master`, should this still be considered for 4.0? cc @vnen
IMHO now in the future `Color` should get `static Color.from_rgba8()` anyway, analogous to `Color.from_hsv()`, `Color.from_rgbe9995()`. Then I see no reason to keep `Color8()`, also I am not a fan of having a function that looks like a constructor, speaking as someone who has gone a long time thinking there is a class `Color8` that derives from `Color`.
The named colors were moved to a static method, but not Color8. Adding a `from_rgba8` could still be done.

Fix patch:
diff --git a/core/math/color.cpp b/core/math/color.cpp
index 1638acd74d8b..4c31d8149d8a 100644
--- a/core/math/color.cpp
+++ b/core/math/color.cpp
@@ -482,6 +482,10 @@ Color Color::from_rgbe9995(uint32_t p_rgbe) {
 	return Color(rd, gd, bd, 1.0f);
 }
 
+Color Color::from_rgba8(int64_t p_r8, int64_t p_g8, int64_t p_b8, int64_t p_a8) {
+	return Color(p_r8 / 255.0f, p_g8 / 255.0f, p_b8 / 255.0f, p_a8 / 255.0f);
+}
+
 Color::operator String() const {
 	return "(" + String::num(r, 4) + ", " + String::num(g, 4) + ", " + String::num(b, 4) + ", " + String::num(a, 4) + ")";
 }
diff --git a/core/math/color.h b/core/math/color.h
index 3943d5ada656..c2ffde1849ca 100644
--- a/core/math/color.h
+++ b/core/math/color.h
@@ -213,6 +213,7 @@ struct [[nodiscard]] Color {
 	static Color from_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);
+	static Color from_rgba8(int64_t p_r8, int64_t p_g8, int64_t p_b8, int64_t p_a8 = 255);
 
 	_FORCE_INLINE_ bool operator<(const Color &p_color) const; // Used in set keys.
 	operator String() const;
diff --git a/core/variant/variant_call.cpp b/core/variant/variant_call.cpp
index d612cb9cead1..5f6eb8399a27 100644
--- a/core/variant/variant_call.cpp
+++ b/core/variant/variant_call.cpp
@@ -2104,11 +2104,12 @@ static void _register_variant_builtin_methods_math() {
 	bind_static_method(Color, hex64, sarray("hex"), varray());
 	bind_static_method(Color, html, sarray("rgba"), varray());
 	bind_static_method(Color, html_is_valid, sarray("color"), varray());
+
 	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_ok_hsl, sarray("h", "s", "l", "alpha"), varray(1.0));
-
 	bind_static_method(Color, from_rgbe9995, sarray("rgbe"), varray());
+	bind_static_method(Color, from_rgba8, sarray("r8", "g8", "b8", "a8"), varray(255));
 }
 
 static void _register_variant_builtin_methods_misc() {
diff --git a/doc/classes/Color.xml b/doc/classes/Color.xml
index 546d90fa3c4f..5434b5ed5915 100644
--- a/doc/classes/Color.xml
+++ b/doc/classes/Color.xml
@@ -177,6 +177,22 @@
 				[/codeblocks]
 			</description>
 		</method>
+		<method name="from_rgba8" qualifiers="static">
+			<return type="Color" />
+			<param index="0" name="r8" type="int" />
+			<param index="1" name="g8" type="int" />
+			<param index="2" name="b8" type="int" />
+			<param index="3" name="a8" type="int" default="255" />
+			<description>
+				Returns a [Color] constructed from red ([param r8]), green ([param g8]), blue ([param b8]), and optionally alpha ([param a8]) integer channels, each divided by [code]255.0[/code] for their final value.
+				[codeblock]
+				var red = Color.from_rgba8(255, 0, 0)             # Same as Color(1, 0, 0).
+				var dark_blue = Color.from_rgba8(0, 0, 51)        # Same as Color(0, 0, 0.2).
+				var my_color = Color.from_rgba8(306, 255, 0, 102) # Same as Color(1.2, 1, 0, 0.4).
+				[/codeblock]
+				[b]Note:[/b] Due to the lower precision of [method from_rgba8] compared to the standard [Color] constructor, a color created with [method from_rgba8] will generally not be equal to the same color created with the standard [Color] constructor. Use [method is_equal_approx] for comparisons to avoid issues with floating-point precision error.
+			</description>
+		</method>
 		<method name="from_rgbe9995" qualifiers="static">
 			<return type="Color" />
 			<param index="0" name="rgbe" type="int" />
diff --git a/modules/gdscript/doc_classes/@GDScript.xml b/modules/gdscript/doc_classes/@GDScript.xml
index e394d194d1a2..1f7bb9d2fb3a 100644
--- a/modules/gdscript/doc_classes/@GDScript.xml
+++ b/modules/gdscript/doc_classes/@GDScript.xml
@@ -11,7 +11,7 @@
 		<link title="GDScript exports">$DOCS_URL/tutorials/scripting/gdscript/gdscript_exports.html</link>
 	</tutorials>
 	<methods>
-		<method name="Color8">
+		<method name="Color8" deprecated="Use [method Color.from_rgba8] instead.">
 			<return type="Color" />
 			<param index="0" name="r8" type="int" />
 			<param index="1" name="g8" type="int" />
diff --git a/modules/gdscript/gdscript_utility_functions.cpp b/modules/gdscript/gdscript_utility_functions.cpp
index 82460696964d..fb93984f61df 100644
--- a/modules/gdscript/gdscript_utility_functions.cpp
+++ b/modules/gdscript/gdscript_utility_functions.cpp
@@ -315,21 +315,22 @@ struct GDScriptUtilityFunctionsDefinitions {
 		}
 	}
 
+#ifndef DISABLE_DEPRECATED
 	static inline void Color8(Variant *r_ret, const Variant **p_args, int p_arg_count, Callable::CallError &r_error) {
 		DEBUG_VALIDATE_ARG_COUNT(3, 4);
 		DEBUG_VALIDATE_ARG_TYPE(0, Variant::INT);
 		DEBUG_VALIDATE_ARG_TYPE(1, Variant::INT);
 		DEBUG_VALIDATE_ARG_TYPE(2, Variant::INT);
 
-		Color color((int64_t)*p_args[0] / 255.0f, (int64_t)*p_args[1] / 255.0f, (int64_t)*p_args[2] / 255.0f);
-
+		int64_t alpha = 255;
 		if (p_arg_count == 4) {
 			DEBUG_VALIDATE_ARG_TYPE(3, Variant::INT);
-			color.a = (int64_t)*p_args[3] / 255.0f;
+			alpha = *p_args[3];
 		}
 
-		*r_ret = color;
+		*r_ret = Color::from_rgba8(*p_args[0], *p_args[1], *p_args[2], alpha);
 	}
+#endif // DISABLE_DEPRECATED
 
 	static inline void print_debug(Variant *r_ret, const Variant **p_args, int p_arg_count, Callable::CallError &r_error) {
 		String s;
@@ -577,8 +578,10 @@ void GDScriptUtilityFunctions::register_functions() {
 	REGISTER_FUNC( load,           false, RETCLS("Resource"), ARGS( ARG("path", STRING)             ), false, varray(     ));
 	REGISTER_FUNC( inst_to_dict,   false, RET(DICTIONARY),    ARGS( ARG("instance", OBJECT)         ), false, varray(     ));
 	REGISTER_FUNC( dict_to_inst,   false, RET(OBJECT),        ARGS( ARG("dictionary", DICTIONARY)   ), false, varray(     ));
+#ifndef DISABLE_DEPRECATED
 	REGISTER_FUNC( Color8,         true,  RET(COLOR),         ARGS( ARG("r8", INT), ARG("g8", INT),
 																	ARG("b8", INT), ARG("a8", INT)  ), false, varray( 255 ));
+#endif // DISABLE_DEPRECATED
 	REGISTER_FUNC( print_debug,    false, RET(NIL),           NOARGS,                                  true,  varray(     ));
 	REGISTER_FUNC( print_stack,    false, RET(NIL),           NOARGS,                                  false, varray(     ));
 	REGISTER_FUNC( get_stack,      false, RET(ARRAY),         NOARGS,                                  false, varray(     ));


Test patch: