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

sRGB awareness for Color #616

Merged
merged 11 commits into from
Oct 8, 2020
Merged

sRGB awareness for Color #616

merged 11 commits into from
Oct 8, 2020

Conversation

julhe
Copy link
Contributor

@julhe julhe commented Oct 2, 2020

#585 raised awareness for a sRGB aware Color struct. Basically, colors that where imported from Photoshop, GIMP or any other application will not match up with bevy, as bevy interpretate the color as linear, while it is in sRGB.

Summary

grafik

  • Color::rgb and Color::rgba are now sRGB aware per default, so the color will be transformed into linear space.
  • Color::rgb_linear and Color::rgba_linear allows for linear colors.
  • A new trait SrgbColorSpace allows for conversion on f32 level. I explicitly didn't implemented the trait for Color as I didn't want even the possiblity to treat Color as a sRGB color. It is suppose to be linear, always. 😈

Follow-up Issues

  • Naming might be sub-optimal. Should Color::rgb be actually Color::rgb_srgb? (Thats confusing in my opinion, but feedback is welcome)
  • Color::rgb and Color::rgba can't be used for const since they relly on traits, see Meta tracking issue for const fn rust-lang/rust#57563. (Color::rgb_linear and Color::rgba_linear however can)
  • All colors that rely on Color::rgb and Color::rgba are darker now.

@julhe julhe changed the title sRGB awareness for Colorspace sRGB awareness for Color Oct 2, 2020
@karroffel karroffel added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Oct 2, 2020
@cart
Copy link
Member

cart commented Oct 2, 2020

Brilliant. At first glance this all looks good. However I think I would value "const-ness" over having a trait for conversions. Can we just expose the conversion logic as const functions and use those?

@julhe
Copy link
Contributor Author

julhe commented Oct 2, 2020

I've tried that, but also the conversion logic relies on powf, which is also a trait. 😅

@cart
Copy link
Member

cart commented Oct 2, 2020

Hmm then I guess we'll need to wait / not be const anymore. Turns out float-ops in const functions is a much more complicated topic than I thought 😄

@julhe julhe marked this pull request as ready for review October 2, 2020 19:42
crates/bevy_render/src/color.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/colorspace.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/color.rs Outdated Show resolved Hide resolved
julhe and others added 3 commits October 2, 2020 22:54
Co-authored-by: Gray Olson <gray@grayolson.com>
Co-Authored-By: Gray Olson <gray@grayolson.com>
@cart
Copy link
Member

cart commented Oct 3, 2020

Something that will almost certainly be an issue with this new api:

let mut color = Color::rgb(0.6, 0.0, 0.0);
assert!(color.r != 0.6);
color.r = 0.6;
assert!(color.r == 0.6);

This will be confusing for people. I think we need to make the color fields private and expose them via srgb getters/setters.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Great work! This looks good to me for now, once we wrap up the comment below. I think there are two followups to this pr we should do:

  1. Adjust example colors to use srgb. I'll do that!
  2. Try resolving the "to/from conversion precision" issue, as illustrated by test_color_components_roundtrip. We could, for example, store a colorspace enum internally (ex: struct Color { colorspace: ColorSpace::srgb(1.0, 0.0, 0.0, 0.0) }). The main drawback is that the inner machinery is more complicated. We would still want the r() g() b() methods to return srgb colorspace and it should byte-convert to linear. We could also consider having a Color type (which is stored as srgb) and ColorLinear (which is stored as linear). However this can't happen yet because its currently assumed Color is directly Byteable to linear bytes. I plan on rethinking the Uniform/RenderResources derives, which might alleviate this restriction.

crates/bevy_render/src/color.rs Outdated Show resolved Hide resolved
@cart
Copy link
Member

cart commented Oct 7, 2020

Haha one more request. As this is a change that will immediately break people on master, can we add a changelog entry in this pr?

@julhe
Copy link
Contributor Author

julhe commented Oct 8, 2020

Like this?

@CleanCut
Copy link
Member

CleanCut commented Oct 8, 2020

@julhe I suggest something like this:

## Unreleased

### Added

- [Another fast compile flag for macOS][552]

### Changed

- Breaking Change: [sRGB awareness for `Color`][616]
  - Color is now assumed to be provided in the non-linear sRGB colorspace, and constructors such as `Color::rgb` and `Color::rgba` will be converted to linear sRGB under-the-hood.
  - New methods `Color::rgb_linear` and `Color::rgba_linear` will accept colors already in linear sRGB (the old behavior)
  - Individual color-components must now be accessed through setters and getters: `.r`, `.g`, `.b`, `.a`, `.set_r`, `.set_g`, `.set_b`, `.set_a`, and the corresponding methods with the `*_linear` suffix.

[552]: https://github.com/bevyengine/bevy/pull/552
[616]: https://github.com/bevyengine/bevy/pull/616

Julian Heinken and others added 2 commits October 8, 2020 12:45
Co-Authored-By: Nathan Stocks <cleancut@github.com>
Co-Authored-By: Nathan Stocks <cleancut@github.com>
@cart
Copy link
Member

cart commented Oct 8, 2020

Alrighty lets merge this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants