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

Added support for color space conversion in DX12 shader #7904

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

adv-sw
Copy link

@adv-sw adv-sw commented Aug 19, 2024

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

@meshula
Copy link

meshula commented Aug 19, 2024

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.

@adv-sw
Copy link
Author

adv-sw commented Aug 20, 2024

No objection to the flag being named ImGuiConfigFlags_ConvertSRGBToLinear.

@adv-sw
Copy link
Author

adv-sw commented Aug 20, 2024

Drawing to a linear render target with this patch enabled :

imgui_color_correct.mp4

Drawing to a linear render target without this patch enabled :

imgui_color_incorrect.mp4

@adv-sw
Copy link
Author

adv-sw commented Aug 20, 2024

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.

@ocornut ocornut changed the title Added support for color space conversion generically over all ImGui operations as required. Added support for color space conversion in DX12 shader Aug 20, 2024
@ocornut
Copy link
Owner

ocornut commented Aug 20, 2024

Why not doing the conversion in the vertex shader?

@adv-sw
Copy link
Author

adv-sw commented Aug 20, 2024

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.

@meshula
Copy link

meshula commented Aug 20, 2024

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.

@adv-sw
Copy link
Author

adv-sw commented Aug 21, 2024

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.

@pdoane
Copy link

pdoane commented Aug 22, 2024

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

@adv-sw
Copy link
Author

adv-sw commented Aug 22, 2024

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.

@pdoane
Copy link

pdoane commented Aug 22, 2024

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
Copy link

pdoane commented Aug 22, 2024

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).

@adv-sw
Copy link
Author

adv-sw commented Aug 24, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants