Skip to content
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

Use LinearRgba for user-facing APIs, rather than Color #13103

Closed

Conversation

alice-i-cecile
Copy link
Member

Objective

As discussed in #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 #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 alice-i-cecile added A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide X-Controversial There is active debate or serious implications around merging this PR labels Apr 25, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Apr 25, 2024
@BD103
Copy link
Member

BD103 commented Apr 26, 2024

Opened alice-i-cecile#167, which migrates bevy_gizmos to use LinearRgba :)

@alice-i-cecile alice-i-cecile added the D-Trivial Nice and easy! A great choice to get started with Bevy label May 3, 2024
@BD103
Copy link
Member

BD103 commented May 3, 2024

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!)

@alice-i-cecile alice-i-cecile added D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed D-Trivial Nice and easy! A great choice to get started with Bevy labels May 3, 2024
@alice-i-cecile alice-i-cecile marked this pull request as ready for review May 3, 2024 17:21
@BD103 BD103 self-requested a review May 3, 2024 17:54
@BD103
Copy link
Member

BD103 commented May 3, 2024

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.

@alice-i-cecile
Copy link
Member Author

alice-i-cecile commented May 3, 2024

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.

@BD103
Copy link
Member

BD103 commented May 3, 2024

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 BD103 added the S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help label May 3, 2024
@BD103
Copy link
Member

BD103 commented May 3, 2024

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

Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this is a big regression in terms of UX. I would really like to see benchmarks here to have a better sense. Conversion cost feels pretty theoretical and while I'm sure there are scenarios where it matters quite a bit I'd imagine there are many more where it doesn't matter at all. I'm coming at this from the perspective of wanting to support and make it easy for users to be expressive, which colors (🎨) my bias. In Nannou, we support Into<Color> everwhere and can convert to LinearRgba internally if that's the final decision, but imagining myself as a raw Bevy consumer, the API feels less playful. I'm less likely to experiment with other color spaces on a whim. Yeah, .into() isn't much, but IME this kind of friction matters.

Personally, a split approach of using Into<Color> by default and LinearRgba where there's a know performance concern doesn't seem that bad to me? I've missed out on some of this discussion but I'm not sure that consistency is really the most important thing here. I think that it's sensible that there are "high level" and "low level" color operations and while there may be some blurriness between the two, that seems more like a case by case basis to make a decision on.

I'd also like to ask whether we could stick with Into<Color> but provide "raw" alternatives for where performance matters. I know this means the user would have to know when they're getting hurt by this, but I think that's an option I'd consider too.

Thanks for doing the massive refactor on this though! These kinds of changes are tedious and sometimes you need to see it to have an opinion. Curious what others think.

@alice-i-cecile
Copy link
Member Author

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.

@BD103
Copy link
Member

BD103 commented May 3, 2024

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 BD103 closed this May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Color Color spaces and color math A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Benchmarking This set of changes needs performance benchmarking to double-check that they help X-Controversial There is active debate or serious implications around merging this PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Don't use the polymorphic Color enum in components and user-facing config
3 participants