bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
35.23k stars 3.47k forks source link

Ensure that bevy_color works with HDR #12089

Closed eero-lehtinen closed 4 months ago

eero-lehtinen commented 6 months ago

What problem does this solve or what need does it fill?

Currently bevy_color stresses everywhere in its documentation that values should be contained in [0.0, 1.0]. This assumption is incorrect when working with HDR values.

There is also no simple way to construct HDR values from SDR values. Before it could be done with Color::RED * 5.0 (I'm not sure if this is correct but I'm doing it this way).

What solution would you like?

We could update documentation with all color spaces to include HDR values. Then make sure that all color functions support these extended values. E.g. adjust_luminance currently does explicit clamping which is broken with HDR.

Supporting straight up multiplications for constructing HDR values seems error prone, but some kind of function for this would be convenient.

What alternative(s) have you considered?

We could also do a separate HDR color type. Not sure how that would work with our current color spaces.

Additional context

viridia commented 6 months ago

I've been using the palette crate as inspiration for what methods and traits to support - but only as a starting point, not as a rigid spec. Now, palette doesn't have any traits or methods that would address HDR issues, but they do have some traits that deal with the inverse: Clamp and IsWithinBounds.

eero-lehtinen commented 6 months ago

I guess at this point I would be happy if there was a small mention in the documentation that large values are allowed with HDR, and Mul implementation for Srgb and LinearRgb to manipulate HDR brightness (palette has this too).

All color space conversions may not work very well with HDR but maybe it doesn't matter if they weren't even designed for that.

eero-lehtinen commented 6 months ago

I've been using the palette crate as inspiration for what methods and traits to support - but only as a starting point, not as a rigid spec. Now, palette doesn't have any traits or methods that would address HDR issues, but they do have some traits that deal with the inverse: Clamp and IsWithinBounds.

It is a bit weird that the whole palette crate has only a single mention of HDR, but we need to clarify it.

I feel that clamping functions aren't needed for this issue, but could be a future enhancement.

bushrat011899 commented 6 months ago

So I think this needs a well considered solution beyond just "clamping" like palette does. The real problem is we need a tone mapping solution. Clamping is a form of tone mapping, where [0,∞) is mapped to [0, 1] by clamping all values above 1. This is a simple solution with great performance, but can look less than ideal since all that information is just lost.

eero-lehtinen commented 6 months ago

We do have a tonemapping pipeline already, if this is what you mean. We don't need to do any tonemapping in bevy_color.

eero-lehtinen commented 6 months ago

Ok I found that the palette library has a stimulus trait, that does at least mention that high color values exist. Color is called stimulus in our tonemapping shader too.

fintelia commented 6 months ago

I think it would be worth working through precisely which uses of color should allow values outside the [0, 1] range. A few cases that do/don't make sense:

  1. When specifying a material's albedo (sometimes called "base color" or similar), values greater than 1 are non-physical because albedo represents the fraction of light that a surface reflects.
  2. When specifying a light source's color there is separate intensity scale value, so values larger than 1 aren't needed (though larger values would be physically meaningful).
  3. One case where larger color values might make sense is as the clear color for a render target.
NthTensor commented 5 months ago

I think it would be worth working through precisely which uses of color should allow values outside the [0, 1] range. A few cases that do/don't make sense:

  1. When specifying a material's albedo (sometimes called "base color" or similar), values greater than 1 are non-physical because albedo represents the fraction of light that a surface reflects.

  2. When specifying a light source's color there is separate intensity scale value, so values larger than 1 aren't needed (though larger values would be physically meaningful).

This is not quite correct, as I understand it. WebGPU (and by extension wgpu) expects all colors to be specified according to the Srgb primaries. However, WebGPU also allows rendering to attachments with wider gamut color spaces, like P3. This is the display color format for most apple products. If users wish to supply textures with colors in the P3 gamut (outside of the normal Srgb triangle) WebGPU expects them to use non-normalized Srgb color points. So it's actually quite reasonable to have values outside of [0, 1] for materials or lights. It is true however that that the luminance of these colors should be in [0, 1]. I would call these "display-referred". By contrast, what we have been calling "HDR values" in this thread (colors with unbounded luminance) I would call "scene-referred".

If we are talking about documentation changes, the no. 1 thing I would want is to stop talking about colors as clamped/not-clamped or hdr/sdr but as scene-referred/display-referred or (in the very few cases where it is important) as in-gamut/out-of-gamut.

With that language established, I think a few points become very clear:

  1. Both scene and display referred colors may have components outside of [0, 1].
  2. Users should never specify scene-referred colors using the color api. When necessary, they should instead specify a display-referred color along with an intensity in some well defined physical units.
  3. It is still possible users may use the color api to represent scene-referred colors. This is fine, so long as they never try to mix scene and display referred colors together.
  4. The color api should make no assumptions about what colors are valid and what colors are not, because it isn't aware of the display color-space and doesn't know if any given color is display or scene referred.
  5. Clamping colors is colors is bad and should be strongly discouraged. Wgpu can pretty much always be trusted do that for us: correctly, to the right bounds, and only when necessary.

We seem to be largely compliant with these points already. There's some other interesting stuff here to do with gamma curves and tone-mapping but I don't think any of it really has to do with the color api.

JMS55 commented 4 months ago

On the rendering side of things, excluding asset (are we loading embedded color profiles?) and color handling:

EDIT: We probably also need to adjust our PBR math/API carefully. E.g. StandardMaterial::base_color (albedo) is based on the fraction of light reflect [0, 1], and is a physical measurement, and not a color.


I also want to note that in terms of hardware support your OS needs to have HDR enabled (Windows 11 has a toggle in settings), you need a semi-recent GPU, and you need a compatible monitor. DisplayHDR v1.1 is the important standard, really DisplayHDR 500+. It requires 500+ nits peak brightness, high DCI-P3 color gamut coverage, 10bit color, and other stuff (DisplayHDR 400 requires only 8bit color, and high sRGB coverage).


I want to note down some terms that I've had to learn:

alice-i-cecile commented 4 months ago

This should be substantively fixed by #13307. Please feel free to open follow-up issues for other required features or docs :)