-
-
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
Don't use the polymorphic Color
enum in components and user-facing config
#12212
Comments
I think the option 5 is the ideal solution, but the disadvantage is a little hard to handle, depending on the circumstances. |
Yeah, as a maintainer (and SME-Docs) I'd really like to avoid the fragmentation of solution 5. If there's really compelling benchmarks showing that the performance costs matter in some cases but not others I'd consider it. Personally I think that solution 4 (the cached hybrid Color) is the best choice, but it's not clearly better than either a |
I think some of the disadvantages of "linear everywhere" can be overcome with proper tooling. When we talk about user experience, there are two sets of users that we need to address:
For the first group, an interactive design tool can paper over the difficulties fairly easily. Writing a color editor widget is every UI developer's fun project, and it can allow the user to select whatever color space they prefer. The widget can be written to store the color space as a global user preference, so the user won't have to select the color space every time. Typically, the way I write color editors is that they edit all color spaces simultaneously: that is, I keep a separate state variable for each color space, and then synchronize them as needed. The reason for doing this is to avoid "gimble lock" - if I'm editing HSL and the color is black, and then I switch to RGB and the back to HSL, I don't want to to lose the value of the hue slider, even though "black" has no hue. The second case is a bit trickier. The good news is that we've made it easy and relatively efficient for users to convert back from linear into whatever color space they find comfortable. |
If we end up going with "linear everywhere", I'd be in favor of marking the |
|
Playing devils advocate for option 4, using // 8-Bytes
struct CachedLinearRgb {
// Alpha would need simple conversion
alpha: std::num::NonZeroU16,
red: f16,
green: f16,
blue: f16,
}
// 20 Bytes
struct CachedColor {
// Still 8 Bytes
linear: Option<CachedLinearRgb>,
// 12 Bytes using f16
original: Color,
} Going mad with power for a moment, we could get some pretty wild powers for colour caching if we collapsed the struct Color {
alpha: f16,
linear_rgb: (f16, f16, f16),
srgb: (f16, f16, f16),
hwb: (f16, f16, f16),
ok_lightness: f16,
ok_ab: (f16, f16),
ok_ch: (f16, f16),
} Since the |
I like constructing colors using and when manipulating colors in Systems everything has drawbacks. rgb gives you "always valid" colors, but the way colors combine and gradient isn't intuitive and is harder to use. Oklch lightness and hue may be human-readable, but chroma and what is "a valid color" is definitely not. So IMO any choice is going to have a requirement to do some color science research for end-users who want to programmatically modify color. Inside shaders, I tend to incur conversion to oklch to maintain perceptual lightness, etc, or just not converting at all depending on what I'm doing. My plan when I saw #12163 was to do roughly option 4 here when I cared about exposing user-configurable, human-readable color operation. When defining palettes I often find that defining palettes concretely rather than programmatically is the dominant approach. For example: tailwind, catppuccin, and custom design systems I've seen all tend to have a long list of explicitly chosen colors, not programmatically generated scales. Defining in Oklch and immediately (const-ly?) converting to LinearRgba doesn't seem like a problem to me either, making option 2 seem fine to me too. It does seem like option 2 would potentially push "I want to do all my color picking/processing in Oklch" (or lch, or Okhsl, or some future approach) to an external crate, which might be where it should be anyway? At least its a choice to incur the additional cost. Could be an additional Oklch component that handles serializing to the LinearRgb. Option 4 seems like the best for usability, while Option 2 seems like the best "nothing more than Bevy needs" approach in terms of resource usage and expectations. |
I'd tentatively go for option 2. It's the simplest option for something I feel shouldn't be overcomplicated. I also quickly looked it up and found that both Unity and Godot store their colors in this way. To colloborate on what Chris said. If I need to do color work and I do not have an editor, I usually pick from a list of named colors or, if I need it to look good, go to an online color picker and copy the hex from there. And if an editor is available, I would just use that. But that could easily do the conversions for you. The biggest problem I have with 2 is that the gains are largely theoretical. Conversions are really simple, mathematically and also computationally, so while it could save time, I'm not sure how much. A benchmark would be good I suppose, but I don't think it's worth doing given how hard it is to define a realistic workload in the first place and we could end up stuck interpreting trace data. It's better to head that off at the pass and go with the cheapest and simplest option, so 2. |
Closing in favor of #13214. Given that the perf implications were not measurable in my tests, I'd prefer to maximize stability and ergonomics. |
What problem does this solve or what need does it fill?
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 previousbevy_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.
Goals
LinearRgba
once, and ideally only use 4 bytes of memory (4 f32).Possible solutions
Color
enum.Advantages: easy to migrate, stores the exact stable values provided by users.
Disadvantage: requires conversion into
LinearRgba
every time the data needs to be extracted to the GPU. Requires runtime checks for color types, most useful operations are not defined on this type.LinearRgba
everywhere.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.
Oklcha
) everywhere.Advantages: much better for interpolation, blending and manipulation.
Disadvantages: very unusual choice, expensive repeated conversions. Not all color spaces can be directly converted.
LinearRgba
inside ofColor
, and lock down direct access to this type to ensure validity.Advantages: Preserves information about the original color space, doesn't involve constant re-conversions. Converting out to other color types is faster.
Disadvantages: API becomes more complex, memory usage at rest doubles from 4 to 8 bytes. Still don't get to do nice color operations directly.
Advantages: We can get the best of all worlds, and make the right choice for each domain.
Disadvantages: Much more complex to learn and maintain.
Additional context
This decision was split from #12163 to avoid excessive delays as we come to a decision here. The correct path forward for #12188 will depend on the decision made here.
cc @cart @IceSentry @rparrett @Trashtalk217 @viridia
The text was updated successfully, but these errors were encountered: