New PMREM by elalish · Pull Request #684 · google/model-viewer

elalish · 2019-07-23T20:31:55Z

Fixes #671

Really, the point of this change is to change PMREM to not have sparkles anymore and to be faster. In this PR, it basically makes the rendering mirror the case where you use an HDR floating-point environment with three's Standard Material, but now it can work with an RGBE texture, thus requiring no special gl extensions. That handles very HDR environments well, but does a poor job on roughness > ~0.2. I'm working on a new method to improve that.

This PR demonstrates the method I found to extend the Standard Material, but it does not show the diffs for the shader chunks (since it has nothing to compare them to here). The only changes to envmap_physical_pars_fragment.glsl.js were lines 31 and 92. cube_uv_reflection_fragment.glsl.js on the other hand, was completely rewritten. PMREMGenerator.ts pulls many ideas from PMREMGenerator.js and PMREMCubeUVPacker.js in three, but is also a rewrite.

…into remove_pmrem_flag

* generated environment map now RGBM16 * switched RGBM16 to RGBE * started gaussianBlur * gaussianBlur compiles * gaussian blur functional * gaussianBlur properly parameterized * target caching works * removed extraneous cache, cleaned up generation * Reuse renderer in tests * switched cubeGenerators * reuse renderer in TextureUtils-spec * addressing feedback * generated environment map now RGBM16 * switched RGBM16 to RGBE * started gaussianBlur * gaussianBlur compiles * gaussian blur functional * gaussianBlur properly parameterized * switched cubeGenerators * reuse renderer in TextureUtils-spec * addressing feedback * fixed IE11 shader bug

…del-viewer into newPMREM

elalish · 2019-07-23T21:26:21Z

@cdata I have a strange problem with the current state of this branch. Try loading examples/lighting-and-environment.html: I get a black render for the second one down the page (the rest are fine), and sometimes even a white skybox. However, if I comment out all the others, then this one renders just fine. I assume I've messed up a cache or dispose or something, but I haven't found it. A second set of eyes would be much appreciated.

elalish · 2019-07-24T17:26:33Z

@mrdoob This is more FYI than requesting a code review, but I thought you might be interested in where I'm headed as far as updating PMREM and the MeshStandardMaterial. Also, onBeforeCompile is great, but it would be nice if the uniforms were more accessible / extensible. Maybe I'm missing something?

mrdoob · 2019-07-24T21:47:26Z

I wouldn't rely too much in onBeforeCompile(). It's meant to allow people modify built-in materials, but wasn't meant to be more than that.

Glad to see that you're reworking the PMREM code. Feel free to propose the change upstream whenever you think is ready. WestLangley and others will be able to take a good look.

cdata

This is looking very promising. I had tons of feedback, but most of it is minor stuff.

src/three-components/CachingGLTFLoader.ts

cdata · 2019-07-26T17:39:59Z

src/three-components/CachingGLTFLoader.ts

    const gltf = cloneGltf(await cache.get(url)!);
    const model = gltf.scene ? gltf.scene : null;

+    // This is a patch to three's handling of PMREM environments.


Is there a better description of what this actually does somewhere?

Yeah, I'm working on a doc, but it's not going to fit in a comment.

src/three-components/CachingGLTFLoader.ts

src/three-components/PMREMGenerator.ts

src/three-components/TextureUtils.ts

cdata · 2019-07-26T21:36:18Z

src/three-components/TextureUtils.ts

    };

-    return renderTarget;
+    return cubeUVTarget;


If I understand the implementation correctly, this means you are sharing the same render target instance for all environment maps. That probably explains why you were only getting one working environment map.

src/three-components/cube_uv_reflection_fragment.glsl.js

cdata · 2019-07-26T21:39:55Z

src/three-components/envmap_physical_pars_fragment.glsl.js

