Blur skybox by elalish · Pull Request #695 · google/model-viewer

elalish · 2019-08-06T18:29:40Z

Fixes #181
Fixes #528

This applies our cubemap Gaussian blur function to the skybox. In the process, it also converts an RGBE skybox to RGB so that it can be mipmapped and interpolated like an LDR skybox. I'm not 100% happy with the Gaussian blur function yet, as there is still a bit of aliasing, but I have an idea to improve it that we can look into as a follow-on.

…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

This reverts commit f743b26.

cdata

Looks good 👍 but some minor cleanup requested

cdata · 2019-08-07T21:18:23Z

examples/assets/attributions.md

 licensed under <a href="https://hdrihaven.com/p/license.php">CC0</a>
 (<a href="https://hdrihaven.com/hdri/?h=aircraft_workshop_01">source</a>)

-## music_hall_01_4k.hdr


cdata · 2019-08-07T21:19:07Z

src/three-components/TextureUtils.ts

    Symbol('loadEnvironmentMapFromSkyboxUrl');
 const $loadGeneratedEnvironmentMap = Symbol('loadGeneratedEnvironmentMap');

-export interface TextureUtilsConfig {


cdata · 2019-08-07T21:21:17Z

src/three-components/TextureUtils.ts

 const HDR_FILE_RE = /\.hdr$/;
 const ldrLoader = new TextureLoader();
 const hdrLoader = new RGBELoader();
+const cubemapSize = 256;


Let's capitalize this e.g, CUBEMAP_SIZE (and yeah, ldrLoader / hdrLoader break our convention here a little; they should probably be static class members or something but we can fix that later).

cdata · 2019-08-07T21:23:08Z

src/three-components/TextureUtils.ts

          await Promise.all([environmentMapLoads, skyboxLoads]);

+      if (skybox != null) {
+        skybox = this.gaussianBlur(skybox, 0.004, GammaEncoding);


Doesn't this return a reference to a new, distinct render target? If so, who manages the lifecycle of this instance? When does it get disposed?

Sometimes it is the same renderTarget (if the input is jpg); if it is hdr then gaussianBlur disposes of the one you gave it and returns this new one in its place.

Doesn't that mean we're possibly disposing of the cached render target?

Let's fix this in a following PR to extend our usage of your cachingPolicy.

To be clear, we're no longer disposing of the cached render target, but we are leaking backgrounds.

cdata · 2019-08-07T21:29:10Z

src/three-components/TextureUtils.ts

      cubeTarget: WebGLRenderTargetCube,
-      standardDeviationRadians: number = 0.04) {
+      standardDeviationRadians: number = 0.04,
+      outputEncoding: TextureEncoding|null = null): WebGLRenderTargetCube {


If you just want this to be an optional third argument, you can express it as outputEncoding?: TextureEncoding. The "loose" equality conditionals are useful for this particular scenario only: checking if a value is undefined or null. So, the check further down the page will work as written.

cdata · 2019-08-07T21:31:08Z

src/three-components/TextureUtils.ts

-    this.renderer.toneMapping = toneMapping;
-    this.renderer.toneMappingExposure = toneMappingExposure;
-    this.renderer.gammaOutput = gammaOutput;
+    if (outputEncoding != null && outputEncoding === GammaEncoding &&


Strictly speaking, you probably don't need the first clause of this condition because outputEncoding === GammaEncoding will always be false for every non-number value.

cdata · 2019-08-07T21:32:19Z

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

-vec4 inputTexelToLinear(vec4 value, int encoding){
-    if(encoding == 0){
+uniform int inputEncoding;
+uniform int outputEncoding;


cdata · 2019-08-07T21:35:11Z

src/three-components/TextureUtils.ts

+    if (outputEncoding != null && outputEncoding === GammaEncoding &&
+        cubeTexture.encoding !== GammaEncoding) {
+      cubeTarget.dispose();
+      const outParams = {


Let's call this blurTargetOptions or something like that

cdata

Looks great, let's ship it and fix the leaking backgrounds in a future change 👍

Also, we can do configurable blur as a follow-on feature e.g., #704

* removed pmrem attribute * removed pmrem attribute from non-code files * default environment map now uses PMREM * fixed tests * using PMREM for all cases, fixing #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 (#668) * Refactor envmap / skybox generation * default stage light to zero, removed hemisphere light (#674) * added new PMREMGenerator * modified MeshStandardShader to show it can work * Switch environment generation from HalfFloat to RGBE (#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 * blurring skybox * HDR skyboxes are now interpolated * reduced skybox resolution to match input * minor cleanup * fixed skybox metadata * reduced cubemapSize, cleanup * updated all environments to 1k * re-enable Android tests * Revert "re-enable Android tests" This reverts commit f743b26. * addressing feedback * Gaussian blur returns a new target

mrdoob · 2019-08-13T23:41:24Z

Love the blurred skyboxes! 👌

I tried the feature in http://model-viewer-tester.glitch.me/ and I'm not sure the exposure works correctly for the background now.

Is there something I should change in my test?

cdata · 2019-08-13T23:56:04Z

I played around with it and couldn't tell if anything was wrong. Do you have a screenshot of what you expected it to look like at that exposure level?

…

On Tue, Aug 13, 2019 at 4:41 PM Mr.doob ***@***.***> wrote: Love the blurred skyboxes! 👌 I tried the feature in http://model-viewer-tester.glitch.me/ and I'm not sure the exposure works correctly for the background now. [image: Screen Shot 2019-08-13 at 4 35 40 PM] <https://user-images.githubusercontent.com/97088/62984723-2195b600-bde9-11e9-851b-afcd288b6bff.png> Is there something I should change in my test? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#695>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAB2TUY74V36YSW5NWAI3OLQENBCPANCNFSM4IJZCYAA> .

mrdoob · 2019-08-14T00:05:49Z

Yes, sorry, I did not know how to use a previous version of the element, just figured it out.

This is how it's supposed to look like:

Basically, it looks like the blurred background loses the HDR information.

cdata · 2019-08-14T00:31:24Z

👍 great catch @mrdoob I filed #707 to investigate

elalish · 2019-08-14T00:41:09Z

Ah yes, good catch. This is because of the way I made the skybox interpolate, by converting it to RGB. Works great except when the exposure changes, which I neglected to test. I guess I should tweak the shader instead to interpolate RGBE.

…

On Tue, Aug 13, 2019 at 5:31 PM Christopher Joel ***@***.***> wrote: 👍 great catch @mrdoob <https://github.com/mrdoob> I filed #707 <#707> to investigate — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#695>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMS2LDXGG3GL4CRPVSHY2DQENG53ANCNFSM4IJZCYAA> .

Blur skybox (google#695) * 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 * blurring skybox * HDR skyboxes are now interpolated * reduced skybox resolution to match input * minor cleanup * fixed skybox metadata * reduced cubemapSize, cleanup * updated all environments to 1k * re-enable Android tests * Revert "re-enable Android tests" This reverts commit f743b26. * addressing feedback * Gaussian blur returns a new target

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 August 2, 2019 16:10

HDR skyboxes are now interpolated

383a898

reduced skybox resolution to match input

0c4fc1d

merging master

43cf402

minor cleanup

ffa509b

elalish requested a review from cdata August 6, 2019 18:29

elalish self-assigned this Aug 6, 2019

elalish added 5 commits August 6, 2019 12:44

fixed skybox metadata

56ba2d8

reduced cubemapSize, cleanup

a35b37b

updated all environments to 1k

efea630

re-enable Android tests

f743b26

Revert "re-enable Android tests"

3c2d70c

This reverts commit f743b26.

cdata suggested changes Aug 7, 2019

View reviewed changes

elalish added 4 commits August 7, 2019 15:02

merging master

4c1e8c2

addressing feedback

d0966cd

Gaussian blur returns a new target

42079a2

merging master

67b61ba

elalish requested a review from cdata August 13, 2019 16:42

cdata approved these changes Aug 13, 2019

View reviewed changes

elalish merged commit 6eb788f into master Aug 13, 2019

elalish deleted the blurSkyboxInterp branch August 13, 2019 18:17

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

elalish mentioned this pull request Aug 13, 2019

Pixelated PMREM textures #379

Closed

cdata mentioned this pull request Aug 14, 2019

HDR environment maps lose HDR when used as skybox texture #707

Closed

Conversation

elalish commented Aug 6, 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 left a comment

Choose a reason for hiding this comment

Uh oh!

mrdoob commented Aug 13, 2019

Uh oh!

cdata commented Aug 13, 2019 via email

Uh oh!

mrdoob commented Aug 14, 2019

Uh oh!

cdata commented Aug 14, 2019

Uh oh!

elalish commented Aug 14, 2019 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants