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

HalfFloat PMREM #22998

Merged
merged 7 commits into from
Dec 15, 2021
Merged

HalfFloat PMREM #22998

merged 7 commits into from
Dec 15, 2021

Conversation

elalish
Copy link
Contributor

@elalish elalish commented Dec 10, 2021

We've been rendering to HalfFloat textures for several months now for model-viewer's auto-generated environment lighting without complaint, so it seems that at this point HalfFloat texture support is pretty universal (finally!). The only reason I wrote PMREMGenerator to use RGBE internally was because at the time there was not decent floating point rendering support. This made the code complicated and slow because I had to do manual bilinear interpolation (since decode was needed before mixing).

Here I've removed all that by switching PMREM to use HalfFloat internally. I suppose it'll take twice the GPU RAM (16 bits vs 8), but hopefully that's more than offset by skipping a bunch of exponentiation. I've tested on my Macbook Pro and my Pixel 3 and can't see any difference to our glTF renders. I'd appreciate some more device testing, but I'm fairly confident given our widely deployed use of HalfFloat render targets in MV.

@mrdoob @WestLangley

@elalish
Copy link
Contributor Author

elalish commented Dec 10, 2021

Looks like some problems with the node materials; anyone have any pointers on that? I haven't used those.

@elalish
Copy link
Contributor Author

elalish commented Dec 10, 2021

Also, does anyone have a convenient performance benchmarking tool for this? I'd be interested to know how much of a difference it makes (primarily to frame rate).

@WestLangley
Copy link
Collaborator

/ping @sunag for node

@WestLangley
Copy link
Collaborator

So far, the only issue I see is with the webgl_shaders_ocean.html example

before
webgl_shaders_ocean

after
webgl_shaders_ocean

@elalish
Copy link
Contributor Author

elalish commented Dec 10, 2021

Oh, I think that might be because I removed part of the encodings_fragment that I didn't think anyone else was using. Easy enough to put that back.

@WestLangley WestLangley added this to the r136 milestone Dec 11, 2021
@sunag
Copy link
Collaborator

sunag commented Dec 11, 2021

/ping @sunag for node

There were some issues this month about NodeMaterial but all related with legacy version, legacy version will be discontinued. I am working on the update. In case of this PR was merged, I can fix some urgent issue about it.

@elalish
Copy link
Contributor Author

elalish commented Dec 15, 2021

@WestLangley Thanks for noticing that problem; turns out it was not what I first thought. Instead, the toneMapping changed (it's off during PMREM.fromScene()) but the sky shader was not being recompiled because it was not marked as needsProgramChange. I've added this, since it seems like the correct thing to do. Basically this bug was always there, but I used to change several more things during PMREM, which caused the sky to correctly recompile before.

@elalish
Copy link
Contributor Author

elalish commented Dec 15, 2021

@sunag You can see in the screenshots I updated that this broke something in the pmrem node; seems likely related to the toneMapping issue I found above.

@WestLangley
Copy link
Collaborator

@elalish Currently, the webgl_tonemapping.html example sets the material update flag when tone mapping changes:

renderer.toneMapping = toneMappingOptions[ params.toneMapping ];
mesh.material.needsUpdate = true;

Maybe that can be avoided. I'll defer to @Mugen87 regarding the implementation details.

Also, remember to consider the material.toneMapped property.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 15, 2021

Maybe that can be avoided. I'll defer to @Mugen87 regarding the implementation details.

With @elalish's change to WebGLRenderer, manually setting mesh.material.needsUpdate = true; in webgl_tonemapping should not be necessary anymore.

@mrdoob mrdoob merged commit 2768965 into mrdoob:dev Dec 15, 2021
@mrdoob
Copy link
Owner

mrdoob commented Dec 15, 2021

Beautiful! Thanks!

@WestLangley
Copy link
Collaborator

As I said, ...

Currently, the webgl_tonemapping.html example sets the material update flag when tone mapping changes:

@mrdoob
Copy link
Owner

mrdoob commented Dec 15, 2021

You mean that we still have to remove that line, right?

@WestLangley
Copy link
Collaborator

Yes... and that would be a good test that the code changes are working correctly.

And maybe a Migration note would be advisable.

@Mugen87 Mugen87 mentioned this pull request Dec 15, 2021
gero3 pushed a commit to gero3/three.js that referenced this pull request Dec 16, 2021
* changed format

* PMREM RGBE->HalfFloat

* fixed toneMapping problem

* update screenshots

* Update WebGLRenderer.js

* Update WebGLRenderer.js

Co-authored-by: Michael Herzog <[email protected]>
@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 21, 2021

@sunag
Copy link
Collaborator

sunag commented Dec 21, 2021

@Mugen87 Probably yes., I was thinking it release would be in the Christmas,.. I was working in NodeEditor updates, but I will fix first.

@mrdoob
Copy link
Owner

mrdoob commented Dec 21, 2021

@sunag you can always check here the date in which every release is due:
https://github.com/mrdoob/three.js/milestones

sunag added a commit to sunag/three.js that referenced this pull request Dec 22, 2021
mrdoob pushed a commit that referenced this pull request Dec 22, 2021
* fix keyword example

* fix NodeMaterial ColorSpace: #22998

* variable params.length to ARCTAN: #22930

* NodeMaterial: fix CLEARCOAT

* possible fix to: #23054

* fix NodeMaterial: toon example: #22989
chubei-oppen added a commit to chubei-oppen/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 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
@elalish elalish deleted the HalfFloatPMREM branch February 10, 2022 22:33
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.

5 participants