Improved PMREM by elalish · Pull Request #710 · google/model-viewer

elalish · 2019-08-15T22:39:27Z

Fixes #587
Fixes #632

This is an improvement to the very simple PMREM approximation we were using before. It also includes several fixes to the three.js MeshStandardMaterial that we use. These changes mostly made HDR environment maps look more realistic, but also included is a change which cleans up the aliasing we were getting from high frequency normal maps. There is still room for improvement, but it's significantly better than what it was. I also updated our lighting & environment page to better showcase our rendering with HDR.

…into improvedPMREM

cdata

There is still room for improvement

Can you share your thoughts on specific areas for future improvement?

Generally this looks really good. I have to trust you that some of these changes work as advertised until #682 , but maybe you would be willing to show off with some screenshots in the mean time? 🙏

src/constants.ts

cdata · 2019-08-19T15:05:41Z

src/three-components/PMREMGenerator.ts

    (cubeTarget: WebGLRenderTargetCube, renderer: WebGLRenderer):
        WebGLRenderTarget => {
-          const {cubeUVRenderTarget, cubeLods, meshes} = setup(cubeTarget);
+          const roughnessExtra = [0.5, 0.7, 1.0];


I eventually determined that these were referring to lods, but from this line the purpose of these numbers seems kind of obscure.

cdata · 2019-08-19T15:06:45Z

src/three-components/PMREMGenerator.ts

          });
          meshes.forEach((mesh) => {
-            (mesh.material as Material).dispose();
+            // (mesh.material as Material).dispose();


Did we decide that we aren't going to dispose of this stuff? If so, maybe the meshes should just be a static array that gets updated as needed?

The shaders take longer to compile than they take to run, so it's very important that we don't dispose of them, since three does such a nice job caching them. Are we worried about a leak here? The programs are always identical, so three ensures they don't get duplicated. We shouldn't delete them until will not be loading any new environments anymore, and basing that on page navigation sounds reasonable?

cdata · 2019-08-19T15:08:58Z

src/three-components/PMREMGenerator.ts

+      const lodMax = 8;
+      // lodBase is the mip Level that is integrated to form all of the extra
+      // levels, but is not output directly into the PMREM texture. DO NOT
+      // CHANGE, as the Blur shader below is hard coded for this size.


src/three-components/PMREMGenerator.ts

cdata · 2019-08-19T15:22:33Z

src/three-components/shader-chunk/common.glsl.js

+    vec3 direction = vec3(clamp(uv, -1.0, 1.0), 1.0);
+    uv = abs(uv);
+    float over = max(uv.x, uv.y) - 1.0;
+    if(over > 0.0) direction.z -= over;


This is an interesting tweak. What case is being handled here?

Added a comment above.

cdata · 2019-08-19T15:24:42Z

src/three-components/shader-chunk/cube_uv_reflection_fragment.glsl.js

-#define cubeUV_faceSize (256.0)
-#define cubeUV_maxMipLevel (8.0)
+const float cubeUV_maxMipLevel = 8.0;
+const float cubeUV_minMipLevel = 3.0;


I almost wish these could be shared constants across this shader chunk and the PMREM generator (also mipBase). Or maybe uniforms? But that would be unnecessary since we can just "preprocess" them into the program source here..

Yeah, really they should be uniforms, but since this is internal to three.js, we'll have to wait until it's upstreamed for that.

cdata · 2019-08-19T15:29:27Z

src/three-components/shader-chunk/normalmap_pars_fragment.glsl.js

+
+		vec3 perturbNormal2Arb( vec3 eye_pos, vec3 surf_norm ) {
+
+			// Workaround for Adreno 3XX dFd*( vec3 ) bug. See #9988


Is this file forked from Three.js? If so, can we document the nature of the changes? (ditto for lights_fragment_maps.glsl.js)

Yes, I can do that.

cdata · 2019-08-19T15:32:13Z

src/three-components/shader-chunk/cube_uv_reflection_fragment.glsl.js

-  float f = fract(mip);
+vec4 textureCubeUV(sampler2D envMap, vec3 sampleDir, float roughness) {	
+  float filterMip = 0.0;
+  if(roughness >= 0.7){


Nit: kinda funky formatting in this conditional, maybe match the one below?

cdata · 2019-08-19T15:36:22Z

src/three-components/PMREMGenerator.ts

+// This is a hack because IE claims that a shader which unrolls a loop to access
+// 96 texture lookups has "complexity which exceeds allowed limits", however 48
+// seems to be fine, so we subsample. This could be improved if someone cares a
+// lot about IE.


I wonder if there are low-end devices that might also have issues with this.

Separately, it might be cleaner to read if this was written as a preprocessor directive. Although as I write that, I'm not 100% sure if that will prevent IE from barfing when linking the shader.

That's a good idea; I'll give that a try.

Was it too cumbersome to use Three.js' defines property for ShaderMaterial to do this?

Yeah, those are really for defining constants, not just a switch. I noticed this is the way three tends to do it internally.

…into improvedPMREM

cdata · 2019-08-22T21:18:04Z

src/three-components/PMREMGenerator.ts

+ * even more filtered mips at the same lodMin resolution, associated with higher
+ * roughness levels. These extra mips are stored at X rather than Y offsets to
+ * help distiguish them. In this way we maintain resolution to smoothly
+ * interpolate diffuse lighting while limiting sampling computation.


cdata · 2019-08-22T21:20:39Z

src/three-components/shader-chunk/envmap_physical_pars_fragment.glsl.js

-
-		//float envMapWidth = pow( 2.0, maxMIPLevelScalar );
-		//float desiredMIPLevel = log2( envMapWidth * sqrt( 3.0 ) ) - 0.5 * log2( pow2( blinnShininessExponent ) + 1.0 );
+	// (elalish) Changed from Blinn-Phong to Trowbridge-Reitz distribution and added anti-aliasing.


cdata · 2019-08-22T21:21:24Z

src/three-components/shader-chunk/envmap_physical_pars_fragment.glsl.js

 	}

-	vec3 getLightProbeIndirectRadiance( /*const in SpecularLightProbe specularLightProbe,*/ const in GeometricContext geometry, const in float blinnShininessExponent, const in int maxMIPLevel ) {
+	// (elalish) Changed the input from blinnShininessExponent to roughness, so that we can use the Trowbridge-Reitz distribution instead.


cdata · 2019-08-22T21:21:45Z

src/three-components/shader-chunk/envmap_physical_pars_fragment.glsl.js


 		#endif

+		// (elalish) Mixing the reflection with the normal is more accurate and keeps rough objects from gathering light from behind their tangent plane.


cdata · 2019-08-22T21:23:33Z

src/three-components/shader-chunk/normalmap_pars_fragment.glsl.js

+			vec3 N = normalize( surf_norm );
+			mat3 tsn = mat3( S, T, N );
+
+            // (elalish) Biased the normal mip to anti-alias the normal's screen-space derivatives


cdata · 2019-08-22T21:23:47Z

test/index.html

+  <!-- Uncomment the following script in order to debug shader errors in Sauce Labs. When 
+       a shader fails to compile, it logs a console.error ("Shader failed to compile!") before it logs
+       the debug output from WebGL. Without this script, WCT bails out on the first error, so you 
+       never see the useful log, which tends to be especially critical for IE11. -->


cdata

So beautilful Ship it!

Improved PMREM (google#710) * starting to work * blurShader working * updated IBL lookup * fixed roughness mapping * gave fidelity tests orbit controls * added antialiasing, corrected texelSize * added single-pixel mip * refactored blurShader * added bottom of mipmap chain * refactored antialiasing into cubeUV * extra mipmaps working * fixed moire mostly * simplified PMREM * minMip level => 3 * tweaked filtering function * removed debug export * switching const to #define for IE * fixed IE shader complexity bug * addressing feedback * refactored IE11 #defines * merging master * updated asset paths * replaced shader const with #define to support IE11 * fixed another const and a small math error * using defines in gaussianBlur

elalish added 17 commits August 7, 2019 16:43

starting to work

b6ff97e

blurShader working

d9a67d9

updated IBL lookup

2b99039

fixed roughness mapping

b6f6272

gave fidelity tests orbit controls

4b0df97

added antialiasing, corrected texelSize

c527f9e

added single-pixel mip

dea25ce

merging master

3b94f47

refactored blurShader

1815a2d

added bottom of mipmap chain

4f254bd

refactored antialiasing into cubeUV

0c4a4b6

extra mipmaps working

9c193b0

fixed moire mostly

716094a

simplified PMREM

d394277

minMip level => 3

67369a1

tweaked filtering function

87c6548

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

02b91cb

…into improvedPMREM

elalish requested a review from cdata August 15, 2019 22:39

elalish self-assigned this Aug 15, 2019

elalish added 2 commits August 15, 2019 16:02

removed debug export

31b35a4

switching const to #define for IE

561c972

elalish requested a review from mrdoob August 16, 2019 18:01

fixed IE shader complexity bug

8667f04

cdata suggested changes Aug 19, 2019

View reviewed changes

elalish added 2 commits August 19, 2019 09:58

addressing feedback

414727f

refactored IE11 #defines

e1fd801

elalish requested a review from cdata August 20, 2019 15:26

elalish mentioned this pull request Aug 20, 2019

Unified skybox and environment #712

Merged

elalish added 2 commits August 21, 2019 14:53

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

c6077f5

…into improvedPMREM

merging master

729ed69

elalish added 5 commits August 21, 2019 16:09

merging master

d45661f

updated asset paths

a39efc3

replaced shader const with #define to support IE11

7f4f647

fixed another const and a small math error

9846219

using defines in gaussianBlur

62353b8

cdata reviewed Aug 22, 2019

View reviewed changes

cdata approved these changes Aug 22, 2019

View reviewed changes

elalish merged commit 52c38a3 into master Aug 22, 2019

elalish deleted the improvedPMREM branch August 22, 2019 23:15

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

ViliusLuneckas mentioned this pull request Nov 11, 2019

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

Closed

11 tasks


		vec3 perturbNormal2Arb( vec3 eye_pos, vec3 surf_norm ) {

		// Workaround for Adreno 3XX dFd*( vec3 ) bug. See #9988


		#endif

		// (elalish) Mixing the reflection with the normal is more accurate and keeps rough objects from gathering light from behind their tangent plane.

Conversation

elalish commented Aug 15, 2019

Uh oh!

cdata left a comment

Choose a reason for hiding this comment

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!

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!

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!

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!

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!

2 participants