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

EXRLoader: Stop RGBE/UnsignedByte support. #23039

Merged
merged 1 commit into from
Dec 17, 2021
Merged

EXRLoader: Stop RGBE/UnsignedByte support. #23039

merged 1 commit into from
Dec 17, 2021

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Dec 17, 2021

Related issue: #23007 (comment)

Description

With this PR EXRLoader stops the support for RGBE encoded textures.

Similar to the newly added LogLuvLoader, I suggest all HDR texture loaders only produce half and single precision floating point textures. The overall idea is to remove the GLSL texture decode in the engine.

This PR can introduce breakage for older devices which only support WebGL 1.

@Mugen87 Mugen87 requested a review from sciecode December 17, 2021 11:18
Copy link
Contributor

@sciecode sciecode left a comment

Choose a reason for hiding this comment

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

Everything looks in order, thanks for cleaning up @Mugen87 .

@WestLangley
Copy link
Collaborator

This PR can introduce breakage for older devices which only support WebGL 1.

I suggest the library remove support for WebGL 1 first -- then make changes such as this one.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 17, 2021

We've decided in #22689 to keep WebGL 1 support for some more years.

I think it's inappropriate to couple this PR with the removal of WebGL 1 support. A WebGL 1 only device can still support OES_texture_half_float/OES_texture_half_float_linear and thus use the loader as before.

Besides, we have stopped support for a features like THREE.Geometry gradually and not at once, too.

@sciecode
Copy link
Contributor

I suggest the library remove support for WebGL 1 first -- then make changes such as this one.

I don't feel strongly about removing RGBE or even adding luminance 8-bit so we have full backwards compatible options for WebGL1. So, I don't see the need to remove this stuff ASAP, but at the same time I've been a little out of the loop of how things are moving.

On different token, I question if anyone would even use such features, even for backwards compatibility you know? I'm kinda indiferent to it, but I do know that not having linear sampling on a texture is a very limitting restriction in plenty of scenarios.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 17, 2021

The real issue to me is the removal of GLSL decode/encode. In some sense, we have started this task by introducing SRGB8_ALPHA8 and I think it would be good for the project to move on with it. That means we want to decode data in JS (or via WebGL extensions) and encode/tone map via post processing.

To me the first step in this process is to remove getTexelDecodingFunction() in WebGLProgram. However, this can only be achieved by updating the loaders first.

@sciecode
Copy link
Contributor

sciecode commented Dec 17, 2021

The real issue to me is the removal of GLSL decode/encode. In some sense, we have started this task by introducing SRGB8_ALPHA8 and I think it would be good for the project to move on with it. That means we want to decode data in JS (or via WebGL extensions) and encode/tone map via post processing.

EXT_sRGB extension for WebGL1 is already not supported on Edge/IE . So our fallback support for GL1 contexts is already a bit compromised going that route. If the intended pipeline is indeed that, then it shouldn't be a problem dropping support for most encoded sampling on shaders and the loaders by extension IMO.

But again, I missed that discussion, so I'm assuming you knew the repercusions already, Whatever you guys shake on, is good with me.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 17, 2021

I was thinking to manually decode sRGB textures for WebGL 1 during the texture upload since SRGB8_ALPHA8 can't be used.

Besides, one could state that full HDR support is only guaranteed with WebGL 2 (because of the lack of WebGL 1 extensions on certain devices).

@mrdoob
Copy link
Owner

mrdoob commented Dec 17, 2021

I was thinking to manually decode sRGB textures for WebGL 1 during the texture upload since SRGB8_ALPHA8 can't be used.

Another option, if we really have to, would be to create a RenderTarget for each texture and convert it with a shader.

@mrdoob mrdoob added this to the r136 milestone Dec 17, 2021
@mrdoob mrdoob merged commit 9bdb60f into mrdoob:dev Dec 17, 2021
@mrdoob
Copy link
Owner

mrdoob commented Dec 17, 2021

Thanks!

@donmccurdy
Copy link
Collaborator

donmccurdy commented Dec 19, 2021

Between #23049, #23046, and #23039 (this PR), it sounds like the intention is entirely remove support for 32 bit-per-pixel HDR texture formats in WebGL 1.0, is that correct? The change could potentially create both performance and functional regressions for older WebGL 1.0 devices, is that something we could measure?

I suggest all HDR texture loaders only produce half and single precision floating point textures. The overall idea is to remove the GLSL texture decode in the engine.

I think I'm in favor of the second part, but not so sure about the first. What about internal formats like R11F_G11F_B10F or RGB9_E5? Not saying HDR loaders need to convert textures to these formats, but the engine should support them correct?

