Upstreaming bevy_color. #12013

viridia · 2024-02-20T21:29:00Z

Objective

This provides a new set of color types and operations for Bevy.

Solution

The new crate provides a set of distinct types for various useful color spaces, along with utilities for manipulating and converting colors.

This is not a breaking change, as no Bevy APIs are modified (yet).

of color types and operations for Bevy.

viridia · 2024-02-20T21:37:09Z

@SME-Rendering This has been discussed with cart and others in discord ui-dev.

of color types and operations for Bevy.

viridia · 2024-02-20T22:01:31Z

One of the test failures seems to be platform-specific. I'm using the existing Bevy functions for converting to and from Lcha, and as you can see it's failing on windows but not linux or mac. And this isn't a small failure either, it's like '0 degrees != 90 degrees'.

The way the conversion tests work is that there's a generated test file that uses the palette crate as the source of truth.

alice-i-cecile · 2024-02-20T22:01:56Z

@bushrat011899 can I get your review / opinions on this, based on your work in #9698 ?

viridia · 2024-02-20T22:15:15Z

@bushrat011899 I recognize that you've done a lot of work on this, more than I have. The main difference here is that my version doesn't have any external dependencies. It does use the palette crate to generate test data, but the actual library does not depend on palette.

This library can and should be extended with more features, many of which are present in your PR. However, I wanted to make this small enough to be digestible to reviewers.

alice-i-cecile

Does the gen_tests crate make more sense in our existing tools directory?

viridia · 2024-02-20T22:22:09Z

Does the gen_tests crate make more sense in our existing tools directory?

Maybe. It's an extremely specialized tool that is only useful in this one context.

crates/bevy_color/src/color_ops.rs

viridia · 2024-02-20T22:26:00Z

There are three test failures. One is not my fault (I think). One is a doc problem which I can fix. The third I'm puzzled by, it's a disparity between what the bevy_render LCH conversion and the palette LCH conversion, the hue component comes out different. It's troubling that the test fails on windows only.

That particular test is kind of funky anyway, I had to give a fairly large tolerance (1.7 degrees) to get it to pass on any platform. I hesitate to point the finger at bevy_render, but I trust palette's answer more. Either that, or my test data generator is calling things incorrectly.

alice-i-cecile · 2024-02-20T22:26:18Z

crates/bevy_color/src/color_ops.rs

+
+    /// Return a lighter version of this color. The `amount` should be between 0.0 and 1.0.
+    /// The amount represents an absolute increase in luminance, and is commutative:
+    /// `color.lighter(a).lighter(b) == color.lighter(a + b)`.


This is a distributive property, not commutative.

I don't think distributivity is fully correct either. Perhaps it's best to just leave out the terminology?

I believe the correct term is "linear", since the property is f(a + b) = f(a) + f(b), which is a specialised case of the definition of a linear operation f(ax + b) = a*f(x) + f(b)

I don't want to leave out this comment because I think it's important that implementers of future color spaces adhere to this rule. Let's go with "distributive" for now.

Basically what this means is that if you darken by 10% and then darken again by 20%, it's the same as if you darkened by 20% and then 10%, or if you just darkened once by 30%. This is trivial to ensure if you are just altering a single scalar luminance channel, but more complex if you are multiplying r, g and b by a factor. Note that you wouldn't get this distributive property if you were simply multiplying by a factor: n * (1 - a) * (1 - b) is not the same as n * (1 - a - b), which is essentially what mixing with black does.

Maybe "affine"? I'm not sure.

At the risk of dangerous levels of bikeshedding... Rewriting lighter to $\ell$ you might phrase it as $\ell(b) \circ \ell(a) = \ell(a + b)$ which could be called something like additivity or additivity under composition.

crates/bevy_color/src/color_ops.rs

crates/bevy_color/src/color_range.rs

crates/bevy_color/src/color_ops.rs

crates/bevy_color/src/color_range.rs

alice-i-cecile · 2024-02-20T22:35:40Z

crates/bevy_color/src/hsla.rs

+use bevy_render::color::{Color, HslRepresentation};
+use serde::{Deserialize, Serialize};
+
+/// Color in Hue-Saturation-Lightness color space with alpha


Wikipedia links for these color space types would be a nice addition.

Sadly, there's no wikipedia page for lch (it's just a single paragraph in the page on color spaces) or linear rgb. Maybe some other online reference?

alice-i-cecile · 2024-02-20T22:39:01Z

crates/bevy_color/src/lib.rs

+//! Each of these color spaces is represented as a distinct Rust type. Colors can be converted
+//! from one color space to another using the [`From`] trait.
+//!
+//! # Color Space Usage


