imgui icon indicating copy to clipboard operation
imgui copied to clipboard

Added support for color space conversion in DX12 shader

Open adv-sw opened this issue 1 year ago • 13 comments

This enhancement/bugfix performs dynamic linear color space conversion with new ImGuiConfigFlags_OutputLinear configuration flag applied & performs standard ImGui behaviour without. Thus is backwards compatible & non breaking.

When targeting linear color space rendertargets, ImGuiConfigFlags_OutputLinear flag ensures the output is color correct.

When the flag is ommited & a linear render target selected, the result is blown out colors as reported in various bug reports associated with this issue.

Patch fixes the issue DX12 backend only. Other backends can be upgraded in the same manner but are not featured in this PR.

Color space conversion reference here : https://community.acescentral.com/t/srgb-piece-wise-eotf-vs-pure-gamma/4024

adv-sw avatar Aug 19 '24 19:08 adv-sw

Since it removes the SRGB transfer curve (as opposed to ONLY removing a gamma curve), it's probably worth naming it ImGuiConfigFlags_ConvertSRGBToLinear, or somesuch, to be more explicit. There are lots of common color spaces that use other transfer functions. Although only SRGB is supported by Dear ImGui today, it would be nice to future proof the API in case more color space support is ever on the table. For me, it's more than hypothetical, as I'm maintaining a little color science library meant for realtime, and use it in my own (private) fork :) (https://github.com/meshula/nanocolor) ImGuiConfigFlags_ConvertSRGBToLinear would be a lot easier for me to adapt to my own work, because I would know to consume "traditional" SRGB values, and covert them to my linear rendering color space, whatever it might be.

meshula avatar Aug 19 '24 22:08 meshula

No objection to the flag being named ImGuiConfigFlags_ConvertSRGBToLinear.

adv-sw avatar Aug 20 '24 04:08 adv-sw

Drawing to a linear render target with this patch enabled :

https://github.com/user-attachments/assets/dcbf2c5e-857f-4e4b-b7f4-e5c25bb29ed4

Drawing to a linear render target without this patch enabled :

https://github.com/user-attachments/assets/ca15f2e0-dd77-4c69-8395-747278cf8b16

adv-sw avatar Aug 20 '24 09:08 adv-sw

Fix in pixel shader is the optimal solution because it color corrects everything. Previous PRs which partially fixed by tweaking specific color values are incomplete solutions as they dont color correct color selectors or custom drawn content.

Note one should not merge multiple color correction solutions on the same platform as multiple color correction steps will result in an incorrect result. Anything corrected multiple times will display in the wrong color. It'll look wrong.

adv-sw avatar Aug 20 '24 09:08 adv-sw

Why not doing the conversion in the vertex shader?

ocornut avatar Aug 20 '24 13:08 ocornut

the conversion is not done in the vertex shader because if you look at how colour space conversion works, each channel (r,g,b) is modified indepenently, it's not simply a brightness adjust. the only place where you have the final resolved colors is the pixel shader, hence the color space resolve must occur there.

adv-sw avatar Aug 20 '24 13:08 adv-sw

To explain it a different way, the fragment shader can only linearly interpolates the values coming from the vertex shader because the vertex shader doesn't know about srgb encoding.

If we think about two red vertices, one with a value of zero, and one with a value of 1, half way between them, in the fragment shader, I'll get 0.5. If I apply pow(2.2) on the 0.5 value in the fragment shader, I'll get the linear framebuffer value I want, about 0.22.

If I apply the pow in the vertex shader, the values are still 0 and 1, so the fragment shader will get the value 0.5.

meshula avatar Aug 20 '24 16:08 meshula

The only outstanding issue is a consideration of how that pixel shader is used. If each entity is only presented once to a single buffer using it, imgui effectively operates in linear space, as required by the flag. If there's any level of nested operations, we'll need to pull out a present pixel shader & a straight op pixel shader so things aren't color space converted multiple times.

My test looks right, but there could be more complex imgui operations that need considering.

adv-sw avatar Aug 21 '24 01:08 adv-sw

Why are using a linear format for the render target if you want 0.5 -> 0.22?

pdoane avatar Aug 22 '24 08:08 pdoane

pow(0.5, 2.2) = 0.22

that's approximately sRGB to linear for 0.5 sRGB input. as to why folks want linear render targets, that's outside the scope of this bug. look up linear workflow.

adv-sw avatar Aug 22 '24 12:08 adv-sw

Right, so why not have the fixed function HW do this for you? I am not following why you want to change the pixel shader.

pdoane avatar Aug 22 '24 13:08 pdoane

Sorry I missed the comment that you are using render targets that do not have the HW support - yes this makes sense.

I would extend it from a flag to an enumeration though so that other transfer functions could be supported later (e.g. ST2084).

pdoane avatar Aug 22 '24 14:08 pdoane

extend is follow up PR. lets not have feature creep, plz.

adv-sw avatar Aug 24 '24 16:08 adv-sw