Fix PMREM anisotropic filtering #23556
Conversation
| @@ -409,12 +409,6 @@ class PMREMGenerator { | |||
|
|
|||
| uniforms[ 'envMap' ].value = texture; | |||
|
|
|||
| if ( ! isCubeTexture ) { | |||
|
|
|||
| uniforms[ 'texelSize' ].value.set( 1.0 / texture.image.width, 1.0 / texture.image.height ); | |||
There was a problem hiding this comment.
I realized my previous simplification of PMREM had made this vestigial, so I removed it as well.
|
|
||
| vec3 tm = mix( tl, tr, f.x ); | ||
| vec3 bm = mix( bl, br, f.x ); | ||
| gl_FragColor.rgb = mix( tm, bm, f.y ); |
There was a problem hiding this comment.
More vestigial code.
| @@ -700,7 +701,7 @@ function WebGLProgram( renderer, cacheKey, parameters, bindingStates ) { | |||
| vertexShader = unrollLoops( vertexShader ); | |||
| fragmentShader = unrollLoops( fragmentShader ); | |||
|
|
|||
| if ( parameters.isWebGL2 && parameters.isRawShaderMaterial !== true ) { | |||
There was a problem hiding this comment.
This is probably the most controversial part of my change. Feedback welcome!
There was a problem hiding this comment.
I would be good to find a solution without changing this line since it affects user code.
Just to be clear, you have removed parameters.isRawShaderMaterial to make the extension work, right?
There was a problem hiding this comment.
Would it be possible to use ShaderMaterial in PMREMGenerator instead?
There was a problem hiding this comment.
Yes, that's right. I also considered using ShaderMaterial instead; it would be a bigger change and I can't currently remember why I chose raw in the first place, so I'm afraid I might hit a blocker. Still, doesn't it seem odd that we insert the customExtensions into the RawShaderMaterial, but then don't fix them up for WebGL2? This is why it felt like a bug to me.
There was a problem hiding this comment.
Aren't these extensions automatically enabled with WebGL 2? So you need the injection only with WebGL 1.
I mean all materials are affected by this, not only RawShaderMaterial.
There was a problem hiding this comment.
well, I only removed the check for RawShaderMaterial, so this is only changing the behavior for RawShaderMaterials. the code this adds translates from the old function names like texture2DGradEXT to the WebGL2 version texture. That's very convenient; is there a better way?
There was a problem hiding this comment.
Hmm... I'll merge and do another PR reverting some of these things so we can see how it looks.
There was a problem hiding this comment.
IMO, it's best when PMREMGenerator uses ShaderMaterial. All changes to the renderer can then be reverted.
The idea of excluding RawShaderMaterial from the GLSL 3.0 conversion process was to give the user full control over the GLSL. Meaning devs write GLSL 3 OR GLSL 1 code. But the material never supports both at the same time. I don't think we need to change this policy for fixing PMREMGenerator.
There was a problem hiding this comment.
Gotcha. I didn't realize a RawShaderMaterial couldn't be used to support both WebGL1 and WebGL2. I guess ShaderMaterial is the way to go then. Might be good to add that to the docs somewhere.
| @@ -104,7 +104,15 @@ export default /* glsl */` | |||
| uv.x *= CUBEUV_TEXEL_WIDTH; | |||
| uv.y *= CUBEUV_TEXEL_HEIGHT; | |||
|
|
|||
| return texture2D( envMap, uv ).rgb; | |||
| #ifdef TEXTURE_LOD_EXT | |||
There was a problem hiding this comment.
Would doing this instead work?
#ifdef texture2DGradEXTThere was a problem hiding this comment.
I think it's equivalent, yes, but this was how it was done elsewhere in the code, specifically here: https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderChunk/transmission_pars_fragment.glsl.js#L60
| @@ -672,6 +666,10 @@ function _getBlurShader( lodMax, width, height ) { | |||
|
|
|||
| name: 'SphericalGaussianBlur', | |||
|
|
|||
| extensions: { | |||
There was a problem hiding this comment.
Adding the extensions object like that is actually not supported since all other flags of extensions become undefined. The idea is to use:
shaderMaterial.extensions.shaderTextureLOD = true;There was a problem hiding this comment.
ah, good call, thanks!
|
Thanks! |
Related issue: #23517 #17855 #23468 #23511
Description
Okay, so the problem with the envMap seams showing up on certain Nvidia GPUs turns out to be because they force anisotropic filtering to be on, but that is not compatible with our unusual texture lookups for the PMREM. The NEAREST option changes the interpolation in a not-great way, so I've disabled it instead by using
textureGradand setting the pixel derivatives to zero. This appears to completely fix the problem, but I had to make some other changes that should probably get a bit more scrutiny:Even though
RawShaderMaterialinherits fromShaderMaterial, theextensionsobject seems to be broken: I needed to specify thetexture_lodextension, but it only got part of the glsl; not enough to actually enable it. I'm assuming that's a bug, since it's not documented behavior and it was already getting part of the needed glsl. However, I probably haven't fixed it all and it could also use a little code cleanup if this is acceptable - @Mugen87 @mrdoob.To reproduce the problem on a non-Nvidia GPU, simply add
anisotropy: 16to line 258 ofPMREMGenerator.js.I believe #23517 should be merged as well as this change to complete the fix.