bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Use `LinearRgba` for user-facing APIs, rather than `Color`

Open alice-i-cecile opened this issue 1 year ago • 1 comments

Objective

As discussed in https://github.com/bevyengine/bevy/issues/12056, widespread use of the Color type (which is an enum holding variants that map to all of the supported color spaces) is not ideal. Right now, it is used in virtually all user-facing configuration (e.g. gizmo builders, clear color, fog settings, BackgroundColor components). This decision was primarily made to ease the initial migration: it maps closely to the previous bevy_render::Color enum (but stores types instead of fields). By contrast, internal, rendering-focused abstractions store LinearRgba: this is the color format that users expect.

We should make a decision on this pattern before shipping the new color type to users. There will not be a perfect decision: there are very real trade-offs with each choice, but the choice should be deliberate, rather than incidental.

Fixes https://github.com/bevyengine/bevy/issues/12212.

Solution

This PR implements the simple and efficient solution 2, laid out in #12212, which simply uses LinearRgba as our standard user-facing color type.

Advantages: All color spaces can be directly converted into it. Save on conversion costs. Great for PBR operations.

Disadvantages: LinearRGB is the worst color space for most perceptual operations. Data about the original color space is thrown away.

Change Log

All uses of Color in the engine have been removed. Instead, LinearRgba is consistently used to store information about colors across UI and rendering.

Migration Guide

All fields which previously held a Color now store a LinearRgba. The full list of types and fields is:

  • TODO, populate list

When defining a color palette for your application, consider defining these in a human-friendly color space: such as Hsla, Labch or Srgba. When spawning objects in your game, convert the stored colors into LinearRgba using the From/Into traits.

alice-i-cecile avatar Apr 25 '24 19:04 alice-i-cecile

Opened https://github.com/alice-i-cecile/bevy/pull/167, which migrates bevy_gizmos to use LinearRgba :)

BD103 avatar Apr 26 '24 15:04 BD103

I think this is more D-Straighforward than D-Trivial, because of the amount of files that need to be changed. (I'm fine with either, though. After all, you did write the description!)

BD103 avatar May 03 '24 15:05 BD103

Just as a small warning, this PR may make usage of the Color type significantly decrease. Instead of storing a Color constant for various things, I could see developers just hardcoding a LinearRgba instead.

One could argue that this is actually a good thing, because it forces developers to choose the color representation that bests fits their project. Either way, it's something to keep in mind.

BD103 avatar May 03 '24 17:05 BD103

Just as a small warning, this PR may make usage of the Color type significantly decrease. Instead of storing a Color constant for various things, I could see developers just hardcoding a LinearRgba instead.

Yep, reducing usage of Color is the intent. I'm considering submitting a follow-up PR removing the Color type completely: it doesn't have clear use cases and is a lot of work to maintain.

I'm not thrilled with all of the Into calls however.

alice-i-cecile avatar May 03 '24 18:05 alice-i-cecile

I'm not thrilled with all of the Into calls however.

A few ideas for reducing this:

  1. Encourage calling .into() once, then reusing the calculated result.
    • I'm thinking like lazy static here? My hope is that rustc just automatically does this for you.
  2. Convert all colors within bevy_color::palettes to LinearRgba.
    • This is most definitely a follow-up PR thing.
  3. Add impl Into<LinearRgba> as parameters.
    • This is a last resort in my opinion, because it can sacrifice efficiency for developer experience, specifically because of the extra monomorphization and potential of calling color.into() within Update outside of the developer's control

BD103 avatar May 03 '24 18:05 BD103

Putting this on hold until this thread on Discord resolves itself, deciding whether this is a good idea or not.

BD103 avatar May 03 '24 18:05 BD103

Initial stress testing showed no meaningful difference. I don't think there's serious performance gains here in real applications.

My current stance is that this PR should be closed, with ColorMaterial::emissive getting swapped to LinearRgba and everything else staying the same.

alice-i-cecile avatar May 03 '24 19:05 alice-i-cecile

I agree with closing this. The PR seems like too much change and too little gain. Since Alice and I agree on this, I'm going to close it.

BD103 avatar May 03 '24 20:05 BD103