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

two-way conversions between Color-Vec4 and Color-[f32; 4] #688

Merged
merged 5 commits into from
Oct 19, 2020

Conversation

tigregalis
Copy link
Contributor

Currently you can convert one way from Vec4->Color, and one way from Color->[f32; 4].

Added From trait implementations for Color to allow two-way conversions between Color<->Vec4 and Color<->[f32; 4].

As a side note, replaced impl Into<[f32; 4]> for Color with impl From<Color> for [f32; 4] which is better practice according to Trait std::convert::From.

This is practically my first PR ever - let me know if there's something I've missed / should have done.

@cart
Copy link
Member

cart commented Oct 16, 2020

Congrats on the first-ish pr! This looks good to me and I think its mergeable as-is, but I also think we have an opportunity here to resolve a mismatch in the Color into/from conversions that we introduced in #616. Looks like we forgot to change the Into<Color> impl in that pr.

Now that the "public" interface Color presents is Srgb, i think it makes sense to make the conversions do to/from conversions to Srgb.

For anything converting to Color, can we use the Color::rgba() constructor instead of directly creating the struct like Color { r, g, b, a }. This will ensure that we do srgb conversions.

Likewise for anything extracting values from color (ex: [f32; 4]), can we use color.r(), color.g(), color.b(), and color.a()?

added `impl From<[f32; 4]> for ColorSource` for consistency.
@tigregalis
Copy link
Contributor Author

Thanks. I've learnt something new.

I've made a new commit - is this what you had in mind?

@tigregalis
Copy link
Contributor Author

Perhaps I should make this comment on #616.

impl Mul and impl MulAssign

Should these be changed as well to reflect the public nonlinear API?

Without it:

let starting_vec4 = Vec4::new(0.4, 0.5, 0.6, 1.0);
let starting_color = Color::from(starting_vec4);
dbg!(starting_color);
assert_eq!(
    starting_vec4,
    Vec4::from(starting_color),
); // passes
let transformation = Vec4::new(0.5, 0.5, 0.5, 1.0);
assert_eq!(
    starting_color * transformation,
    Color::from(starting_vec4 * transformation),
); // fails

outputs:

[src\main.rs:45] starting_color = Color {
    red: 0.13286833,
    green: 0.21404114,
    blue: 0.31854683,
    alpha: 1.0,
}
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `Color { red: 0.06643417, green: 0.10702057, blue: 0.15927342, alpha: 1.0 }`,
 right: `Color { red: 0.033104762, green: 0.05087609, blue: 0.07323897, alpha: 1.0 }`', src\main.rs:51:1

What should the behaviour be?

impl Mul<f32> for Color

Is there value in impl Mul<f32> for Color, at least with its current behaviour?

// crates/bevy_render/src/color.rs
impl Mul<f32> for Color {
    type Output = Color;

    fn mul(self, rhs: f32) -> Self::Output {
        Color {
            red: self.red * rhs,
            green: self.green * rhs,
            blue: self.blue * rhs,
            alpha: self.alpha * rhs,
        }
    }
}

I'm not certain of the usefulness of multiplying the alpha.

The same can be achieved with color * Vec3::splat(some_f32) or color * Vec4::splat(some_f32) and it is more explicit.

Separate types

I'm beginning to think these could be separate types that can be converted between each other, or perhaps variants of an enum.

Outline would be something like:

// crates\bevy_render\src\color.rs
pub struct Color;

impl Color {
    pub const fn rgba(r: f32, g: f32, b: f32, a: f32) -> Self { /* ... */ }
    pub fn to_linear(self) -> ColorLinear { /* ... */ }
}

pub struct ColorLinear;

impl ColorLinear {
    pub const fn rgba(r: f32, g: f32, b: f32, a: f32) -> Self { /* ... */ }
    pub fn to_nonlinear(self) -> Color { /* ... */ }
}

impl From<ColorLinear> for Color {
    fn from(color_linear: ColorLinear) -> Self {
        ColorLinear::to_nonlinear(color_linear)
    }
}

impl From<Color> for ColorLinear {
    fn from(color_nonlinear: Color) -> Self {
        Color::to_linear(color_nonlinear)
    }
}

pub enum ColorSource {
    ColorLinear(ColorLinear),
    Color(Color),
    Texture(Handle<Texture>),
}

impl From<Color> for ColorSource { /* ... */ }
impl From<ColorLinear> for ColorSource { /* ... */ }

impl From<Vec4> for Color { /* ... */ }
impl From<Color> for Vec4 { /* ... */ }
impl From<Vec4> for ColorLinear { /* ... */ }
impl From<ColorLinear> for Vec4 { /* ... */ }
// etc.

// crates\bevy_pbr\src\material.rs
impl From<Color> for StandardMaterial { /* ... */ }
impl From<ColorLinear> for StandardMaterial { /* ... */ }

// crates\bevy_sprite\src\color_material.rs
impl From<Color> for ColorMaterial { /* ... */ }
impl From<ColorLinear> for ColorMaterial { /* ... */ }

// everywhere else, Find-Replace /\bColor\b/g for "ColorLinear"

@memoryruins memoryruins added the C-Code-Quality A section of code that is hard to understand or change label Oct 17, 2020
@cart
Copy link
Member

cart commented Oct 18, 2020

Ultimately I think separate types are the right way to go, but that will require slightly smarter gpu data conversions, so I'd rather handle that later.

For now, I do think it makes sense to fix the math ops. I think it makes sense to adjust them to use nonlinear srgb everywhere:

// crates/bevy_render/src/color.rs
impl Mul<f32> for Color {
    type Output = Color;

    fn mul(self, rhs: f32) -> Self::Output {
        Color::rgba(self.r() * rhs.r(), ...);
    }
}

@tigregalis
Copy link
Contributor Author

Ok, I've made those changes.

@cart cart merged commit 03bc5d7 into bevyengine:master Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants