-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[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. |
thanks for the precision, I tried improving naming |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(because we do need the color fixes in this pr for 0.5 😄)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: |
Color
can 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 (
Color
andLinearColor
) to be able to use the different variants in all place where aColor
is 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
,blue
makes it clearer there are differences