-
-
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
Enhance bevy_render::color
with palette
#9698
Enhance bevy_render::color
with palette
#9698
Conversation
It looks like your PR is a breaking change, but you didn't provide a migration guide. Could you add some context on what users should update when this change get released in a new version of Bevy? |
Note that this was previously discussed in #1146 and #4648, and was quite contentious at the time. I personally think we should probably do this (see the remaining open bugs and usability problems), but it's a call for @cart and the rendering SMEs (@superdump, @robtfm). An RFC might be useful, but your PR description is really solid, and may suffice. |
Your PR increases Bevy Minimum Supported Rust Version. Please update the |
Your PR increases Bevy Minimum Supported Rust Version. Please update the |
I checked the trees generated by `cargo deny check bans`, and I don't see how my inclusion of `palette` caused these extra dependencies to be brought into the graph.
I believe this PR is ready for review. I've addressed the warnings generated by the github-actions bot, updated documentation, and fleshed out the PR description. Feel free to reach out if anyone has any questions! 😊 |
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.
Really appreciate the detail you went into with the PR description. The single representation srgba Color
struct feels like a good change to me, more opinionated, less confusing, and I wasn't able to find any obvious mistakes in the code or comments.
I don't have enough knowledge about colour spaces and rendering to have a useful opinion on approval though unfortunately.
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.
I'm generally in favor of the direction of this PR. Making everything more explicit feels like the right choice as color spaces are pretty complex and losing precision from implicit conversions feels wrong.
It'd be nice to have an example of animating in different color spaces.
alpha: 1.0, | ||
} | ||
#[must_use] | ||
pub fn hsla(hue: f32, saturation: f32, lightness: f32, alpha: f32) -> Self { |
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.
I'm not sure we want all these constructors for Color
from alternative color spaces. I feel like having the user do my_color = Hsla::new(h, s, l, a).into_color()
will make is explict that some conversion (loss of precision) is going on and they'll be more likely to store a Hsla when it makes sense to.
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.
Perfectly reasonable. I kept them to reduce the effort required to migrate user code, but they could be removed (or expanded to other types). Does anyone else have strong feelings either way regarding these space-specific constructors?
Now zeros the alpha channel and ensures its in the most significant byte.
This is a little verbose, but here's an example controlling the window clear color in Oklch with minimal conversions: //! Shows how to dynamically change a color in an arbitrary color space.
//!
//! Use the arrow keys, and left Control and Shift keys to control hue, lightness,
//! and chroma.
//!
//! Conversion into sRGBA only occurs when the color has changed.
use bevy::{prelude::*, render::color::palette};
use palette::{IntoColor, DarkenAssign, LightenAssign, ShiftHueAssign};
#[derive(Resource)]
struct Background {
current: palette::Oklcha,
}
fn main() {
App::new()
.init_resource::<ClearColor>()
.insert_resource(FixedTime::new_from_secs(1. / 30.))
.insert_resource(Background {
current: Color::rgb(0.5, 0.5, 0.9).into_color(),
})
.add_plugins(DefaultPlugins)
.add_systems(Startup, setup)
.add_systems(FixedUpdate, change_background_color)
.add_systems(
Update,
update_clear_color_from_background.after(change_background_color),
)
.run();
}
fn setup(mut commands: Commands) {
commands.spawn(Camera2dBundle::default());
}
fn update_clear_color_from_background(
background: Res<Background>,
mut clear_color: ResMut<ClearColor>,
) {
if background.is_changed() {
clear_color.0 = Color::from_space(background.current);
}
}
fn change_background_color(input: Res<Input<KeyCode>>, mut background: ResMut<Background>) {
if input.pressed(KeyCode::Right) {
background.current.shift_hue_assign(1.);
}
if input.pressed(KeyCode::Left) {
background.current.shift_hue_assign(-1.);
}
if input.pressed(KeyCode::Up) {
background.current.lighten_assign(0.02);
}
if input.pressed(KeyCode::Down) {
background.current.darken_assign(0.02);
}
if input.pressed(KeyCode::ShiftLeft) {
background.current.chroma *= 1.02;
}
if input.pressed(KeyCode::ControlLeft) {
background.current.chroma *= 0.98;
}
} Because these colors could be changing constantly, I chose to store the Oklch representation in a resource. But if the background wasn't changing often, you could instead use the |
I actually meant add a example to the repo. I like the second one since it shows users that interpolating in different color spaces leads to different results. |
Fixed `color_mixing` Formatting
The errors raised during `cargo deny check bans` appear to be unrelated to `palette`
Can do! I've added it to the |
Closing in favour of #12013. |
Objective
const fn Color::hue
,Color::saturation
andColor::lightness
etc #7746Color::lerp()
convenience method for interpolating/mixing colors #10332Solution
This is a major rework of the
bevy_render::color
module. The goal of this PR is to provide more color manipulation options to Bevy and to end users by leveraging the palette crate. There are numerous alternative color libraries for Rust:Out of the ones above, I determined
palette
to be the best suited for Bevy based on the following criteria:palette
supports all colorspaces, conversions, and operations that Bevy currently implements itself.rgb
,palette
is the most popular color management library on crates.io, with well over a million downloads.palette
is available in both MIT and Apache-2.0 licences, matching Bevy's current licensing arrangement.What this PR does is replace the entire
color
module with a single file containing:palette
Color
struct.This new
Color
struct is a custom color type using theFromColorUnclamped
derive macro. Internally,Color
is a simplef32
sRGBA
type equivalent topalette::Srgba
:The reason for creating a custom color type is to allow for all the same custom implementations that Bevy already has for its existing
Color
type. Its compatibility withserde
,wgpu
,encase
, andreflect
is unchanged.The reason I chose a single colorspace for
Color
as a struct rather than an enum of multiple spaces is to avoid the current confusion around space-specific methods and properties. Issues #8838 and #7746 are largely caused by the broad nature of the currentColor
type. By choosing sRGBA as the canonical colorspace forColor
, the potential for confusion should be almost entirely eliminated. As an additional bonus,Color
as a struct now uses 4 fewer bytes for the same information and fidelity. While likely negligible difference in performance, for such a fundamental type, I believe this is a notable benefit.Color Spaces
To make working with this more restrictive
Color
type easier, I've included constructors and named converters for all the colorspaces currently used within the current enum:Using
palette
, the entirecolorspaces.rs
file is redundant and can be replaced. A particular idiosyncrasy I noticed within that file is how the LCH color space is converted into and out of sRGB (LCH -> Lab -> XYZ -> Linear sRGB -> sRGB). This is not a bad algorithm, but it does feel unfortunate that Bevy contained conversion methods for Lab and XYZ, but in a way that nobody could actually take advantage of it. Now, any colorspace supported bypalette
is available to Bevy:mix_in
Mixes two
Color
values at a particular ratio and in any arbitrary space. This is a wrapper for thepalette::Mix
trait:This resolves the long-standing #1402, without enforcing a specific colorspace to
lerp
within. Again, note that the onlyuse
that is required (aside fromColor
obviously) is the space you intend on working in.hue_in
/with_hue_in
/shift_hue_in
These methods allow for access, replacement, and rotation of a
Color
's hue in any compatible space, using thepalette:GetHue
,palette::WithHue
, andpalette::ShiftHue
triats:saturate_in
/desaturate_in
These methods allow relative adjustment to saturation via
palette::Saturate
andpalette::Desaturate
. It's important to note these are relative adjustments to saturation, not absolute:lighten_in
/darken_in
These allow relative adjustment to brightness/lightness/etc. via
palette::Lighten
andpalette::Darken
.in_space
/from_space
These are convenience methods that wrap
palette
'sFromColorUnclamped
andIntoColor
, which themselves are analogous to Rust's nativeFrom
andInto
traits. The reason for these wrapper methods is to allow the use of these traits without importing them explicitly, and to avoid the (fairly clunky in my opinion)<X as Y>::method(...)
syntax.These transforms are the backbone for all the previous
Color
operations. But, I would recommend for complex color operations, an end user should convert into their desired space, operate, and then convert back at the end. The following methods performFrom
andInto
operations on either side, introducing wasted compute for chained operations.The above is a small example, but I think the point is fairly clear.
difference_in
This method allows for CIEDE2000 color difference calculation within a specified colorspace.
The utility of this method is to allow for the approximate comparison of arbitrary colors in a way that should be more robust to floating point errors.
palette
offers other difference metrics which can be cheaper to compute, but are incongruent with typical human perception.perceptually_eq
This method is an opinionated convenience wrapper for
difference_in
which operates inLch
and considers a CIEDE2000 difference of 1.0 to be the threshold for a just noticeable difference.Remarks
This is a major change in how Bevy works with color, but I've preserved enough of the public facing API of
Color
that I believe the pain of transition is well worth the greater flexibility. I'm submitting this initially as a draft PR due to the certainly controversial nature of this PR, and I would greatly appreciate feedback on how best to tackle making a change like this (e.g., possible RFC)Migration Guide
Color
Is Not anenum
match
statements over the color variants no longer work, since there is only a single variant! As such, any code that looks like this:Can be replaced with:
as_
some space No Longer ReturnsColor
Any code which needed to transform a
Color
into a particular space must now work with the concrete type that represents that space. This is a fairly major workflow change, but I think a positive one. I think there are two cases where these methods were being used:Color
. In which case, the concrete type saves on amatch
statement with anunreachable!()
wildcard arm.Color
for work future work. In which case, that future work still needs to match over all possible variants, with reasonable conversion anyway.One downside is that a
Color
as a part of a component may receive repeated conversions into and out of sRBGA if a system is operating on it. However, I believe this wont be a significant issue with the following workarounds:Color
to lazily convert tosRGBA
, storing the provided colorspace representation internally. SinceColor
is no longer a publicenum
, this option is now viable, along with any other metadata the rendering team may want to hide withinColor
for whatever reason.with_
X /set_
X Are RemovedMethods which provided access to non-sRGBA values (lightness, chroma, hue, saturation, etc.) have all been removed. My reasoning for this is
palette
supports so many alternate colorspaces, I fear adding a singleh
method would only add confusion, even if properly documented.This is pretty easy to get around regardless, simply use
color.in_space::<S>()
to get into the space you want to work in, access the color property you need, and then useColor::from_space(color)
to get back.Another benefit to this change is no property accessor performs implicit conversion, avoiding possible wasted compute without the user's intent.
Arithmetic Operations Are Only in sRGB(A)
This might be controversial, and I'm open to being told otherwise, but I believe this is a trap for new users:
The reason is that scalar multiplication of
Color
was dependent on whatever space theenum
was already in. Ifcolor
was sRGBA, it would scale the color channels, which I think is what a naive user would expect. But if they happened to be passed aColor::Hsla
variant instead, now the function does something entirely different. Where I see this being particularly hairy is with the provided constant colors (Color::RED
, etc.), as whatever space they're defined in will decide how the above hypothetical function will behave.Regardless, I've included all the same operator definitions that the original
Color
enum
, just exclusively for the sRGB(A) color space. In my opinion, this still provides all the same critical functionality, without the potential foot-guns.