@@ -0,0 +1,135 @@
+export default /* glsl */ `


There seems to be some pretty inconsistent indentation here. Is it possible to align it with cube_uv_reflection_fragment?

I left it intentionally with unchanged formatting from the three.js version since I only changed two lines and this makes the diff a lot nicer.

cdata · 2019-07-30T00:09:00Z

src/three-components/PMREMGenerator.ts

+    let offset = 0;
+    for (let i = 0; i <= maxLods; i++) {
+      const sizeLod = Math.pow(2, i);
+      const renderTarget = new WebGLRenderTargetCube(sizeLod, sizeLod, params);


Do we create an extra WebGLRenderTargetCube here and throw it away in the case that i === maxLods?

indeed, fixed.

cdata

Look great, works great. Ship it!

cdata · 2019-07-30T22:23:44Z

src/three-components/PMREMGenerator.ts

+ * (PMREM) from a cubeMap environment texture. This allows different levels of
+ * blur to be quickly accessed based on material roughness. It is packed into a
+ * special CubeUV format that allows us to perform custom interpolation so that
+ * we can support nonlinear formats such as RGBE.


cdata · 2019-07-30T22:33:00Z

src/three-components/PMREMGenerator.ts

+export const generatePMREM =
+    (cubeTarget: WebGLRenderTargetCube, renderer: WebGLRenderer):
+        WebGLRenderTarget => {
+          const {cubeUVRenderTarget, cubeLods, meshes} = setup(cubeTarget);


cdata · 2019-07-30T22:38:24Z

src/three-components/PMREMGenerator.ts

+          renderer.gammaOutput = gammaOutput;
+
+          cubeLods.forEach((target) => {
+            target.dispose();


Do we need to do any disposal of meshes here? Or are we explicitly relying on them not being disposed (on account of internal caching side-effects in Three.js)?

I'm not relying on it. Honestly I wasn't sure what needed disposal besides renderTargets.

cdata · 2019-07-30T22:39:56Z

src/three-components/PMREMGenerator.ts

+    (cubeTarget: WebGLRenderTargetCube, renderer: WebGLRenderer):
+        WebGLRenderTarget => {
+          const {cubeUVRenderTarget, cubeLods, meshes} = setup(cubeTarget);
+          // This hack in necessary for now because CubeUV is not really a


cdata

Looking great 🙌 we'll land #689 shortly to further improve things. Hopefully test flakes aren't a big deal and just Sauce being sucky 🙏

New PMREM (google#684) * removed pmrem attribute * removed pmrem attribute from non-code files * default environment map now uses PMREM * fixed tests * using PMREM for all cases, fixing google#565 * increasing sauce timeout to 3min * target caching works * removed extraneous cache, cleaned up generation * added LDR and HDR environment tests * updated goldens * Reuse renderer in tests (google#668) * Refactor envmap / skybox generation * default stage light to zero, removed hemisphere light (google#674) * added new PMREMGenerator * modified MeshStandardShader to show it can work * Switch environment generation from HalfFloat to RGBE (google#670) * generated environment map now RGBM16 * switched RGBM16 to RGBE * started gaussianBlur * gaussianBlur compiles * gaussian blur functional * gaussianBlur properly parameterized * target caching works * removed extraneous cache, cleaned up generation * Reuse renderer in tests * switched cubeGenerators * reuse renderer in TextureUtils-spec * addressing feedback * generated environment map now RGBM16 * switched RGBM16 to RGBE * started gaussianBlur * gaussianBlur compiles * gaussian blur functional * gaussianBlur properly parameterized * switched cubeGenerators * reuse renderer in TextureUtils-spec * addressing feedback * fixed IE11 shader bug * added envmap chunk * updated three's shaders * fixed shaders * shaders compile * fixed all WebGL errors * PMREMGenerator now takes any encoding of input texture * fixed several shader bugs * fixed mipmap generation * fixed maxLod * PMREM mostly working * recreate intermediate targets * fixed mipmap level selection * minor cleanup * addressing feedback * refactored PMREM * cleanup * dispose of temp materials * removed envMap hack * removed android sauce test

Emmett Lalish and others added 30 commits May 31, 2019 14:18

removed pmrem attribute

56384e6

removed pmrem attribute from non-code files

ea75251

default environment map now uses PMREM

8a4d719

fixed tests

f508eb4

using PMREM for all cases, fixing #565

b9a7ef8

Merge branch 'master' of github.com:GoogleWebComponents/model-viewer …

4d3a769

…into remove_pmrem_flag

Merge branch 'master' of github.com:GoogleWebComponents/model-viewer …

9cd1980

…into remove_pmrem_flag

merging master

0e977d3

increasing sauce timeout to 3min

fdf4cdd

target caching works

cb5c8f9

removed extraneous cache, cleaned up generation

44dd9f8

Reuse renderer in tests (#668)

5016911

Refactor envmap / skybox generation

6959a95

added LDR and HDR environment tests

06e01a5

updated goldens

0e59d9b

default stage light to zero, removed hemisphere light (#674)

19ba22d

added new PMREMGenerator

218cc74

modified MeshStandardShader to show it can work

578ef6e

added envmap chunk

954fd23

updated three's shaders

9b2fae8

Merge branch 'remove_pmrem_flag' of github.com:GoogleWebComponents/mo…

21f83af

…del-viewer into newPMREM

fixed shaders

1655d4b

shaders compile

fce04e6

fixed all WebGL errors

77b6ac3

PMREMGenerator now takes any encoding of input texture

2675085

fixed several shader bugs

b53345d

fixed mipmap generation

9bbbb71

fixed maxLod

f6915ac

PMREM mostly working

05d4849

elalish added 4 commits July 19, 2019 14:22

recreate intermediate targets

0732c7b

fixed mipmap level selection

b5bc8e8

merging master

c57d961

minor cleanup

7154974

elalish requested a review from cdata July 23, 2019 20:31

elalish self-assigned this Jul 23, 2019

elalish requested a review from mrdoob July 24, 2019 17:24

cdata suggested changes Jul 26, 2019

View reviewed changes

addressing feedback

feab7ba

cdata reviewed Jul 30, 2019

View reviewed changes

elalish added 2 commits July 30, 2019 15:07

refactored PMREM

d99f27c

cleanup

8ba7f41

elalish requested a review from cdata July 30, 2019 22:21

cdata previously approved these changes Jul 30, 2019

View reviewed changes

mrdoob previously approved these changes Jul 31, 2019

View reviewed changes

dispose of temp materials

347e5ac

elalish dismissed stale reviews from mrdoob and cdata via 347e5ac July 31, 2019 15:58

elalish added 2 commits August 1, 2019 09:53

removed envMap hack

82eb1fa

removed android sauce test

75db7a8

cdata approved these changes Aug 6, 2019

View reviewed changes

elalish merged commit 81b0d04 into master Aug 6, 2019

cdata mentioned this pull request Aug 12, 2019

Improved default material for loaded glTFs #650

Closed

cdata added this to the v0.6.0 milestone Aug 13, 2019

elalish deleted the newPMREM branch September 27, 2019 16:46

ViliusLuneckas mentioned this pull request Nov 11, 2019

HDR environment & incorrect roughness mrdoob/three.js#17911

Closed

11 tasks

Conversation

elalish commented Jul 23, 2019

Uh oh!

elalish commented Jul 23, 2019

Uh oh!

elalish commented Jul 24, 2019

Uh oh!

mrdoob commented Jul 24, 2019

Uh oh!

cdata left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdata left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdata left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants