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

WebGLRenderer: Remove inline sRGB decode. #23129

Merged
merged 4 commits into from
Jan 4, 2022
Merged

WebGLRenderer: Remove inline sRGB decode. #23129

merged 4 commits into from
Jan 4, 2022

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Dec 31, 2021

Fixed #23109.

Description

This PR removes the sRGB inline decode in shaders. There are now new policies for sRGB workflows:

  • WebGL 2 requires the usage of RGBA8 for uncompressed sRGB encoded textures.
  • WebGL 1 now tries to use EXT_sRGB and the respective unsized formats. However, this only works by disabling mipmapping. If EXT_sRGB is not supported, a (slow) CPU decode is performed.

@sunag I've updated the node related code so that the examples still work. However, the node systems require some cleanup since methods like fromDecoding() and getTextureEncodingFromMap() should not be used anymore.

@mrdoob
Copy link
Owner

mrdoob commented Dec 31, 2021

image

@mrdoob mrdoob added this to the r137 milestone Dec 31, 2021
@mrdoob
Copy link
Owner

mrdoob commented Dec 31, 2021

If EXT_sRGB is not supported, a (slow) CPU decode is performed.

I guess we can replace this later with a GPU approach? @kenrussell suggested creating a WebGLRenderTarget for each texture and converting to linear with a shader.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 31, 2021

I've tried using render targets however this will making the code way more complex. The problem is that we can't trigger a render within WebGLTextures since this will mess up the uniform setup for the actual frame. Using WebGLRenderTarget would have to happen at an earlier stage. E.g. one could gather the sRGB encoded color textures in projectObject() and convert them. However, storing the converted textures without overwriting the original one is tricky, too. Besides, the current solution handles all sRGB encoded textures (even in custom shaders). Achieving the same with a render target approach is also more complicated.

Ideally, we don't have to go down this path and just use EXT_sRGB and CPU decodes until we completely remove WebGL 1 support.

@sunag
Copy link
Collaborator

sunag commented Dec 31, 2021

@sunag I've updated the node related code so that the examples still work. However, the node systems require some cleanup since methods like fromDecoding() and getTextureEncodingFromMap() should not be used anymore.

@Mugen87 No problem! It seems easy to do.
Great PR this.

@mrdoob mrdoob merged commit 05fc79c into mrdoob:dev Jan 4, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 4, 2022

Thanks!

@makc
Copy link
Contributor

makc commented Jan 20, 2022

until we completely remove WebGL 1 support.

and when is that, @Mugen87

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 20, 2022

see #22689 (comment)

0b5vr added a commit to pixiv/three-vrm that referenced this pull request Jan 28, 2022
0b5vr added a commit to pixiv/three-vrm that referenced this pull request Jan 28, 2022
chubei-oppen added a commit to oppenfuture/three.js that referenced this pull request Mar 16, 2022
chubei-oppen added a commit to oppenfuture/three.js that referenced this pull request Mar 16, 2022
chubei-oppen added a commit to oppenfuture/three.js that referenced this pull request Mar 16, 2022
chubei-oppen added a commit to chubei-oppen/three.js that referenced this pull request Mar 16, 2022
chubei-oppen added a commit to chubei-oppen/three.js that referenced this pull request Mar 16, 2022
chubei-oppen added a commit to oppenfuture/three.js that referenced this pull request Mar 16, 2022
@tangobravo
Copy link
Contributor

I'm not a big fan of the CPU WebGL1 fallback - doing that sRGB -> linear conversion into an 8-bit texture is likely to introduce pretty severe banding artefacts that wouldn't be apparent with the conversion in the shader.

0b5vr added a commit to pixiv/three-vrm that referenced this pull request Jun 7, 2023
…e conversion

This commit drops the support of pre-r137 (Jan 29, 2022)

This compat code used to be required to compensate for changes in shader codes that happened in r137
mrdoob/three.js#23129

Because of the recent changes related to color spaces, we found that it's difficult to maintain the compat codes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove sRGB inline decoding.
5 participants