Enhance bevy_render::color with palette#9698
Enhance bevy_render::color with palette#9698bushrat011899 wants to merge 27 commits intobevyengine:mainfrom
bevy_render::color with palette#9698Conversation
|
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.
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.
hymm
left a comment
There was a problem hiding this comment.
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.
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.
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::saturationandColor::lightnessetc #7746Color::lerp()convenience method for interpolating/mixing colors #10332Solution
This is a major rework of the
bevy_render::colormodule. 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
paletteto be the best suited for Bevy based on the following criteria:palettesupports all colorspaces, conversions, and operations that Bevy currently implements itself.rgb,paletteis the most popular color management library on crates.io, with well over a million downloads.paletteis available in both MIT and Apache-2.0 licences, matching Bevy's current licensing arrangement.What this PR does is replace the entire
colormodule with a single file containing:paletteColorstruct.This new
Colorstruct is a custom color type using theFromColorUnclampedderive macro. Internally,Coloris a simplef32sRGBAtype 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
Colortype. Its compatibility withserde,wgpu,encase, andreflectis unchanged.The reason I chose a single colorspace for
Coloras 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 currentColortype. By choosing sRGBA as the canonical colorspace forColor, the potential for confusion should be almost entirely eliminated. As an additional bonus,Coloras 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
Colortype easier, I've included constructors and named converters for all the colorspaces currently used within the current enum:Using
palette, the entirecolorspaces.rsfile 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 bypaletteis available to Bevy:mix_inMixes two
Colorvalues at a particular ratio and in any arbitrary space. This is a wrapper for thepalette::Mixtrait:This resolves the long-standing #1402, without enforcing a specific colorspace to
lerpwithin. Again, note that the onlyusethat is required (aside fromColorobviously) is the space you intend on working in.hue_in/with_hue_in/shift_hue_inThese methods allow for access, replacement, and rotation of a
Color's hue in any compatible space, using thepalette:GetHue,palette::WithHue, andpalette::ShiftHuetriats:saturate_in/desaturate_inThese methods allow relative adjustment to saturation via
palette::Saturateandpalette::Desaturate. It's important to note these are relative adjustments to saturation, not absolute:lighten_in/darken_inThese allow relative adjustment to brightness/lightness/etc. via
palette::Lightenandpalette::Darken.in_space/from_spaceThese are convenience methods that wrap
palette'sFromColorUnclampedandIntoColor, which themselves are analogous to Rust's nativeFromandIntotraits. 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
Coloroperations. 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 performFromandIntooperations on either side, introducing wasted compute for chained operations.The above is a small example, but I think the point is fairly clear.
difference_inThis 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.
paletteoffers other difference metrics which can be cheaper to compute, but are incongruent with typical human perception.perceptually_eqThis method is an opinionated convenience wrapper for
difference_inwhich operates inLchand 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
Colorthat 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
ColorIs Not anenummatchstatements 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 ReturnsColorAny code which needed to transform a
Colorinto 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 amatchstatement with anunreachable!()wildcard arm.Colorfor 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
Coloras 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:Colorto lazily convert tosRGBA, storing the provided colorspace representation internally. SinceColoris no longer a publicenum, this option is now viable, along with any other metadata the rendering team may want to hide withinColorfor 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
palettesupports so many alternate colorspaces, I fear adding a singlehmethod 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
Colorwas dependent on whatever space theenumwas already in. Ifcolorwas 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::Hslavariant 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
Colorenum, just exclusively for the sRGB(A) color space. In my opinion, this still provides all the same critical functionality, without the potential foot-guns.