Skip to content

Fix color space issues with sRGB framebuffer in Graph Editor#2402

Merged
jstone-lucasfilm merged 6 commits into
AcademySoftwareFoundation:mainfrom
mialana:fix-graph-editor-color-space
May 23, 2025
Merged

Fix color space issues with sRGB framebuffer in Graph Editor#2402
jstone-lucasfilm merged 6 commits into
AcademySoftwareFoundation:mainfrom
mialana:fix-graph-editor-color-space

Conversation

@mialana

@mialana mialana commented May 21, 2025

Copy link
Copy Markdown
Contributor

Related issue and Dev Days PR for Viewer

This PR addresses color space discrepancies / logic in MaterialX Graph Editor.


Summary of Problem:

Before, the graph editor used an sRGB framebuffer (i.e. glEnable(GL_FRAMEBUFFER_SRGB)) for the entire GUI, meaning both the rendered material (with its color values given in linear space) and the ImGUI components had an automatic "to-sRGB" transformation applied to them.

Although this is desired for the material preview, which is a rendered, PBR result and needs to be color corrected to display space, it has adverse results on ImGUI components that handle color. This is because currently, ImGUI is not completely compatible with sRGB-enabled framebuffers (ref: 578, 1724, 2943 4890, 6583). To exemplify a problem that arises, alpha-blending is calculated incorrectly when using an sRGB framebuffer.

Compare a "correct" color picker rendered w/o sRGB-enabled framebuffer with a color picker rendered w/ sRGB-enabled framebuffer, which is how it was done before this PR. I gave it the same color value, but in linear space:

Screenshot 2025-05-21 at 16 55 20 Screenshot 2025-05-21 at 16 57 52

There is clearly a non-negligible difference in both the color ramp and the overall canvas (which is the result of a completely different blending equation being applied, as commented on here). This calculation error results in the color picker UI component being in a nonlinear display space and should be avoided.


Resolution Implemented:

  1. To avoid these calculation errors in ImGUI, the sRGB framebuffer should be disabled for the UI. However, the rendered material view needs to still have a sRGB transformation applied to it. To solve this, I used the ImGui::GetWindowDrawList()->AddCallback() function to to inject a call toglEnable(GL_FRAMEBUFFER_SRGB) into ImGUI's iterative draw list right before the materialview texture is drawn, and disable it immediately after.

This effectively maintains the original material render result, but solves the color problems in the UI.

  1. Then, regarding the original issue, Color3::linearToSrgb() and Color3::srgbToLinear() are used to transform between color spaces. The result is below.

Before: User previews rendered material and color picker swatch in display space, but the actual color value numbers are actually in the linear range. These values will not match up in other applications, such as Adobe products, which is misleading.
Screenshot 2025-05-21 at 13 29 01

After: User always interacts with color in display space (pick + view), but behind-the-scenes, the color is written / stored in linear space.
Screenshot 2025-05-21 at 16 26 17

Rendered material result is successfully maintained:

Screenshot 2025-05-21 at 17 19 02 Screenshot 2025-05-21 at 17 18 24
  1. Lastly, a simple tooltip is added to the color picker component, clarifying the difference between front-facing UI values and stored values.
Screenshot 2025-05-21 at 17 44 40

Please let me know if I can clarify on any of the points I mentioned above. I tried to balance digital color principles / correctness with overall UI experience in these changes. Thanks!

@lfl-eholthouser lfl-eholthouser left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this issue @mialana. I think the changes look great!

Comment thread source/MaterialXGraphEditor/Graph.cpp Outdated
ImGui::SameLine();
ImGui::ColorEdit3("##color", &temp[0], ImGuiColorEditFlags_NoInputs);

// read material value in converted display space

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I want to flag that this pr #2395 is also modifying the color3 and color4 section of setConstant()

@jstone-lucasfilm jstone-lucasfilm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks great to me, @mialana, and I've added two minor suggestions to match the coding style of the MaterialX project.

Comment thread source/MaterialXGraphEditor/Graph.cpp Outdated
Comment thread source/MaterialXGraphEditor/Graph.cpp Outdated
@mialana

mialana commented May 23, 2025

Copy link
Copy Markdown
Contributor Author

I have just made the style fixes. Thank you @lfl-eholthouser and @jstone-lucasfilm!

@jstone-lucasfilm jstone-lucasfilm changed the title Fix color space issues with RGB-enabled framebuffer in Graph Editor Fix color space issues with sRGB framebuffer in Graph Editor May 23, 2025
@jstone-lucasfilm jstone-lucasfilm merged commit c1e7122 into AcademySoftwareFoundation:main May 23, 2025
32 checks passed
@mialana mialana deleted the fix-graph-editor-color-space branch May 14, 2026 23:56
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.

3 participants