We should add a short note here about how you should use Okalba or at least Hsla for most color blending operations.

To add to this, I'd like the following:

Short comparisons between the color spaces with preferred defaults for different use cases, including when to use ColorRepresentation.

Links to online color pickers for each color space (maybe, up to maintainers)

Explanation of color space, gamut, display ranges, and other terms

Some links to references on how to understand color in more detail

* Links to online color pickers for each color space (maybe, up to maintainers)

Definitely an extension beyond the initial PR, but this sounds like an excellent case for a Bevy tool to be provided, perhaps in the examples folder. I think there's been quite a few examples of colour pickers made using the current Bevy UI system, and a colour picking tool in Bevy is a nice demonstration of how to use bevy_color and an explanation of the broader concept of colours as well.

I've expanded the explanation of color spaces, but not the other suggestions (gamut etc.). Mainly because I'm not confident that my explanation of these would be correct.

Linking to other resources would be fine

crates/bevy_color/src/testing.rs

crates/bevy_color/src/color_difference.rs

crates/bevy_color/src/oklaba.rs

bushrat011899

This looks great! I can appreciate the desire to keep 3rd parties to a minimum, and I think bringing in palette as a comparison for testing/validation is a really good middle ground. I've probably been very nit-picky on some of the suggestions which I'll just apologise for upfront!

I would like to expand this to include some more colour spaces (XYZ, Oklch, etc.) but those are really well suited for follow-up PRs so I'm happy with the current amount of of spaces supported.

Probably the two biggest bits of feedback I have are:

I think the color operations should take self and produce Self to allow the space itself to decide if Copy is appropriate, and default to move semantics.
I think including CIEDE2000 perceptual difference is important enough to consider for this initial PR, but I also understand moving that into a followup.

crates/bevy_color/src/color_difference.rs

crates/bevy_color/crates/gen_tests/Cargo.toml

crates/bevy_color/crates/gen_tests/README.md

crates/bevy_color/src/lcha.rs

crates/bevy_color/src/lib.rs

crates/bevy_color/src/linear_rgba.rs

crates/bevy_color/src/srgba.rs

viridia · 2024-02-22T04:51:16Z

I've added a general link to the Wikipedia article on color spaces in the crate-level doc. I also added a bit more detail on color transformations.

cart

Looks good to me! I'm reasonably confident that this is the right path forward. I'm down to merge this provided we still consider it in the "candidate for upstreaming" phase. I don't think we can fully evaluate its fitness for Bevy until it has been ported.

crates/bevy_color/src/lib.rs

cart · 2024-02-23T00:35:06Z

crates/bevy_color/src/color.rs

+    Oklaba(Oklaba),
+}
+
+impl Color {


I suspect if we swap our current Color type for this, that we will want to add a bunch of convenience constructors here. But that can wait!

crates/bevy_color/Cargo.toml

cart · 2024-02-23T00:47:22Z

Sure. I don't want to go overboard with helper methods, but I'm ok with adding the most commonly used fluent methods. Bear in mind that you can get the same effect, slightly more verbosely, with:

I'm in a similar camp. I like Bevy to be as "field driven" as possible for most types, especially the high-traffic ones. Builder-style should be a last-resort as it increases API surface. I like leaning on Rust as much as possible.

cart · 2024-02-23T00:57:52Z

My current thinking is that rather than changing all of the Bevy APIs for 0.14, we actually refine and improve bevy_color over the next release cycle before doing the integration. This means that people who use bevy_color will need to call .into() when passing the color to ECS components like StandardMaterial or BorderColor. If there are places where we can accept an Into and be backwards-compatible, those can be done right away.

I think it will be hard to refine this further without adopting it internally / we risk refining in the wrong direction. I'd prefer a hard switch PR asap where we can tweak / polish / evaluate the new approach. I think a "middle ground / support both" release would just be confusing and introduce more churn.

Also, we need to decide whether the fundamental color type used in StandardMaterial is eventually going to be Color or LinearRgba. It would be nice to avoid the runtime check of the enum when doing low-level setting of uniforms.

Agreed. I think this is the "biggest" / most important choice to be made. This also applies to other areas, such as UI / sprite color. When a user (or internal Bevy dev) needs a color in their component, what do they reach for? We should be consistent and make doing the "recommended" thing easy by giving "good names" to the recommended types (ex: we should probably rename enum Color to something else if we decide that Srgba should be used in most places, and maybe even consider renaming Srgba to Color).

JMS55 · 2024-02-23T04:44:04Z

For deciding Color vs LinearRgba for StandardMaterial, reflection and editor-driven-development are important considerations.

ameknite · 2024-02-23T21:38:36Z

if this fixes #1402 why is not closed?

# Objective The migration process for `bevy_color` (#12013) will be fairly involved: there will be hundreds of affected files, and a large number of APIs. ## Solution To allow us to proceed granularly, we're going to keep both `bevy_color::Color` (new) and `bevy_render::Color` (old) around until the migration is complete. However, simply doing this directly is confusing! They're both called `Color`, making it very hard to tell when a portion of the code has been ported. As discussed in #12056, by renaming the old `Color` type, we can make it easier to gradually migrate over, one API at a time. ## Migration Guide THIS MIGRATION GUIDE INTENTIONALLY LEFT BLANK. This change should not be shipped to end users: delete this section in the final migration guide! --------- Co-authored-by: Alice Cecile <[email protected]>

# Objective - Simplify `Srgba` hex string parsing using std hex parsing functions and removing loops in favor of bitwise ops. This is a follow-up of the `bevy_color` upstream PR review: #12013 (comment) ## Solution - Reworked `Srgba::hex` to use `from_str_radix` and some bitwise ops; --------- Co-authored-by: Rob Parrett <[email protected]>

# Objective This provides a new set of color types and operations for Bevy. Fixes: bevyengine#10986 bevyengine#1402 ## Solution The new crate provides a set of distinct types for various useful color spaces, along with utilities for manipulating and converting colors. This is not a breaking change, as no Bevy APIs are modified (yet). --------- Co-authored-by: François <[email protected]>

# Objective The migration process for `bevy_color` (bevyengine#12013) will be fairly involved: there will be hundreds of affected files, and a large number of APIs. ## Solution To allow us to proceed granularly, we're going to keep both `bevy_color::Color` (new) and `bevy_render::Color` (old) around until the migration is complete. However, simply doing this directly is confusing! They're both called `Color`, making it very hard to tell when a portion of the code has been ported. As discussed in bevyengine#12056, by renaming the old `Color` type, we can make it easier to gradually migrate over, one API at a time. ## Migration Guide THIS MIGRATION GUIDE INTENTIONALLY LEFT BLANK. This change should not be shipped to end users: delete this section in the final migration guide! --------- Co-authored-by: Alice Cecile <[email protected]>

# Objective - Simplify `Srgba` hex string parsing using std hex parsing functions and removing loops in favor of bitwise ops. This is a follow-up of the `bevy_color` upstream PR review: bevyengine#12013 (comment) ## Solution - Reworked `Srgba::hex` to use `from_str_radix` and some bitwise ops; --------- Co-authored-by: Rob Parrett <[email protected]>

# Objective This provides a new set of color types and operations for Bevy. Fixes: bevyengine#10986 bevyengine#1402 ## Solution The new crate provides a set of distinct types for various useful color spaces, along with utilities for manipulating and converting colors. This is not a breaking change, as no Bevy APIs are modified (yet). --------- Co-authored-by: François <[email protected]>

# Objective The migration process for `bevy_color` (bevyengine#12013) will be fairly involved: there will be hundreds of affected files, and a large number of APIs. ## Solution To allow us to proceed granularly, we're going to keep both `bevy_color::Color` (new) and `bevy_render::Color` (old) around until the migration is complete. However, simply doing this directly is confusing! They're both called `Color`, making it very hard to tell when a portion of the code has been ported. As discussed in bevyengine#12056, by renaming the old `Color` type, we can make it easier to gradually migrate over, one API at a time. ## Migration Guide THIS MIGRATION GUIDE INTENTIONALLY LEFT BLANK. This change should not be shipped to end users: delete this section in the final migration guide! --------- Co-authored-by: Alice Cecile <[email protected]>

# Objective - Simplify `Srgba` hex string parsing using std hex parsing functions and removing loops in favor of bitwise ops. This is a follow-up of the `bevy_color` upstream PR review: bevyengine#12013 (comment) ## Solution - Reworked `Srgba::hex` to use `from_str_radix` and some bitwise ops; --------- Co-authored-by: Rob Parrett <[email protected]>

Upstreaming bevy_color. This provides a new set

a01f513

of color types and operations for Bevy.

Upstreaming bevy_color. This provides a new set

e9ed79c

of color types and operations for Bevy.

alice-i-cecile added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets X-Controversial There is active debate or serious implications around merging this PR labels Feb 20, 2024

JMS55 added this to the 0.14 milestone Feb 20, 2024

JMS55 self-requested a review February 20, 2024 22:07