Implement HDR image support (fixes #65) and transitions to using `scene.background` by jsantell · Pull Request #323 · google/model-viewer

jsantell · 2019-01-30T21:03:34Z

Broken up form of #237 with only HDR support and initial work for using scene.background, which greatly simplifies our skybox logic. The PR is big enough with enough test changes (and scene.background is necessary since we can't use two different mappings with the same texture), so PMREM can be in a follow up.

There was an issue with using scene.background with cube targets (mrdoob/three.js#15662) so the work around here is until we update to r101 with the change -- tracking the removal of this hack via #196.

The HDR environments aren't terribly exciting without PMREM unfortunately, so this is pre-work for that, and to scope PMREM issues in its own branch.

Of note, below is an LDR env (top) and an HDR env; it appears the HDR map does not handle any roughness values at all without the PMREM, at least for the specular (the other lights appear to be more diffused as roughness increases). Not sure what's going on there, not what I'd expect three to do (and exactly the same area I was looking at for mrdoob/three.js#15644, so a problem for another day if this is unintended. Either way, this should be thought of as part 1 of 2, i.e. not ready to promote/highlight.

jsantell · 2019-01-30T23:48:25Z

TIL IE doesn't have Array.prototype.find; repush test

jsantell · 2019-01-31T00:10:25Z

Object doesn't support property or method 'findIndex' oh come on

cdata

Some general feedback/questions:

Almost all of the first party code changes would not be required if Three.js r101 was available to us. Would it be better for us to wait for that release?
No tests seem to hit the code path for a newly introduced 350 line third party library. Can we add at least one test that hits that code path?

cdata · 2019-01-30T21:43:20Z

src/features/environment.js

-      this[$scene].skysphere.material.needsUpdate = true;
-      this[$currentCubemap] = cubemap;
-      this[$scene].model.applyEnvironmentMap(cubemap);
+      // TODO #196


Let's follow Google convention for TODO formatting: // TODO(#xxx): elevator summary of thing not done

cdata · 2019-01-30T21:45:00Z

src/features/environment.js

-      this[$setShadowLightColor](skysphereColor);
+      const parsedColor = new Color(color);
+
+      // TODO #196


cdata · 2019-01-30T21:45:58Z

src/features/environment.js

-      this[$scene].skysphere.material.needsUpdate = true;
+      this[$setShadowLightColor](parsedColor);

      // TODO can cache this per renderer and color


Does this one have an issue?

cdata · 2019-01-30T21:48:19Z

src/features/environment.js

-      this[$currentCubemap] = cubemap;
-      this[$scene].model.applyEnvironmentMap(this[$currentCubemap]);
+      const envmap = textureUtils.generateDefaultEnvMap();
+      this[$currentEnvMap] = envmap;


Formatting convention is a bit scattered here. Env and Environment are used interchangeably, and map is lowercased or capitalized depending on where you look. I would prefer Environment and capitalized Map.

cdata · 2019-01-30T21:49:31Z

src/features/environment.js

+    /**
+     * Work around for #196, can remove this function
+     * and set backgrounds on `scene.background` directly
+     * once upgraded to r100. Most of the logic from


I thought we upgraded to r100?

#312

Did you mean r101?

cdata · 2019-01-31T05:22:55Z

src/test/features/environment-spec.js

-    model,
-    'envmap-change',
-    e => textureMatchesMeta(e.value, {...meta, type: 'EnvironmentMap'}));
+const waitForEnvmap = (model, meta) =>


Envmap => EnvironmentMap throughout

cdata · 2019-01-31T05:26:04Z

src/test/three-components/ModelScene-spec.js

 }

-// Checks that the skysphere is within the camera's far plane
-function ensureSkysphereVisible(scene) {


Testing and assertions reduced here. What changed?

The positioning of the skysphere is in the hands of three now, and the environment feature tests cover whether or not it's visible, these tests here ensured the skybox was positioned and sized correctly

cdata · 2019-01-31T05:29:18Z

src/three-components/TextureUtils.js

   * @return {Promise<THREE.Texture>}
   */
  async load(url) {
+    const isHDR = /\.hdr$/.test(url);


Let's name this RegExp e.g., const HDR_FILE_RE = /\.hdr$/; and hoist it to the module scope.

cdata · 2019-01-31T05:31:20Z

src/three-components/TextureUtils.js

+      equirect.dispose();
+
+      return {envmap, skybox};
    } catch (e) {


e => error

cdata · 2019-01-31T05:31:51Z

src/three-components/TextureUtils.js

+        envmap.dispose();
      }
+
+      console.error(e);


Remove and/or re-throw

cdata · 2019-01-31T05:45:55Z

HDR looks so good 🤩thanks for all your work to make this happen @jsantell

jsantell · 2019-02-01T23:27:40Z

Addressed all comments, I think -- let me know if I missed any!

Removed the hack now that r101 has landed
Updated TODO comments
Added tests that use HDR textures
consistently use camel case environmentMap throughout

cdata

One minor question, but otherwise this looks awesome. Very exciting! 💖

cdata · 2019-02-01T23:32:08Z

examples/background-image.html

            <template>
                <model-viewer
                    id="toggle-image"
-                    backgroud-color="#ff0077"


Nice catch 👍

cdata · 2019-02-01T23:32:42Z

src/features/environment.js

-      const cubemap = textureUtils.generateDefaultEnvMap();
-      this[$currentCubemap] = cubemap;
-      this[$scene].model.applyEnvironmentMap(this[$currentCubemap]);
+      // TODO(#336): can cache this per renderer and color


🍻 thanks for filing!

cdata · 2019-02-01T23:34:11Z

src/test/three-components/TextureUtils-spec.js

+                 textures.skybox.texture, {mapping: 'Cube', url: EQUI_URL}))
          .to.be.ok;
+
+      // TODO (#215): Re-enable once PMREM is added


cdata · 2019-02-01T23:45:57Z

src/three-components/TextureUtils.js

  url: null,
-  // 'Equirectangular', 'EnvironmentMap'
-  type: null,
+  // 'Equirectangular', 'Cube'


cdata · 2019-02-01T23:46:41Z

src/three-components/TextureUtils.js

   * @return {Promise<THREE.Texture>}
   */
  async load(url) {
+    const isHDR = HDR_FILE_RE.test(url);


cdata · 2019-02-01T23:48:10Z

src/three-components/TextureUtils.js

  dispose() {
-    this.envMapGenerator.camera.renderTarget.dispose();
-    this.envMapGenerator = null;
+    this.environmentMapGenerator.camera.renderTarget.dispose();


Should EnvironmentMapGenerator have its own dispose method so that we don't have to dig this deeply into its implementation details here?

yeah, probably, this is a bit much

cdata · 2019-02-04T22:07:06Z

Excellent work 👍 LGTM

scene.background for skybox (fixes #196).

jsantell requested review from cdata and mrdoob January 30, 2019 21:03

jsantell force-pushed the scene-skybox branch from 5c8aac7 to 52ecd4e Compare January 30, 2019 21:07

jsantell force-pushed the scene-skybox branch from 52ecd4e to 42160e0 Compare January 30, 2019 23:48

jsantell force-pushed the scene-skybox branch from 42160e0 to bbab0d8 Compare January 31, 2019 00:10

cdata reviewed Jan 31, 2019

View reviewed changes

jsantell force-pushed the scene-skybox branch 2 times, most recently from 6143437 to deb62b1 Compare February 1, 2019 23:27

cdata previously approved these changes Feb 1, 2019

View reviewed changes

jsantell dismissed cdata’s stale review via bbc0929 February 4, 2019 20:26

jsantell force-pushed the scene-skybox branch from deb62b1 to bbc0929 Compare February 4, 2019 20:26

cdata previously approved these changes Feb 4, 2019

View reviewed changes

mrdoob previously approved these changes Feb 4, 2019

View reviewed changes

Implement HDR image support (fixes #65) and transitions to using

a8d9ad0

scene.background for skybox (fixes #196).

jsantell dismissed stale reviews from mrdoob and cdata via a8d9ad0 February 5, 2019 01:05

jsantell force-pushed the scene-skybox branch from bbc0929 to a8d9ad0 Compare February 5, 2019 01:05

jsantell requested a review from cdata February 5, 2019 01:06

cdata approved these changes Feb 5, 2019

View reviewed changes

jsantell merged commit 01e0de9 into master Feb 5, 2019

jsantell deleted the scene-skybox branch February 5, 2019 01:28

This was referenced Feb 6, 2019

Integrate accurate radiance maps via PMREMGenerator for HDR environments #338

Merged

(WIP) PMREM+HDR+r99 #237

Closed

Conversation

jsantell commented Jan 30, 2019

Uh oh!

jsantell commented Jan 30, 2019

Uh oh!

jsantell commented Jan 31, 2019

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!

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 commented Jan 31, 2019

Uh oh!

jsantell commented Feb 1, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdata commented Feb 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants