Remove experimental-pmrem flag by elalish · Pull Request #591 · google/model-viewer

elalish · 2019-05-31T22:03:15Z

Fixes #565
Fixes #601
Fixes #396
Fixes #648
Fixes #574
Fixes #336
Fixes #616
Fixes #346
Fixes #647
Fixes #637

The only real trouble with bug #587 is that our HDR environment maps really don't work at all without pmrem. Additionally, #565 can be solved by turning on PMREM for the generated enviroment map, and #601 can be solved by turning on PMREM for LDR environment maps. Given all of that, this PR removes the experimental-pmrem flag and instead enables PMREM in all cases, which has the nice side effect of simplifying both the API and the code.

The only downside is a performance hit. However, we plan to look into improving our rendering in the near future, and I believe we can attain what PMREM has been doing with a simpler and faster algorithm.

cdata

Looking good, just want to kick the tires on it a little

cdata · 2019-06-04T15:31:48Z

src/three-components/TextureUtils.ts

+      // Apply PMREM to HDR textures, which if loaded from file have
+      // RGBEEncoding, and if generated are HalfFloatType
+      if (environmentMap.encoding === RGBEEncoding ||
+          environmentMap.type === HalfFloatType) {


Woah, we weren't even applying PMREM to the generated map? That... doesn't seem right...

cdata · 2019-06-04T15:34:00Z

Marked as breaking because as of this change we turn on experimental-pmrem for everything.

@elalish what do you think about adding an escape hatch for folks who might need to migrate? I'm thinking something along the lines of a disable-pmrem flag...

elalish · 2019-06-04T16:29:44Z

Well, dusting off my PM hat, I'd ask if we know of any situations where this behavior is worse (less physically accurate) than the current behavior? Also, for the sake of our own testing and maintenance, I think it's important that we reduce the number of rendering paths we expose. Better to do that now so that we can get the bug reports and fix them before our stable release.

cdata · 2019-06-04T16:39:48Z

The proposal would be to give folks an off-ramp and then remove the flag entirely in a subsequent version. The risk of not doing it is that you lose those users entirely when they feel left behind/blind-sided without a transition path into the future.

elalish · 2019-06-06T20:03:44Z

I think there are too many serious rendering bugs that this fixes to warrant keeping the old path open. I'm also concerned about the dev burden it will put on us to try and keep up both paths as we improve the rendering in 0.5.

cdata · 2019-06-06T20:11:43Z

I think we should wait for v0.5.0 as a matter of timeliness. v0.4.0 has been lingering too long, and our partners need some incremental improvements to become unblocked. I'm also worried that without low-barrier migration path accompanying this change, we could inadvertently hurt the uptake of v0.4.0. v0.5.0 will give us the time to live with this change for a bit before shipping it to users.

Keep in mind that we can start landing v0.5.0 changes as soon as v0.4.0 goes out.

…into remove_pmrem_flag

cdata · 2019-06-19T04:13:28Z

Just had a thought as I filed #649 : if we fix this first, we'll cut the number of PMREM generation passes by about half in the tests. Maybe we should go ahead and fix this early in case it wins us some wiggle room in Safari.

cdata · 2019-06-21T17:03:37Z

Let's give this another shot now that #649 is fixed.

jsantell

I wonder if using spherical harmonics is a potential option for performance. Could provide a gist explaining how to generate it, achieving correct indirect illumination with no initial runtime overhead.

Are there any big installs of MV not using PMREM? While this would be a breaking change, it is at least an opinionated nudge in the direction of correct PBR. Although not everyone has HDR versions to their environments. When I was looking before, I couldn't find any info on 'simulating' HDR with an LDR image, but increasing brightness seems "OK"

jsantell · 2019-06-25T17:47:19Z

src/features/environment.ts

              this.stageLightIntensity, illuminationRole);
-          scene.model.setEnvironmentMapIntensity(environmentIntensity);
+          scene.model.setEnvironmentMapIntensity(
+              this.environmentIntensity * 0.65);


I understand this magic number was in here before this PR, but what model is it based on? Either way, maybe define as a constant

I've removed this in a follow-on PR.

elalish

Thanks, I just reviewed your caching change, and it looks a lot better. I just have one question about a potential race condition.

elalish · 2019-07-12T15:49:48Z

src/three-components/TextureUtils.ts

 const hdrLoader = new RGBELoader();

+const $environmentMapCache = Symbol('environmentMapCache');
+const $skyboxCache = Symbol('skyboxCache');


👍 It didn't occur to me that we could make the caches private members, but now I see that TextureUtils is owned by Renderer, which is global. Nice!

elalish · 2019-07-12T15:58:57Z

src/three-components/TextureUtils.ts

+      url: string,
+      progressTracker?: ProgressTracker): Promise<WebGLRenderTarget> {
+    if (!this[$environmentMapCache].has(url)) {
+      const skyboxLoads = this[$loadSkyboxFromUrl](url, progressTracker);


Considering this is all async now, doesn't this mean that when I have a skybox url, but not an environment url (and no cache yet) that it will load the skybox twice, because the skybox cache won't be set yet when this function goes looking for it?

The cache will be populated synchronously (relative to this line). The trick is that we populate it with a promise for the eventual value, and that promise can be looked up and awaitd in parallel by other routines trying to load from the same URL.

elalish · 2019-07-12T16:42:25Z

Ah, very cool. Thanks for the explanation!

…

On Fri, Jul 12, 2019 at 9:08 AM Christopher Joel ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/three-components/TextureUtils.ts <#591 (comment)> : > + this[$environmentMapCache].set(url, environmentMapLoads); + } + + return this[$environmentMapCache].get(url)!; + } + + /** + * Loads a skybox from a given URL, then PMREM is applied to the + * skybox texture and the resulting WebGLRenderTarget is returned, + * with the assumption that it will be used as an environment map. + */ + private[$loadEnvironmentMapFromSkyboxUrl]( + url: string, + progressTracker?: ProgressTracker): Promise<WebGLRenderTarget> { + if (!this[$environmentMapCache].has(url)) { + const skyboxLoads = this[$loadSkyboxFromUrl](url, progressTracker); The cache will be populated synchronously (relative to this line). The trick is that we populate it with a promise for the eventual value, and that promise can be looked up and awaitd in parallel by other routines trying to load from the same URL. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#591>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMS2LBRNAXPDVAH7BZFBLDP7CT6NANCNFSM4HR6I7JQ> .

elalish

LGTM; any idea why the tests aren't passing the CI anymore?

elalish · 2019-07-12T17:03:52Z

src/three-components/TextureUtils.ts

+
+      // We should only ever generate this map once, and we will not be using
+      // the environment map as a skybox, so go ahead and dispose of all
+      // interstitial artifacts:


Is it true that we don't use the generated environment map as a skybox? I'm not sure why that hadn't occurred to me before. Maybe I should update our fidelity tests accordingly.

I think that it's fine to let the fidelity tests have a skybox. This is the existing behavior when specifying a background-image, so it is expected. We can change this behavior if we want 🤷‍♂ but let's file an issue to discuss it?

elalish · 2019-07-12T17:06:10Z

src/three-components/TextureUtils.ts

+      if (skybox != null) {
+        skybox.dispose();
+      }
+    }


This for loop could be a method to skip the repetition.

elalish · 2019-07-12T17:07:30Z

test/wct-sauce.conf.json

  "clientOptions": {
    "mochaOptions": {
-      "timeout": 90000
+      "timeout": 180000


This actually didn't help as I recall, so we could probably revert it. Up to you.

It's arbitrarily high either way. No harm in keeping it this high.

* 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

cdata

Regarding khronos-MetalRoughSpheres-LDR: it's notable that Filament w/ LDR seems more illuminated than <model-viewer>. What can we do to bring these two cases closer together?

This has been an awesome thread of work to follow 🙌 great job. Just a few loose ends to tie up, I think.

cdata · 2019-07-18T04:04:34Z

index.html

              environment entirely. Defaults to 1.</p>
            </li>
-            <li>
-              <div>experimental-pmrem</div>


cdata · 2019-07-18T04:05:27Z

src/features/environment.ts

        @property({type: String, attribute: 'background-color'})
        backgroundColor: string = DEFAULT_BACKGROUND_COLOR;

-        @property({type: Boolean, attribute: 'experimental-pmrem'})


cdata · 2019-07-18T04:07:20Z

src/features/environment.ts

-
-          scene.configureStageLighting(
-              this.stageLightIntensity, illuminationRole);
-          scene.model.setEnvironmentMapIntensity(environmentIntensity);


cdata · 2019-07-18T04:12:04Z

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

+  // The renderer can retain state, so these tests have the possibility of
+  // getting different results in different orders. However, our use of the
+  // renderer *should* always return its state to what it was before to avoid
+  // this kind of problem (and many other headaches).


cdata · 2019-07-18T04:12:50Z

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

  suite('generating an environment map and skybox', () => {
-    let textures: {skybox: WebGLRenderTargetCube|null, environmentMap: Texture}|
+    let textures:
+        {environmentMap: WebGLRenderTarget, skybox: WebGLRenderTargetCube|null}|


Let's import the type we created for this and use it here.

cdata · 2019-07-18T04:20:02Z

src/three-components/TextureUtils.ts

 const hdrLoader = new RGBELoader();

+const $environmentMapCache = Symbol('environmentMapCache');
+const $skyboxCache = Symbol('skyboxCache');


cdata · 2019-07-18T04:24:59Z

src/three-components/TextureUtils.ts

+
+      // We should only ever generate this map once, and we will not be using
+      // the environment map as a skybox, so go ahead and dispose of all
+      // interstitial artifacts:


I think that it's fine to let the fidelity tests have a skybox. This is the existing behavior when specifying a background-image, so it is expected. We can change this behavior if we want 🤷‍♂ but let's file an issue to discuss it?

cdata · 2019-07-18T04:27:10Z

test/fidelity/config.json

+        "width": 1536,
+        "height": 1536
+      }
+    },


cdata · 2019-07-18T04:31:37Z

test/fidelity/khronos-MetalRoughSpheres-HDR/index.html

@@ -0,0 +1,38 @@
+<!--
+/*
+ * Copyright 2018 Google Inc. All Rights Reserved.


cdata · 2019-07-18T04:38:13Z

test/wct-sauce.conf.json

  "clientOptions": {
    "mochaOptions": {
-      "timeout": 90000
+      "timeout": 180000


It's arbitrarily high either way. No harm in keeping it this high.

cdata · 2019-07-18T04:47:03Z

This PR includes several threads of work by @elalish , the descriptions for which I am quoting here for posterity:

Cache environments and skyboxes

Really #336 was already fixed, but not in a complete way as only the generated environment was cached, but not any loaded environments, skyboxes, or PMREM results. Now all of the above are cached in a single system. The near-term goal of this PR is to get our automated tests to pass on Safari with PMREM, as we're guessing the current problem is they use too much memory from duplicate environments.

Reuse renderer in tests

Still working on getting our tests to pass. All those Safari warnings about too many WebGL contexts made me dig in a little. I found this thread pixijs/pixijs#2233 (comment) which says that Three's renderer.dispose() method only simulates a context being destroyed and there's no guarantee the browsers actually clean up at that point. Everyone seems to think the best answer to create a renderer (and therefore a context) once, and always reuse it. We were doing that everywhere except our tests, so I've updated them, and even on Chrome they run markedly faster. We still create and (try to) destroy a few in tests that don't involve an element. I'm not sure if it's worth it to try and make those share a renderer too.

Switch environment generation from HalfFloat to RGBE

Several major Samsung devices don't support halfFloat textures, causing our environment generation to fail. Switching to RGBE fixes this, but kills our current shader for blurring that environment, so I've created a new gaussianBlur filter for cubeMaps to replace it.

Add new fidelity tests

I added two new fidelity tests as copies of the Khronos-MetalRoughSpheres, one with an HDR environment and one with an LDR environment (both spruit_sunrise, which has very high dynamic range, so good for catching bugs). Combined with the original that uses lightroom (similar to our generated environment), we now cover all of our lighting cases with a single, canonical model.

Interestingly, this exposed a bug in filament, or possibly our usage thereof: Our Filament renders for the HDR and LDR environments are identical, and both appear to be LDR. This explains why we thought our renders looked "too yellow" when using spruit_sunrise_2k.hdr; in actuality Filament was not yellow enough, as it was lacking HDR.

Default stage light to zero

In addition to changing the default, I removed the hemisphere light as I believe its only purpose was to hide an old LDR environment lighting bug that has since been fixed. I also removed all the arbitrary constants I could find relating to lighting.

I also added a top-light to our environment generation to take the place of our default stage light, but we'll need to update our fidelity tests to properly compare.

cdata · 2019-07-18T15:03:07Z

src/three-components/TextureUtils.ts

-      }
+    var gammaOutput = this.renderer.gammaOutput;
+    var toneMapping = this.renderer.toneMapping;
+    var toneMappingExposure = this.renderer.toneMappingExposure;


In modern JS, there are few good reasons to use var ever. Use const or let instead (in this case probably const).

elalish · 2019-07-18T16:49:55Z

Regarding illumination, I did play with this a bit, and chose to match the lightroom renders instead, since that's our default. Honestly, it bothers me that we still need exposure = 0.8 just to get the skyboxes to match. We should be able to find an equation to match that exactly. Then we can continue with trying to more accurately match their model illumination, but given the Filament HDR bug I found, I don't want to work on that until it's fixed, because I'd guess their fix may affect LDR lighting as well.

cdata

Awesome work on this @elalish 🙌

Remove experimental-pmrem flag (google#591) * 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) * 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 * addressing feedback * Remove accidental render target disposal

Emmett Lalish added 2 commits May 31, 2019 14:18

removed pmrem attribute

56384e6

removed pmrem attribute from non-code files

ea75251

elalish requested a review from cdata May 31, 2019 22:03

elalish self-assigned this May 31, 2019

default environment map now uses PMREM

8a4d719

elalish requested a review from jsantell June 3, 2019 20:23

cdata added the flag: breaking change label Jun 4, 2019

cdata added this to the v0.4.0 milestone Jun 4, 2019

cdata reviewed Jun 4, 2019

View reviewed changes

cdata modified the milestones: v0.4.0, v0.6.0, v0.5.0 Jun 6, 2019

elalish added 2 commits June 6, 2019 12:12

fixed tests

f508eb4

using PMREM for all cases, fixing #565

b9a7ef8

elalish requested a review from cdata June 6, 2019 20:03

Emmett Lalish added 2 commits June 7, 2019 11:16

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

Emmett Lalish added 2 commits June 21, 2019 10:16

merging master

0e977d3

increasing sauce timeout to 3min

fdf4cdd

jsantell reviewed Jun 25, 2019

View reviewed changes

Emmett Lalish added 2 commits July 12, 2019 08:27

target caching works

cb5c8f9

removed extraneous cache, cleaned up generation

44dd9f8

elalish and others added 5 commits July 12, 2019 08:27

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

elalish commented Jul 12, 2019

View reviewed changes

cdata suggested changes Jul 18, 2019

View reviewed changes

cdata reviewed Jul 18, 2019

View reviewed changes

addressing feedback

e67ddc4

elalish requested a review from cdata July 18, 2019 16:58

Remove accidental render target disposal

42afbcc

cdata approved these changes Jul 22, 2019

View reviewed changes

elalish merged commit 1265621 into master Jul 22, 2019

pushmatrix mentioned this pull request Jul 23, 2019

Rendering inconsistencies with fine detailed normals + roughness #587

Closed

6 tasks

This was referenced Jul 23, 2019

Make PMREM the default when using an HDR environment #346

Closed

Use consistent, cross-device compatible single rendering path for IBL #647

Closed

HDR Environment not lighting models on Samsung Galaxy S8 or S9 #637

Closed

elalish deleted the remove_pmrem_flag branch September 27, 2019 16:45

snyk-bot mentioned this pull request Aug 16, 2021

[Snyk] Security upgrade http-server from 0.12.3 to 0.13.0 Zintexiss/model-viewer#17

Open

Conversation

elalish commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdata left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdata commented Jun 4, 2019

Uh oh!

elalish commented Jun 4, 2019

Uh oh!

cdata commented Jun 4, 2019

Uh oh!

elalish commented Jun 6, 2019

Uh oh!

cdata commented Jun 6, 2019

Uh oh!

cdata commented Jun 19, 2019

Uh oh!

cdata commented Jun 21, 2019

Uh oh!

jsantell 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!

elalish 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!

elalish commented Jul 12, 2019 via email

Uh oh!

elalish 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!

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 Jul 18, 2019

elalish commented May 31, 2019 •

edited

Loading