-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WebGLRenderer: Integrate PMREM. #22178
Conversation
Your auto-generated PMREM implementation is working for me. Thanks for the PR! Personally, I would focus only on auto-generated PMREM environment maps for now and save the background blurriness for another PR. But I would not let that block this PR. PMREM is too blocky as a blurry background, and not even close to linear in your "alpha" parameter when used that way. @elalish may have some suggestions for improvement -- and recommendations on how to handle the "roughness-mipmapper" feature. |
The biggest improvement to make is supporting variable-size PMREMs, which I have been meaning to get around to for ages. That could also help with the blockiness problem. The RoughnessMipmapper is pretty independent of this PR, but agree that building the functionality into the renderer would be great. It helps a lot with aliasing and general PBR correctness when zooming out. |
I'm not really sure how to even define linearity for blurriness, especially on a 0-1 scale. If you do allow variable-size PMREMs eventually, you'll find that to get 1-mip down from full, you'll need increasingly tiny values or roughness (blurriness). But on the other hand, at a given value you'll always get the same output regardless of input resolution. So it depends how you want it to behave. |
Hmm... I have not seen a need for that. I suppose you are aware of a need? // It is personal taste, obviously, but a non-blocky, "slightly out-of-focus" blurry would be welcome -- like a depth-of-field pass. As it is currently implemented, I think PMREM should only be used for what it was designed for. |
I was thinking it could help with performance to reduce the PMREM texture size for models that don't have low roughness. On the other side, higher res would make for better backgrounds. Obviously it hasn't been a huge priority... |
That is what I was guessing: you were referring to reducing the size and the levels. Has there been a performance problem? If so, it should be noted. |
Well, we always want to increase performance, right? What I have not measured yet is what gain might be available from a change like this. If you have any performance numbers that might shed some light, they would be much appreciated! |
Then we need to find a way to solve the issue I have mentioned in #20463 (comment). |
This is an interesting comment. Is this the current problem? Because as long as you use the cubeuv shader fragment, you should still be able to get interpolated values from the PMREM. Maybe we just need to add that to Background? |
@Mugen87 I may be missing something... I am not sure why PMREM is used for backgrounds in the first place. It should't be in my opinion. If the HDR is blocky, then you need to avoid |
I've seen many users assigning the result of If the renderer starts to provide an auto-conversion, |
Well, that is not correct. PMREM is an IBL, not a background image. |
Just to be clear I'm referring on code like: three.js/examples/webgl_tonemapping.html Lines 103 to 106 in 3a9152a
|
Not true! The way CubeUV works for PMREM is that it does in fact use nearest filtering, but the |
I wasn't referring to PMREM. |
So the be clear the example code should actually look like so, right? https://jsfiddle.net/pg5rhctx/2/ |
Yes, IMO, the rendering should look like the rendering in your fiddle. The app in your fiddle should not have to handle PMREM. scene.background = texture; // the loaded cubeMap or equirectangular texture
scene.environment = texture; // environment texture will be auto-converted to PMREM format by the renderer
new GLTFLoader()
.setPath( '...' )
.load( 'DamagedHelmet.gltf', function ( gltf ) {
scene.add( gltf.scene ); // the app does not have to do anything else (yay!)
render();
} );
} ); No more PMREM-related code at the app level. |
Yes, I just wanted to make sure we clarify how |
I've updated most examples like suggested in #22178 (comment). The ones which are using For a different PR, we might want to consider to allow |
@mrdoob This looks good to me. |
Thanks! |
Interesting idea... The serialisation code would have to be redesigned a bit in order to support that though... |
On LEFT: const pmremGenerator = new THREE.PMREMGenerator( renderer );
const hdrCubeRenderTarget = pmremGenerator.fromEquirectangular( equirectHDR );
material.envMap = hdrCubeRenderTarget.texture; // PMREM texture On RIGHT: material.envMap = equirectHDR; // loaded texture For users who were not using PMREM with |
Good catch, if someone was was just only using I guess they'll have to switch to |
@mrdoob I do not understand your comment, TBH.
|
Indeed. Switching to
Well... I guess we'll have to wait and see if anyone complains. |
@mrdoob Maybe you are misunderstanding what has happened... (?) There are no complaints. Assigning an ordinary cube map as Now, users should not use PMREM at the app level at all. And they do not need to. We can remove PMREM from the examples. Their scenes will be brighter than before. Tell them to remove the unneeded ambient lighting. |
I was going to quote google/model-viewer#912 but apparently after almost a year someone had fixed it |
Ah, I misunderstood indeed. Yeah... I didn't think of that scenario. |
Related issue: see #20463
Description
Second attempt of integration PMREM.