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 RGBEEncoding and RGBEFormat. #23060

Merged
merged 2 commits into from
Dec 21, 2021
Merged

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Dec 21, 2021

Related issue: -

Description

This PR removes RGBEEncoding and RGBEFormat.

That means RGBELoader and HDRCubeTextureLoader do not support UnsignedByteType anymore.

@mrdoob mrdoob merged commit cee9706 into mrdoob:dev Dec 21, 2021
@mrdoob mrdoob added this to the r136 milestone Dec 21, 2021
@mrdoob
Copy link
Owner

mrdoob commented Dec 21, 2021

Thanks!

@chubei-oppen
Copy link
Contributor

Is it possible to revert this? There're still devices out there not supporting floating point texture extensions.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 31, 2021

No sorry. We definitely remove all inline GLSL decodes. You have to enhance the build-in shaders by yourself if you still need RGBA8 HDR textures.

@chubei-oppen
Copy link
Contributor

I see. May I ask what's the motivation behind it? It certainly makes keeping our fork up-to-date with main repo more difficult, so I'm curious about what we are paying for.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 31, 2021

The general approach of inline decode and encodes is problematic. It produces wrong texture filtering and can break blending. Since WebGL 2 is now mature enough, we've decided to make Float16 mandatory for HDR workflows.

chubei-oppen added a commit to chubei-oppen/three.js that referenced this pull request Dec 31, 2021
@chubei-oppen
Copy link
Contributor

TBH I don't know anyone complaining RGBEEncoding because it's only used in IBL. The filtering was handled in built-in shader code and no blending will be taking place. But thanks for you answer still.

chubei-oppen added a commit to chubei-oppen/three.js that referenced this pull request Dec 31, 2021
chubei-oppen added a commit to chubei-oppen/three.js that referenced this pull request Dec 31, 2021
@mrdoob
Copy link
Owner

mrdoob commented Dec 31, 2021

@chubei-oppen

There're still devices out there not supporting floating point texture extensions.

What devices are those?

It certainly makes keeping our fork up-to-date with main repo more difficult,

Out of curiosity, what other modifications did you need to do that required a fork?

@chubei-oppen
Copy link
Contributor

@mrdoob Sorry, not devices. Platforms actually.

WeChat Mini Program only supports WebGL and a very limited set of extensions(not the same set depending on OS/device). And they won't add WebGL2 support in the near future(we know because we're working closely with Mini Program group). But WeChat has over 1 billion users so I expect a lot of people sticking to below r136 just to keep their app running on WeChat. These people will be Chinese developers so the feedback won't be as strong as the actual impact because of the language barrier.

Other mini programs, namely Taobao from Alibaba and TicTok, have similar levels of WebGL support the last time I checked it(several months ago).

As of our fork, there are two big things that are important to us but I don't see hope merging into upstream:

chubei-oppen added a commit to oppenfuture/three.js that referenced this pull request Jan 1, 2022
@joshuaellis joshuaellis mentioned this pull request Jan 3, 2022
9 tasks
@mrdoob
Copy link
Owner

mrdoob commented Jan 3, 2022

WeChat Mini Program only supports WebGL and a very limited set of extensions(not the same set depending on OS/device). And they won't add WebGL2 support in the near future(we know because we're working closely with Mini Program group).

That's unfortunate...

Right now there are 3 code paths: WebGL1, WebGL2 and WebGPU...

We do not have the resources to maintain all of them and we have to align with the stacks modern browsers are focusing on (WebGL2 and WebGPU).

Hopefully WeChat and co will update to WebGL2 and WebGPU soon.

This shouldn't require a fork. We implemented this workaround too last year.

@chubei-oppen
Copy link
Contributor

We do not have the resources to maintain all of them and we have to align with the stacks modern browsers are focusing on (WebGL2 and WebGPU).

Totally understand. Your work on three.js has been incredible, and we owe huge thanks to all three.js contributors. I was not complaining about removing this feature, just trying to give information that you might not have and see if it changes the decision. We'll pay what we have to for supporting outdated platforms.

This shouldn't require a fork. We implemented this workaround too last year.

If you're referring to #21202, that's a different bug. The bug we were trying to fix can be reproduced with this page. You should see a black quad in a gray background, but with Adreno 6xx GPU the quad would be blue.

chubei-oppen added a commit to oppenfuture/three.js that referenced this pull request Jan 4, 2022
chubei-oppen added a commit to chubei-oppen/three.js that referenced this pull request Jan 4, 2022
chubei-oppen added a commit to oppenfuture/three.js that referenced this pull request Jan 4, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 4, 2022

This shouldn't require a fork. We implemented this workaround too last year.

If you're referring to #21202, that's a different bug. The bug we were trying to fix can be reproduced with this page. You should see a black quad in a gray background, but with Adreno 6xx GPU the quad would be blue.

Hmm, is that because Adreno ignores the precision of numbers when using structs?
https://twitter.com/garrettkjohnson/status/1453446051738767361

@chubei-oppen
Copy link
Contributor

Hmm, is that because Adreno ignores the precision of numbers when using structs? https://twitter.com/garrettkjohnson/status/1453446051738767361

Yes, I believe so.

@mrdoob
Copy link
Owner

mrdoob commented Jan 4, 2022

Grrrr

@gkjohnson
Copy link
Collaborator

Hmm, is that because Adreno ignores the precision of numbers when using structs? https://twitter.com/garrettkjohnson/status/1453446051738767361

I've made an issue in the Khronos WebGL repo for this but so far not a whole lot more info, yet:

KhronosGroup/WebGL#3351

@mrdoob
Copy link
Owner

mrdoob commented Jan 11, 2022

@chubei-oppen

WeChat Mini Program only supports WebGL and a very limited set of extensions(not the same set depending on OS/device). And they won't add WebGL2 support in the near future(we know because we're working closely with Mini Program group).

Would it be possible for you to ask them what are the reasons they won't add WebGL2?

@chubei-oppen
Copy link
Contributor

They have other priorities. There's no strong enough motivation for them to put resources into WebGL2.

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 oppenfuture/three.js that referenced this pull request Mar 16, 2022
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.

4 participants