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

OpenGL: Unconditionally do glDisable(GL_FRAMEBUFFER_SRGB) because we do our own sRGB conversion #95433

Merged

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Aug 12, 2024

Fixes #95313

I think this is the right thing to do, because:

  • OpenGL will only do the linear to sRGB conversion enabled via glEnable(GL_FRAMEBUFFER_SRGB) on FBO's that have a color attachment which uses an sRGB internal format. So, it will do the conversion on GL_SRGB8_ALPHA8, but not on GL_RGBA8
  • Godot's renderer doesn't normally use any sRGB internal formats for render buffers, because we do the linear to sRGB conversion ourselves in our shaders
  • We're only using GL_SRGB8_ALPHA8 in OpenXR, because if we don't, the OpenXR compositor will assume the content is linear, and it'll do another linear to sRGB conversion. So, we're forced to use that as the internal format, in order to signal that we're giving it sRGB data
  • It should be safe to do glDisable(GL_FRAMEBUFFER_SRGB) unconditionally, because Godot's OpenGL renderer isn't even relying on that anyway (as explained above in my second point in this list)

The main thing I'm not sure about, is if doing this in RasterizerGLES3::begin_frame() is the right place to do it? We don't actually need to do it every frame, but I couldn't find an obvious central place to do it, It works fine in GLES3::Config::Config() which only runs once, but we're only querying config there, not setting it, so it seemed inappropriate. I'd appreciate any advice about this!

UPDATE: Moved to RasterizerGLES3::RasterizerGLES3() based on advice from Clay.

This PR also partially reverts the changes from #74892, which did glDisable(GL_FRAMEBUFFER_SRGB), but only when rendering viewports with use_xr set to true (which doesn't help the composition layers). With this PR doing it unconditionally, that is no longer necessary.

@dsnopek dsnopek force-pushed the openxr-composition-layers-srgb branch from 0c47c6a to dfcff4e Compare August 12, 2024 17:41
@dsnopek dsnopek requested a review from clayjohn August 12, 2024 17:47
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@decacis
Copy link
Contributor

decacis commented Aug 13, 2024

I can confirm this fixes #95313 I tested both on Quest native and with Quest Link.

4.3-rc3 This PR
godot_4 3-rc3 godot_4 3-rc3_95433

And it's supposed to look like this PR :)

Copy link
Contributor

@devloglogan devloglogan left a comment

Choose a reason for hiding this comment

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

Tested this out by making a few changes/comparisons in the meta composition layer sample, looks good to me!

@akien-mga akien-mga merged commit ae9fb96 into godotengine:master Aug 16, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
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.

OpenXR composition layers perform sRGB conversion on android, when color space is already sRGB
5 participants