-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Changes from 43 commits
b684a2a
138e26c
fa8ad85
21de265
e00b6bd
c07ec86
d0dedbd
9d1f0d5
3829efd
ba58dc9
8bc6c4f
8e8d237
1df0713
1b46fd1
0cfe96f
c6806fa
2ae656c
0621f62
5d8212e
fe75eac
a6b8fb4
00616e2
80cb407
b7598cd
edd6988
d88b2fa
58c2eab
c2f52b0
1fb2f55
a094530
0dd2ba4
6b50ac6
948c1ee
6d2848e
ca5f22f
fd6616f
6a03503
66833d4
07500da
86614a9
5e52c49
3d7f8d2
76567a6
2072eee
4f3115d
d2a8e12
c06d915
8181762
cf8c4c7
075600b
1503719
13584c7
502f9d6
246e65a
03fdefc
ced8cd4
c7fd5e3
d400acb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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}; | ||
|
@@ -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; | ||
|
@@ -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 { | ||
|
@@ -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 { | ||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is alpha singled out? 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 /// 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), There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the linear domain using physical units, what does alpha mean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.