[Merged by Bors] - color spaces and representation #1572
Conversation
|
I wouldn't call HSL a color space in itself. It can be applied to sRGB and linearRGB. |
mockersf
changed the title
|
thanks for the precision, I tried improving naming |
mockersf
changed the title
alice-i-cecile
added
C-Bug
|
Just started my review. Just pushed a proposal to remove |
examples/ecs/state.rs
Outdated
| material | ||
| .color | ||
| .set_b((time.seconds_since_startup() * 5.0).sin() as f32 + 2.0); | ||
| if let Color::Rgba { blue, .. } = &mut material.color { |
There was a problem hiding this comment.
I think this is a pretty major downside to the current approach. Interacting with srgb r, g, and b components directly will almost certainly be high-frequency operations. r() and set_r() ops will be missed. Most people shouldn't even need to know that Color could have other representations.
There was a problem hiding this comment.
This could be solved by doing internal conversions as necessary to get srgb components, or by breaking out each Color enum variant into its own struct (and doing conversions as necessary). That would mean that Color would need to be backed by srgb (and we would have HslColor, LinearSrgbColor, etc and From impls for conversions).
I think I like the enum (with an r() method that does internal conversions) more. But I'm curious what you think.
There was a problem hiding this comment.
I mainly like the enum more because it protects against conversion error except when absolutely necessary. But it also complicates the api / forces us to make hard choices about implicit behaviors.
There was a problem hiding this comment.
I'm not sure setting only a specific channel of the color is something that happens often in a game. I don't have an experience with "real" game development, but when I tried I handpicked a few colors, set them to consts with explicit names and swapped the colors completely
There was a problem hiding this comment.
Interacting with individual color channels comes into play for things like "animated materials", where you might want something to get more/less red over some period of time. Animating color happens in most visually polished games (although sometimes it happens inside shaders and often its an interpolation between two pre-defined colors). Ex: explosions, shield effects, outlines, ui transitions, etc. Animating channels individually is not uncommon.
if let Color::Rgba { red, .. } = &mut material.color {
*red = time.sin() * speed
} else {
// by not handling other cases we are introducing silent error cases
// if we handle each type individually or add error cases this gets very boilerplate-ey
}
// easier to type, easier to read, no need to handle each error case (but also implicitly does lossy conversions if not stored as rgba)
material.color.set_r(time.sin() * speed)All major engines (Unreal, Unity, Godot) have color channel properties/fields. I think our users will expect similar apis.
There was a problem hiding this comment.
I think whether or not Color should be an enum (or a simple sRGB struct) is a longer conversation. By adding the getters/setters we also buy time for that conversation by making this a non-breaking change.
There was a problem hiding this comment.
(because we do need the color fixes in this pr for 0.5 😄)
There was a problem hiding this comment.
fair enough, I can add setter/getter for rgb that will convert the color to rgba if needed 👍
There was a problem hiding this comment.
Awesome. Much appreciated. There are a lot of tradeoffs to discuss and its very possible that we'll end up with the exact impl currently in this pr (by removing the getters / setters), but I don't want to rush that decision now.
|
bors r+ |
`Color` can now be from different color spaces or representation: - sRGB - linear RGB - HSL This fixes #1193 by allowing the creation of const colors of all types, and writing it to the linear RGB color space for rendering. I went with an enum after trying with two different types (`Color` and `LinearColor`) to be able to use the different variants in all place where a `Color` is expected. I also added the HLS representation because: - I like it - it's useful for some case, see example `contributors`: I can just change the saturation and lightness while keeping the hue of the color - I think adding another variant not using `red`, `green`, `blue` makes it clearer there are differences Co-authored-by: Carter Anderson <mcanders1@gmail.com>
|
for future reference, the color as enum will greatly benefit from rust-lang/rfcs#2593 when it's available |
|
Pull request successfully merged into main. Build succeeded: |
bors
bot
changed the title
Colorcan now be from different color spaces or representation:This fixes #1193 by allowing the creation of const colors of all types, and writing it to the linear RGB color space for rendering.
I went with an enum after trying with two different types (
ColorandLinearColor) to be able to use the different variants in all place where aColoris expected.I also added the HLS representation because:
contributors: I can just change the saturation and lightness while keeping the hue of the colorred,green,bluemakes it clearer there are differences