Skip to content
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.

Sign up for GitHub

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

Fix PMREM anisotropic filtering #23556

Merged
merged 1 commit into from
Feb 22, 2022
+19 −29
Merged

Fix PMREM anisotropic filtering #23556

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 7 additions & 26 deletions src/extras/PMREMGenerator.js
View file
Original file line number Diff line number Diff line change
@@ -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 );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized my previous simplification of PMREM had made this vestigial, so I removed it as well.


}

const size = this._cubeSize;
_setViewport( cubeUVRenderTarget, 0, 0, 3 * size, 2 * size );

@@ -672,6 +666,10 @@ function _getBlurShader( lodMax, width, height ) {

name: 'SphericalGaussianBlur',

extensions: {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, good call, thanks!

shaderTextureLOD: true
},

defines: {
'n': MAX_SAMPLES,
'CUBEUV_TEXEL_WIDTH': 1.0 / width,
@@ -765,14 +763,12 @@ function _getBlurShader( lodMax, width, height ) {

function _getEquirectShader() {

const texelSize = new Vector2( 1, 1 );
const shaderMaterial = new RawShaderMaterial( {

name: 'EquirectangularToCubeUV',

uniforms: {
'envMap': { value: null },
'texelSize': { value: texelSize }
'envMap': { value: null }
},

vertexShader: _getCommonVertexShader(),
@@ -785,31 +781,16 @@ function _getEquirectShader() {
varying vec3 vOutputDirection;

uniform sampler2D envMap;
uniform vec2 texelSize;

#include <common>

void main() {

gl_FragColor = vec4( 0.0, 0.0, 0.0, 1.0 );

vec3 outputDirection = normalize( vOutputDirection );
vec2 uv = equirectUv( outputDirection );

vec2 f = fract( uv / texelSize - 0.5 );
uv -= f * texelSize;
vec3 tl = texture2D ( envMap, uv ).rgb;
uv.x += texelSize.x;
vec3 tr = texture2D ( envMap, uv ).rgb;
uv.y += texelSize.y;
vec3 br = texture2D ( envMap, uv ).rgb;
uv.x -= texelSize.x;
vec3 bl = texture2D ( envMap, uv ).rgb;

vec3 tm = mix( tl, tr, f.x );
vec3 bm = mix( bl, br, f.x );
gl_FragColor.rgb = mix( tm, bm, f.y );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More vestigial code.


gl_FragColor = vec4( texture2D ( envMap, uv ).rgb, 1.0 );

}
`,

10 changes: 9 additions & 1 deletion src/renderers/shaders/ShaderChunk/cube_uv_reflection_fragment.glsl.js
View file
Original file line number Diff line number Diff line change
@@ -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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would doing this instead work?

#ifdef texture2DGradEXT

Copy link
Contributor Author

@elalish elalish Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see...


return texture2DGradEXT( envMap, uv, vec2( 0.0 ), vec2( 0.0 ) ).rgb; // disable anisotropic filtering

#else

return texture2D( envMap, uv ).rgb;

#endif

}

5 changes: 3 additions & 2 deletions src/renderers/webgl/WebGLProgram.js
View file
Original file line number Diff line number Diff line change
@@ -423,7 +423,8 @@ function WebGLProgram( renderer, cacheKey, parameters, bindingStates ) {
prefixFragment = [

customExtensions,
customDefines
customDefines,
parameters.rendererExtensionShaderTextureLod ? '#define TEXTURE_LOD_EXT' : ''

].filter( filterEmptyLine ).join( '\n' );

@@ -700,7 +701,7 @@ function WebGLProgram( renderer, cacheKey, parameters, bindingStates ) {
vertexShader = unrollLoops( vertexShader );
fragmentShader = unrollLoops( fragmentShader );

if ( parameters.isWebGL2 && parameters.isRawShaderMaterial !== true ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably the most controversial part of my change. Feedback welcome!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to use ShaderMaterial in PMREMGenerator instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@Mugen87 Mugen87 Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I'll merge and do another PR reverting some of these things so we can see how it looks.

Copy link
Collaborator

@Mugen87 Mugen87 Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ( parameters.isWebGL2 ) {

// GLSL 3.0 conversion for built-in materials and ShaderMaterial