Explicit color conversion methods

[st0rmbtw]

Objective

Closes #10319

Changelog

• Added a new Color::rgba_from_array([f32; 4]) -> Color method.
• Added a new Color::rgb_from_array([f32; 3]) -> Color method.
• Added a new Color::rgba_linear_from_array([f32; 4]) -> Color method.
• Added a new Color::rgb_linear_from_array([f32; 3]) -> Color method.
• Added a new Color::hsla_from_array([f32; 4]) -> Color method.
• Added a new Color::hsl_from_array([f32; 3]) -> Color method.
• Added a new Color::lcha_from_array([f32; 4]) -> Color method.
• Added a new Color::lch_from_array([f32; 3]) -> Color method.
• Added a new Color::rgba_to_vec4(&self) -> Vec4 method.
• Added a new Color::rgba_to_array(&self) -> [f32; 4] method.
• Added a new Color::rgb_to_vec3(&self) -> Vec3 method.
• Added a new Color::rgb_to_array(&self) -> [f32; 3] method.
• Added a new Color::rgba_linear_to_vec4(&self) -> Vec4 method.
• Added a new Color::rgba_linear_to_array(&self) -> [f32; 4] method.
• Added a new Color::rgb_linear_to_vec3(&self) -> Vec3 method.
• Added a new Color::rgb_linear_to_array(&self) -> [f32; 3] method.
• Added a new Color::hsla_to_vec4(&self) -> Vec4 method.
• Added a new Color::hsla_to_array(&self) -> [f32; 4] method.
• Added a new Color::hsl_to_vec3(&self) -> Vec3 method.
• Added a new Color::hsl_to_array(&self) -> [f32; 3] method.
• Added a new Color::lcha_to_vec4(&self) -> Vec4 method.
• Added a new Color::lcha_to_array(&self) -> [f32; 4] method.
• Added a new Color::lch_to_vec3(&self) -> Vec3 method.
• Added a new Color::lch_to_array(&self) -> [f32; 3] method.

Migration Guide

Color::from(Vec4) is now Color::rgba_from_array(impl Into<[f32; 4]>)
Vec4::from(Color) is now Color::rgba_to_vec4(&self)

Before:

let color_vec4 = Vec4::new(0.5, 0.5, 0.5);
let color_from_vec4 = Color::from(color_vec4);

let color_array = [0.5, 0.5, 0.5];
let color_from_array = Color::from(color_array);

After:

let color_vec4 = Vec4::new(0.5, 0.5, 0.5);
let color_from_vec4 = Color::rgba_from_array(color_vec4);

let color_array = [0.5, 0.5, 0.5];
let color_from_array = Color::rgba_from_array(color_array);

[alice-i-cecile]

Thanks for tackling this! I really like the diff on the existing code: it's much more explicit about what exactly is going on.

[MrGVSV]

Would it make sense for these methods to take two parameters, with the second being the target color space? Or are we trying to keep the function signature fn(Vec3) -> Color?

[killercup]

I like that the color space is explicit but the input type name could be elided using custom conversion traits, which would look like Color::rgba_from(Vec4), Color::rgba_from(Vec3), and Color::rgba_from([f32; 4]). Is that something we want? Or do we have other APIs that explicitly contain the type name?

[alice-i-cecile]

I like the way that looks, but it will make the docs harder to read and require importing a trait 🤔 Mildly in favor of it still I think?

[st0rmbtw]

So what we are going to do? Conversion traits or leave this as it is?

[alice-i-cecile]

@mockersf you're good at having opinions; want to make a call?

[tim-blackbird]

We could make the functions accept both arrays and glam's vector types like this:

fn rgba_from(values: impl Into<[f32; 4]>)

[killercup]

I'm sorry for starting a bikeshed 😅

We could make the functions accept both arrays and glam's vector types like this:

fn rgba_from(values: impl Into<[f32; 4]>)

This only works for Vec4

[tim-blackbird]

This only works for Vec4

The Vec3 case would be a separate method, from_rgb like the existing constructors.

[alice-i-cecile]

Once the docs and PR description are cleaned up this LGTM :)

[killercup]

I'd still skip the _array suffix and keep the from_ prefix (from_rgba_linear instead rgba_linear_from_array) but maybe I've cause enough bike shedding 😅

[MrGVSV]

I'm still wondering if it would make sense to introduce a ColorSpace enum and just specify that as a second parameter so we don't have to introduce a bunch of new methods for each color space.

I suppose it would then make more sense to call it new or something in that case since from methods tend to only take a single parameter.

So something like:

let color = Color::new(my_vec4, ColorSpace::Rgba);

[st0rmbtw]

@MrGVSV I like the idea with the enum, but we can't have the only new method, because we want to create a Color from different types like: Vec4, [f32; 4], Vec3, [f32; 3] and Rust doesn't allow to overload functions.

[MrGVSV]

@MrGVSV I like the idea with the enum, but we can't have the only new method, because we want to create a Color from different types like: Vec4, [f32; 4], Vec3, [f32; 3] and Rust doesn't allow to overload functions.

Right. In my head I was thinking new_vec4 and such but thinking on this now I feel like keeping the from_vec4 should be okay despite the second argument. It reads clearer at least.

I’m not saying we have to do this, just putting the idea out there in case we want to do something like it 😄

[inodentry]

I highly approve of this PR. Given that Color supports different color spaces, we should have API coverage of each. APIs should be explicit, not have implicit assumptions on RGB. I've seen plenty of bugs in Bevy projects due to various things just assuming RGB. The "Rusty" thing to do is to have explicit APIs so you can always tell what you are working with.

Looked through the code, did not notice anything wrong.

[alice-i-cecile]

@st0rmbtw the CI errors are real: can you fix up the broken doc tests?