Skip to content

(WIP) PMREM+HDR+r99#237

Closed
jsantell wants to merge 1 commit intomasterfrom
hdr2
Closed

(WIP) PMREM+HDR+r99#237
jsantell wants to merge 1 commit intomasterfrom
hdr2

Conversation

@jsantell
Copy link
Contributor

@jsantell jsantell commented Nov 27, 2018

Fixes #65, #196, #215, enables #200
Supercedes #133

I had PMREM+HDR working in #133 again from master, but the envmap was off with the skybox again, so figured lets get this working on three r99dev that added scene.background support for cubemaps (mrdoob/three.js#15187, mrdoob/three.js#15186, mrdoob/three.js#15081) so that we can have a good baseline for judging reflectivity correctness rather than keep flipping the skysphere.

I have PMREM+HDR with scene.background handling the sky rather than a skysphere mostly working, but there were some issues with cubemap encoding changes when used in scene.background (mrdoob/three.js#15325), and the flipping of the reflection found in WebGLRenderTargetCubes (mrdoob/three.js#15301) mentioned earlier. PR out for encoding changes (mrdoob/three.js#15299), and I have a hacky way of fixing the reflection, but my in progress PR is out (mrdoob/three.js#15302).

At this point, can the HDR+PMREM work be decoupled from the scene.background change? We could land just the HDR+PMREM changes, and flip the skysphere again, but it'll be inverted. We can't just land the scene.background change and remove the skysphere because we can't reuse the same cubemap from the envmap in scene.background (or else we get INVALID_OPERATION: bindTexture: textures can not be used with multiple targets), and the rational why we have an equirect for the skysphere (although we could deep clone the texture?). Adding PMREMs means we can use cubemaps for both the skysphere and envmap since we have 2 cubemaps per environment now, and can discard the equirects.

There are still a few more caveats for the PMREM feature. The memory leak issue worked around in #227 is also in three's PMREM generators -- we would either have to do the same messy workaround, or can try to fix it upstream in mrdoob/three.js#15288. The leak FWIW would only occur when swapping out background images continuously for an hour, so not a cause for alarm (edit: using HDR images exacerbate the leak; I don't think the texture is being duplicated, but maybe more resources are created). iPhone 6 also has an issue with the PMREM implementation (mrdoob/three.js#15005).

Not quite ready for review yet, still some tests failing, and textures sometimes bleed into other elements (more alarming). These changes of course may not always look great out of the box but will enable the ability to add configuration. If missing my three.js patches, the first environment map encoding used is applied globally to all skyboxes, and the environment map projected on the model will be inversed. Almost there, wanted to document progress so far here.

Some pictures! (More pictures in #133)

LDR radiance vs HDR radiance: (maybe the LDR image should be boosted/mapped or PMREM should not be used for LDR)

screenshot from 2018-11-20 17-38-14

Inverted env map

screenshot from 2018-11-21 15-42-23

PMREM at different sampling levels (top/left clockwise 8, 16, 24, 32), lower samples more noticeable on smooth meshes.

pmrem-samples

@jsantell jsantell changed the title PMREM+HDR (WIP) PMREM+HDR Nov 27, 2018
@jsantell jsantell changed the title (WIP) PMREM+HDR (WIP) PMREM+HDR+r99 Nov 27, 2018
@jsantell
Copy link
Contributor Author

My PRs for WebGLBackground texture encoding (mrdoob/three.js#15299), inverted reflection PMREMs for WebGLRenderTargetCubes (mrdoob/three.js#15302), and the PMREM/EquirectangularToCubeGenerator memory leak (mrdoob/three.js#15330) have been merged upstream which will make this PR much easier, as well as remove some of the work arounds in place for the memory leak 🎉

@jsantell
Copy link
Contributor Author

jsantell commented Feb 6, 2019

Superceded by #323 and #338

@jsantell jsantell closed this Feb 6, 2019
@jsantell jsantell deleted the hdr2 branch February 6, 2019 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support HDR environment maps, and HDR-ify default environment map

1 participant