Besides, one could state that full HDR support is only guaranteed with WebGL 2 ...

Removing support for environment maps is effectively the same as removing support for PBR — I don't think we can claim to still support WebGL 1.0 if we start making this type of change.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 20, 2021

Between #23049, #23046, and #23039 (this PR), it sounds like the intention is entirely remove support for 32 bit-per-pixel HDR texture formats in WebGL 1.0, is that correct?

Yes, the goal is to require FP16 for a HDR workflow. Notice that #22998 needs to be included in your PR listing.

The change could potentially create both performance and functional regressions for older WebGL 1.0 devices, is that something we could measure?

I'm not aware of any kind of measuring. However, the disadvantages of not making this change (wrong blending and wrong hardware interpolation for all users) seems more relevant than supporting older devices.

What about internal formats like R11F_G11F_B10F or RGB9_E5?

I think it's okay to support them (at least technically) but I doubt they will have much relevance if no official loader is going to produce textures with such formats.

Removing support for environment maps is effectively the same as removing support for PBR — I don't think we can claim to still support WebGL 1.0 if we start making this type of change.

Um, I have the feeling the discussion goes in the wrong direction. As mentioned in #23039 (comment), WebGL 1.0 only devices can still use HDR if they support the respective (half) float extensions. I don't understand why this change is now coupled with a complete WebGL 1.0 support stop. There are certainly users who are not using HDR at all.

@sciecode
Copy link
Contributor

One aspect of requiring default half float extension for HDR envmaps is that most (pre ~2018 Android) mobiles devices that rely on WebGL1 contexts actually don't support OES_texture_half_float and/or EXT_sRGB. So we have a release that discontinues the classical HDR support for that class of devices. With the responsibility of defining fallback support ( if deemed necessary ) shifting to the users. I just wanted to point that out, because I think that is important to note that within the patch notes.

I understand why the change itself needs to happen in order to simplify shader workflow & standardize internal behavior. Which is both better in-line with GL2 and GPU working specs moving forward, and does bring compatibility guarantees on newer GL2 context. Making the working pipeline cleaner and more straight forward to work with.

Nowadays, I can't seem to find any predictive extension support metrics in order to figure out the actual impact of these changes, so it's hard to say things for sure. But model-viewer seems to have tested the workflow enough to warrant it safe, so I don't have any objections, but I would make sure support an optional solution for handling HDR on these devices if needed by the user.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 20, 2021

I just wanted to point that out, because I think that is important to note that within the patch notes.

It will be highlighted in the release notes and the migration guide that a HDR workflow now requires FP16 textures.

but I would make sure support an optional solution for handling HDR on these devices if needed by the user.

We can't provide a real fallback without polluting loader and shaders with legacy code again. If devs can't use THREE.HalfFloatType, they have to stick with r135. Or create a custom loader and enhance built-in materials via onBeforeCompile(). If three.js is WebGL 2 only at some point, users won't be able to upgrade the engine, too, if they still rely on WebGL 1.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Dec 20, 2021

Hm perhaps I'm confused... Aren't all .hdr files RGBE files? Are we dropping support for loading .hdr IBLs?

I think it's okay to support [R11F_G11F_B10F or RGB9_E5] (at least technically) but I doubt they will have much relevance if no official loader is going to produce textures with such formats.

We could add this to KTX2Loader if we wanted to. KTX2 containers support these formats but it'd just be zstd compression, not Basis compression.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 20, 2021

The RGBE image format has the .hdr extension, yes. You can still load RGBE files but the HDR texels will already be decoded on the CPU into (half) float. We just drop the support for uploading the RGBE decoded texels to the GPU since we have to decode in the shader in this case.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Dec 20, 2021

Ah ok! I'd forgotten that #22265 switched RGBELoader's output to float16 by default a few months ago, and it sounds like that fixed other issues too? I have no objection, then. 👍

@sciecode
Copy link
Contributor

sciecode commented Dec 20, 2021

We can't provide a real fallback without polluting loader and shaders with legacy code again. If devs can't use THREE.HalfFloatType, they have to stick with r135. Or create a custom loader and enhance built-in materials via onBeforeCompile(). If three.js is WebGL 2 only at some point, users won't be able to upgrade the engine, too, if they still rely on WebGL 1.

Makes sense to me, I just wonder if we should keep the shaderChunk encodings_pars_fragment to facilitate discontinuation upgrades even if we don't use internally, but I don't think it's a big deal eitherway. Let's give it a try.

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.

5 participants