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

Migrate from LegacyColor to bevy_color::Color #12163

Merged
merged 58 commits into from
Feb 29, 2024
Merged
Show file tree
Hide file tree
Changes from 29 commits
Commits
Show all changes
58 commits
Select commit Hold shift + click to select a range
b684a2a
Initial port of bevy_gizmos
Feb 27, 2024
138e26c
Use impl Into<Color> args in gizmo builder methods
Feb 27, 2024
fa8ad85
Merge remote-tracking branch 'origin/main' into color-externals
Feb 28, 2024
21de265
Use new convenience method in aabb.rs
Feb 28, 2024
e00b6bd
Fix color constants in gizmo docs tests
Feb 28, 2024
c07ec86
Remove unused import
Feb 28, 2024
d0dedbd
Add Color::srgba_u8
Feb 28, 2024
9d1f0d5
Add `Alpha::set_alpha` method
Feb 28, 2024
3829efd
Port over fog
Feb 28, 2024
ba58dc9
Port over ClearColor
Feb 28, 2024
8bc6c4f
Add new deprecated Color convenience methods
Feb 28, 2024
8e8d237
Port wireframes
Feb 28, 2024
1df0713
Port sprites
Feb 28, 2024
1b46fd1
Port text
Feb 28, 2024
0cfe96f
Add Alpha::is_fully_transparent and is_fully_opaque
Feb 28, 2024
c6806fa
Port bevy_ui
Feb 28, 2024
2ae656c
Add Mul and Div for LinearRgba
Feb 28, 2024
0621f62
Port standard material
Feb 28, 2024
5d8212e
Port lights
Feb 28, 2024
fe75eac
Register color types
Feb 28, 2024
a6b8fb4
Fix some doc tests
Feb 28, 2024
00616e2
Port ColorMaterial and MaterialMesh2d
Feb 28, 2024
80cb407
Port UI outlines
Feb 28, 2024
b7598cd
Add RED/GREEN/BLUE color constants
Feb 28, 2024
edd6988
to_array -> to_f32_array
Feb 28, 2024
d88b2fa
Add LinearRgba::as_u32
Feb 28, 2024
58c2eab
Eliminate remaining uses of `LegacyColor`
Feb 28, 2024
c2f52b0
Remove LegacyColor
Feb 28, 2024
1fb2f55
Merge remote-tracking branch 'origin/main' into color-externals
Feb 28, 2024
a094530
Use to_f32_array for fog code
Feb 28, 2024
0dd2ba4
Use WHITE constant
alice-i-cecile Feb 28, 2024
6b50ac6
set_alpha for Oklcha
Feb 28, 2024
948c1ee
Fix fog typo
Feb 28, 2024
6d2848e
Port over newly added code
Feb 28, 2024
ca5f22f
Merge branch 'color-externals' of https://github.com/alice-i-cecile/b…
Feb 28, 2024
fd6616f
Undo Color::NONE -> Color::TRANSPARENT
Feb 28, 2024
6a03503
Clippy
Feb 29, 2024
66833d4
Merge remote-tracking branch 'origin/main' into color-externals
Feb 29, 2024
07500da
Add deprecated rgb_linear methods to ease migration
Feb 29, 2024
86614a9
Fix doc tests failing in CI
Feb 29, 2024
5e52c49
Clean up redundant import
Feb 29, 2024
3d7f8d2
Tidy up examples
rparrett Feb 29, 2024
76567a6
Merge pull request #164 from rparrett/color-externals-examples
alice-i-cecile Feb 29, 2024
2072eee
More CI failures
Feb 29, 2024
4f3115d
Merge branch 'color-externals' of https://github.com/alice-i-cecile/b…
Feb 29, 2024
d2a8e12
Fixup bad rebase
rparrett Feb 29, 2024
c06d915
Fix clippy
rparrett Feb 29, 2024
8181762
Use .into where possible
rparrett Feb 29, 2024
cf8c4c7
Use color mixing in example
alice-i-cecile Feb 29, 2024
075600b
Improve rgb_from_array
alice-i-cecile Feb 29, 2024
1503719
Merge pull request #165 from rparrett/color-externals-clippy
alice-i-cecile Feb 29, 2024
13584c7
Fix more doc tests
Feb 29, 2024
502f9d6
Merge branch 'main' into color-externals
alice-i-cecile Feb 29, 2024
246e65a
Typo in doc test
Feb 29, 2024
03fdefc
Use `LinearRgba` in more rendering doc tests
Feb 29, 2024
ced8cd4
Fix doc test
Feb 29, 2024
c7fd5e3
Remove Add and Sub from LinearRgba
Feb 29, 2024
d400acb
More doc tests
Feb 29, 2024
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
87 changes: 87 additions & 0 deletions crates/bevy_color/src/color.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ impl Color {
(*self).into()
}

#[deprecated = "Use `Color::srgba` instead"]
/// Creates a new [`Color`] object storing a [`Srgba`] color.
pub const fn rgba(red: f32, green: f32, blue: f32, alpha: f32) -> Self {
Self::srgba(red, green, blue, alpha)
}

Comment on lines +48 to +53
Copy link
Contributor

Choose a reason for hiding this comment

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

Shame to add new deprecated functions but definitely better for this transitional period than the alternative!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I realized that I could do this to ease migration, and promptly started laughing due to how silly it is to add new immediately deprecated functions.

/// Creates a new [`Color`] object storing a [`Srgba`] color.
pub const fn srgba(red: f32, green: f32, blue: f32, alpha: f32) -> Self {
Self::Srgba(Srgba {
Expand All @@ -55,6 +61,12 @@ impl Color {
})
}

#[deprecated = "Use `Color::srgb` instead"]
/// Creates a new [`Color`] object storing a [`Srgba`] color with an alpha of 1.0.
pub const fn rgb(red: f32, green: f32, blue: f32) -> Self {
Self::srgb(red, green, blue)
}

/// Creates a new [`Color`] object storing a [`Srgba`] color with an alpha of 1.0.
pub const fn srgb(red: f32, green: f32, blue: f32) -> Self {
Self::Srgba(Srgba {
Expand All @@ -65,6 +77,67 @@ impl Color {
})
}

#[deprecated = "Use `Color::srgb_from_array` instead"]
/// Reads an array of floats to creates a new [`Color`] object storing a [`Srgba`] color with an alpha of 1.0.
pub fn rgb_from_array(array: [f32; 3]) -> Self {
Self::Srgba(Srgba {
red: array[0],
green: array[1],
blue: array[2],
alpha: 1.0,
})
}
alice-i-cecile marked this conversation as resolved.
Show resolved Hide resolved

/// Reads an array of floats to creates a new [`Color`] object storing a [`Srgba`] color with an alpha of 1.0.
pub fn srgb_from_array(array: [f32; 3]) -> Self {
Self::Srgba(Srgba {
red: array[0],
green: array[1],
blue: array[2],
alpha: 1.0,
})
}

#[deprecated = "Use `Color::srgba_u8` instead"]
/// Creates a new [`Color`] object storing a [`Srgba`] color from [`u8`] values.
///
/// A value of 0 is interpreted as 0.0, and a value of 255 is interpreted as 1.0.
pub fn rgba_u8(red: u8, green: u8, blue: u8, alpha: u8) -> Self {
Self::srgba_u8(red, green, blue, alpha)
}

/// Creates a new [`Color`] object storing a [`Srgba`] color from [`u8`] values.
///
/// A value of 0 is interpreted as 0.0, and a value of 255 is interpreted as 1.0.
pub fn srgba_u8(red: u8, green: u8, blue: u8, alpha: u8) -> Self {
Self::Srgba(Srgba {
red: red as f32 / 255.0,
green: green as f32 / 255.0,
blue: blue as f32 / 255.0,
alpha: alpha as f32 / 255.0,
})
}

#[deprecated = "Use `Color::srgb_u8` instead"]
/// Creates a new [`Color`] object storing a [`Srgba`] color from [`u8`] values with an alpha of 1.0.
///
/// A value of 0 is interpreted as 0.0, and a value of 255 is interpreted as 1.0.
pub fn rgb_u8(red: u8, green: u8, blue: u8) -> Self {
Self::srgb_u8(red, green, blue)
}

/// Creates a new [`Color`] object storing a [`Srgba`] color from [`u8`] values with an alpha of 1.0.
///
/// A value of 0 is interpreted as 0.0, and a value of 255 is interpreted as 1.0.
pub fn srgb_u8(red: u8, green: u8, blue: u8) -> Self {
Self::Srgba(Srgba {
red: red as f32 / 255.0,
green: green as f32 / 255.0,
blue: blue as f32 / 255.0,
alpha: 1.0,
})
}

/// Createsa new [`Color`] object storing a [`LinearRgba`] color.
pub const fn linear_rgba(red: f32, green: f32, blue: f32, alpha: f32) -> Self {
Self::LinearRgba(LinearRgba {
Expand Down Expand Up @@ -286,6 +359,20 @@ impl Alpha for Color {
Color::Xyza(x) => x.alpha(),
}
}

fn set_alpha(&mut self, alpha: f32) {
match self {
Color::Srgba(x) => x.set_alpha(alpha),
Color::LinearRgba(x) => x.set_alpha(alpha),
Color::Hsla(x) => x.set_alpha(alpha),
Color::Hsva(x) => x.set_alpha(alpha),
Color::Hwba(x) => x.set_alpha(alpha),
Color::Laba(x) => x.set_alpha(alpha),
Color::Lcha(x) => x.set_alpha(alpha),
Color::Oklaba(x) => x.set_alpha(alpha),
Color::Xyza(x) => x.set_alpha(alpha),
}
}
}

impl From<Srgba> for Color {
Expand Down
13 changes: 13 additions & 0 deletions crates/bevy_color/src/color_ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,17 @@ pub trait Alpha: Sized {

/// Return a the alpha component of this color.
fn alpha(&self) -> f32;

/// Sets the alpha component of this color.
fn set_alpha(&mut self, alpha: f32);

/// Is the alpha component of this color less than or equal to 0.0?
fn is_fully_transparent(&self) -> bool {
self.alpha() <= 0.0
}

/// Is the alpha component of this color greater than or equal to 1.0?
fn is_fully_opaque(&self) -> bool {
self.alpha() >= 1.0
}
}
5 changes: 5 additions & 0 deletions crates/bevy_color/src/hsla.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,11 @@ impl Alpha for Hsla {
fn alpha(&self) -> f32 {
self.alpha
}

#[inline]
fn set_alpha(&mut self, alpha: f32) {
self.alpha = alpha;
}
}

impl Luminance for Hsla {
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_color/src/hsva.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ impl Alpha for Hsva {
fn alpha(&self) -> f32 {
self.alpha
}

#[inline]
fn set_alpha(&mut self, alpha: f32) {
self.alpha = alpha;
}
}

impl From<Hsva> for Hwba {
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_color/src/hwba.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,11 @@ impl Alpha for Hwba {
fn alpha(&self) -> f32 {
self.alpha
}

#[inline]
fn set_alpha(&mut self, alpha: f32) {
self.alpha = alpha;
}
}

impl From<Srgba> for Hwba {
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_color/src/laba.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ impl Alpha for Laba {
fn alpha(&self) -> f32 {
self.alpha
}

#[inline]
fn set_alpha(&mut self, alpha: f32) {
self.alpha = alpha;
}
}

impl Luminance for Laba {
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_color/src/lcha.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,11 @@ impl Alpha for Lcha {
fn alpha(&self) -> f32 {
self.alpha
}

#[inline]
fn set_alpha(&mut self, alpha: f32) {
self.alpha = alpha;
}
}

impl Luminance for Lcha {
Expand Down
135 changes: 135 additions & 0 deletions crates/bevy_color/src/linear_rgba.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::ops::{Add, AddAssign, Div, Mul, Sub, SubAssign};

use crate::{color_difference::EuclideanDistance, Alpha, Luminance, Mix, StandardColor};
use bevy_math::Vec4;
use bevy_reflect::{Reflect, ReflectDeserialize, ReflectSerialize};
Expand Down Expand Up @@ -50,6 +52,30 @@ impl LinearRgba {
alpha: 0.0,
};

/// A fully red color with full alpha.
pub const RED: Self = Self {
red: 1.0,
green: 0.0,
blue: 0.0,
alpha: 1.0,
};

/// A fully green color with full alpha.
pub const GREEN: Self = Self {
red: 0.0,
green: 1.0,
blue: 0.0,
alpha: 1.0,
};

/// A fully blue color with full alpha.
pub const BLUE: Self = Self {
red: 0.0,
green: 0.0,
blue: 1.0,
alpha: 1.0,
};

/// An invalid color.
///
/// This type can be used to represent an invalid color value;
Expand Down Expand Up @@ -127,6 +153,26 @@ impl LinearRgba {
self.mix_assign(Self::new(1.0, 1.0, 1.0, self.alpha), adjustment);
}
}

/// Converts the color into a [f32; 4] array in RGBA order.
///
/// This is useful for passing the color to a shader.
pub fn to_f32_array(&self) -> [f32; 4] {
[self.red, self.green, self.blue, self.alpha]
}

/// Converts this color to a u32.
///
/// Maps the RGBA channels in RGBA order to a little-endian byte array (GPUs are little-endian).
/// `A` will be the most significant byte and `R` the least significant.
pub fn as_u32(&self) -> u32 {
u32::from_le_bytes([
(self.red * 255.0) as u8,
(self.green * 255.0) as u8,
(self.blue * 255.0) as u8,
(self.alpha * 255.0) as u8,
])
}
}

impl Default for LinearRgba {
Expand Down Expand Up @@ -193,6 +239,11 @@ impl Alpha for LinearRgba {
fn alpha(&self) -> f32 {
self.alpha
}

#[inline]
fn set_alpha(&mut self, alpha: f32) {
self.alpha = alpha;
}
}

impl EuclideanDistance for LinearRgba {
Expand Down Expand Up @@ -228,6 +279,90 @@ impl From<LinearRgba> for wgpu::Color {
}
}

/// All color channels are scaled directly,
/// but alpha is unchanged.
///
/// Values are not clamped.
impl Mul<f32> for LinearRgba {
type Output = Self;

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

impl Mul<LinearRgba> for f32 {
type Output = LinearRgba;

fn mul(self, rhs: LinearRgba) -> LinearRgba {
rhs * self
}
}

/// All color channels are scaled directly,
/// but alpha is unchanged.
///
/// Values are not clamped.
impl Div<f32> for LinearRgba {
type Output = Self;

fn div(self, rhs: f32) -> Self {
Self {
red: self.red / rhs,
green: self.green / rhs,
blue: self.blue / rhs,
alpha: self.alpha,
}
}
}

/// All color channels are added directly,
/// but alpha is the average of the two alphas.
impl Add<LinearRgba> for LinearRgba {
type Output = LinearRgba;

fn add(self, rhs: LinearRgba) -> LinearRgba {
LinearRgba {
red: self.red + rhs.red,
green: self.green + rhs.green,
blue: self.blue + rhs.blue,
alpha: (self.alpha + rhs.alpha) / 2.0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it okay that this (the alpha averaging) makes addition a non-associative operation?
I think this might lead to surprising behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm really not sure of the best way to handle alpha with color addition and subtraction. Our options:

  1. Average: leads to fairly surprising behavior, non-associative.
  2. Use the left-hand alpha: fairly surprising, non-commutative.
  3. Set the alpha to 1: very surprising, associative and commutative.

Copy link
Contributor

@pablo-lua pablo-lua Feb 29, 2024

Choose a reason for hiding this comment

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

Another possibility is to do a alpha multiplication, as to say "the alpha will blend in each other". If we have ½ + ¼, this will turn in ⅛. But I'm not sure of this behavior in a add function. I think Avarage is better.

Copy link
Contributor

@rparrett rparrett Feb 29, 2024

Choose a reason for hiding this comment

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

In my non-expert opinion, adding colors with alpha doesn't make any sense and maybe we shouldn't provide this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is alpha singled out?
Couldn't we just treat it as we do with the other components (it gets scaled by * and / and gets added/subtracted by + and -).
That would make the most sense when thinking about colours as an affine (or rather convex) space.
0.5 * BLACK is somewhat poorly defined and doesn't represent a colour, but
0.5 * BLACK + 0.5 * WHITE makes perfect sense.
Generally, you can take any set of coefficients a_i >= 0 that sum to 1 and do a convex combination sum_i a_i * color_i. I think that is completely reasonable behaviour. I'm not a color-space expert, but I think that all the colour space gamuts we have are convex sets? (Therefore this kind of operation creates valid colours.)

This is also relevant if we want to implement Point on colours and use color curves. The code for curves generally assumes a space closed under convex combinations, with a commutative and associative addition operation (and a scaling operation that distributes over addition and agrees with scalar multiplication (meaning that (ab) * Point = a * (b * Point)), at least as far as I remember those are the assumptions made.)

Copy link
Member Author

Choose a reason for hiding this comment

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

We definitely don't want to multiply or divide alpha: multiplying and dividing colors by floats is used for things like HDR and emissivity, and suddenly affecting alpha there will lead to subtle bugs.

In terms of addition and subtraction, one of the things I like about the averaging (and all other methods proposed): we stay bounded between 0 and 1 alpha (assuming inputs within that range). What does an alpha of 2 mean? An alpha of -1?

There is a secret 4th options of course: just panic with non 0 or 1 alpha! That seems particularly nasty though, and likely to slow down this operation.

Generally I think we should punt this question to follow-up work, and tackle it once we can see how colors play with both the curves and the Animatable trait.

Copy link
Contributor

@superdump superdump Feb 29, 2024

Choose a reason for hiding this comment

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

In the linear domain using physical units, adding RGB together makes sense as just "more light". What does adding alpha mean? Should it apply 'over' style alpha blending i.e. when adding colours A and B together, it would produce:

red: self.red * self.alpha + other.red * (1.0 - self.alpha),
green: self.green * self.alpha + other.green * (1.0 - self.alpha),
blue: self.blue * self.alpha + other.blue * (1.0 - self.alpha),
alpha: self.alpha + other.alpha * (1 - self.alpha),

?

EDIT: I may have got those the wrong way around. I am referring to wgpu's configuration of alpha blending, which is the standard alpha blending used for transparency:

    /// Blend mode that does standard alpha blending with non-premultiplied alpha.
    pub const ALPHA_BLENDING: Self = Self {
        color: BlendComponent {
            src_factor: BlendFactor::SrcAlpha,
            dst_factor: BlendFactor::OneMinusSrcAlpha,
            operation: BlendOperation::Add,
        },
        alpha: BlendComponent::OVER,
    };
    /// Blend state of (1 * src) + ((1 - src_alpha) * dst)
    pub const OVER: Self = Self {
        src_factor: BlendFactor::One,
        dst_factor: BlendFactor::OneMinusSrcAlpha,
        operation: BlendOperation::Add,
    };

I think I mixed up source and destination. Source is the result of the fragment shader output, destination is what is already in the texture. In my reading-left-to-right biased perspective, I want to consider the first operand as what is in the texture, so the destination, I suppose. And the second operand as the new colour being added to it, so the source. In which case:

red: other.red * other.alpha + self.red * (1.0 - other.alpha),
green: other.green * other.alpha + self.green * (1.0 - other.alpha),
blue: other.blue * other.alpha + self.blue * (1.0 - other.alpha),
alpha: other.alpha + self.alpha * (1 - other.alpha),

Copy link
Contributor

Choose a reason for hiding this comment

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

In the linear domain using physical units, what does alpha mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

We definitely don't want to multiply or divide alpha: multiplying and dividing colors by floats is used for things like HDR and emissivity, and suddenly affecting alpha there will lead to subtle bugs.

Hm, that is a bit of a pickle. I don't know about HDR and rendering stuff like this so I can't comment, though I'll ask if there is a reason that this uses the multiplication rather than a custom method like .lighten() or something appropriate? Is it standard or there's some other reason?

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, I think we should avoid overloading math operators unless they have a clear and unambiguous meaning.

HDR is an important niche, but it's still a niche, and we've already run into the problem that the "meaning" of r, g, and b channels are different in an HDR context than they are in other contexts; common intuitions about color operations, such as clamping, don't necessarily hold. We risk creating confusion if we add a bunch of HDR-specific calculation operations that aren't useful outside of an HDR domain. These would be better done via traits that are clearly marked as being HDR related.

}
}
}

impl AddAssign<LinearRgba> for LinearRgba {
fn add_assign(&mut self, rhs: LinearRgba) {
*self = *self + rhs;
}
}

/// All color channels are subtracted directly,
/// but alpha is the average of the two alphas.
impl Sub<LinearRgba> for LinearRgba {
type Output = LinearRgba;

fn sub(self, rhs: LinearRgba) -> LinearRgba {
LinearRgba {
red: self.red - rhs.red,
green: self.green - rhs.green,
blue: self.blue - rhs.blue,
alpha: (self.alpha + rhs.alpha) / 2.0,
}
}
}

impl SubAssign<LinearRgba> for LinearRgba {
fn sub_assign(&mut self, rhs: LinearRgba) {
*self = *self - rhs;
}
}

// [`LinearRgba`] is intended to be used with shaders
// So it's the only color type that implements [`ShaderType`] to make it easier to use inside shaders
impl encase::ShaderType for LinearRgba {
Expand Down
5 changes: 5 additions & 0 deletions crates/bevy_color/src/oklaba.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,11 @@ impl Alpha for Oklaba {
fn alpha(&self) -> f32 {
self.alpha
}

#[inline]
fn set_alpha(&mut self, alpha: f32) {
self.alpha = alpha;
}
}

impl Luminance for Oklaba {
Expand Down
Loading
Loading