Skip to content

Replace 8 with real maxMipLevel in lights_fragment_maps.glsl #13501

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

Merged
mrdoob merged 4 commits into from
Mar 7, 2018
Merged

Replace 8 with real maxMipLevel in lights_fragment_maps.glsl #13501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f860006
Replace 8 with real maxMipLevel in lights_fragment_maps.glsl
takahirox Mar 5, 2018
ddc3538
Fix typo
takahirox Mar 5, 2018
07256a3
Replace another 8 with maxMipLevel in lights_fragment_maps.glsl
takahirox Mar 5, 2018
33ecb12
Hold maxMipMapLevel in textureProperties
takahirox Mar 6, 2018
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
2 changes: 2 additions & 0 deletions src/renderers/WebGLRenderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1975,6 +1975,8 @@ function WebGLRenderer( parameters ) {
uniforms.reflectivity.value = material.reflectivity;
uniforms.refractionRatio.value = material.refractionRatio;

uniforms.maxMipLevel.value = properties.get( material.envMap ).__maxMipLevel;

}

if ( material.lightMap ) {
Expand Down
1 change: 1 addition & 0 deletions src/renderers/shaders/ShaderChunk/envmap_pars_fragment.glsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
uniform sampler2D envMap;
#endif
uniform float flipEnvMap;
uniform int maxMipLevel;

#if defined( USE_BUMPMAP ) || defined( USE_NORMALMAP ) || defined( PHONG ) || defined( PHYSICAL )
uniform float refractionRatio;
Expand Down
8 changes: 3 additions & 5 deletions src/renderers/shaders/ShaderChunk/lights_fragment_maps.glsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,18 @@

#if defined( USE_ENVMAP ) && defined( PHYSICAL ) && defined( ENVMAP_TYPE_CUBE_UV )

// TODO, replace 8 with the real maxMIPLevel
irradiance += getLightProbeIndirectIrradiance( /*lightProbe,*/ geometry, 8 );
irradiance += getLightProbeIndirectIrradiance( /*lightProbe,*/ geometry, maxMipLevel );

#endif

#endif

#if defined( USE_ENVMAP ) && defined( RE_IndirectSpecular )

// TODO, replace 8 with the real maxMIPLevel
Copy link

@emackey emackey Mar 5, 2018

Choose a reason for hiding this comment

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

Just curious, this file appears to contain two copies of this TODO comment, but only one got removed. Was that intentional?

Copy link
Collaborator Author

@takahirox takahirox Mar 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Oops, I've just missed another one. I'll update.

Copy link
Collaborator Author

@takahirox takahirox Mar 5, 2018

Choose a reason for hiding this comment

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

Updated, thanks!

radiance += getLightProbeIndirectRadiance( /*specularLightProbe,*/ geometry, Material_BlinnShininessExponent( material ), 8 );
radiance += getLightProbeIndirectRadiance( /*specularLightProbe,*/ geometry, Material_BlinnShininessExponent( material ), maxMipLevel );

#ifndef STANDARD
clearCoatRadiance += getLightProbeIndirectRadiance( /*specularLightProbe,*/ geometry, Material_ClearCoat_BlinnShininessExponent( material ), 8 );
clearCoatRadiance += getLightProbeIndirectRadiance( /*specularLightProbe,*/ geometry, Material_ClearCoat_BlinnShininessExponent( material ), maxMipLevel );
#endif

#endif
3 changes: 2 additions & 1 deletion src/renderers/shaders/UniformsLib.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ var UniformsLib = {
envMap: { value: null },
flipEnvMap: { value: - 1 },
reflectivity: { value: 1.0 },
refractionRatio: { value: 0.98 }
refractionRatio: { value: 0.98 },
maxMipLevel: { value: 0 }

},

Expand Down
50 changes: 45 additions & 5 deletions src/renderers/webgl/WebGLTextures.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,16 @@ function WebGLTextures( _gl, extensions, state, properties, capabilities, utils,

}

function generateMipmap( texture, target ) {

_gl.generateMipmap( target );

var image = Array.isArray( texture.image ) ? texture.image[ 0 ] : texture.image;
Copy link
Collaborator Author

@takahirox takahirox Mar 6, 2018

Choose a reason for hiding this comment

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

Can I expect images for cubic map have the same size?

Copy link
Owner

@mrdoob mrdoob Mar 6, 2018

Choose a reason for hiding this comment

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

It's not guaranteed, but lets assume that.

Copy link
Collaborator Author

@takahirox takahirox Mar 7, 2018

Choose a reason for hiding this comment

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

OK. I wanna add comment about our assumption here later.

var textureProperties = properties.get( texture );
textureProperties.__maxMipLevel = Math.max( Math.log2( Math.max( image.width, image.height ) ), textureProperties.__maxMipLevel );
Copy link
Collaborator Author

@takahirox takahirox Mar 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Will gl.generateMipmap() clear the existing mipmap configuration?
For example, imagine you have 64x64 texture image and set mip level up to 10 with gl.texImage2D and then call gl.generateMipmap. gl.generateMipmap sets mip levels 1-6. So 7-10 levels will be removed?

I suppose it doesn't, so using max to choose the bigger one here.

Copy link
Owner

@mrdoob mrdoob Mar 6, 2018

Choose a reason for hiding this comment

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

@kenrussell do you know the answer for this?

Copy link

@kenrussell kenrussell Mar 7, 2018

Choose a reason for hiding this comment

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

Please see section 3.8.10.5 "Manual Mipmap Generation" of the OpenGL ES 3.0.5 spec.

It says that levels levelbase+1 through q are derived from the contents of levelbase. Other mip levels are untouched.

I'm not sure what the intent of this code is, but probably you need to zero out all levels of the texture that were previously set, and set the max mip level to the one that will be set by GenerateMipmap.

Copy link
Owner

@mrdoob mrdoob Mar 7, 2018

Choose a reason for hiding this comment

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

@kenrussell thanks!

Copy link
Collaborator Author

@takahirox takahirox Mar 7, 2018

Choose a reason for hiding this comment

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

I think I had just misunderstood a bit.
In the example I mentioned, 7-10 levels makes no sense (seems like we can set 1x1 image tho)
I can remove max.


}

// Fallback filters for non-power-of-2 textures

function filterFallback( f ) {
Expand Down Expand Up @@ -325,9 +335,19 @@ function WebGLTextures( _gl, extensions, state, properties, capabilities, utils,

}

if ( ! isCompressed ) {

textureProperties.__maxMipLevel = 0;

} else {

textureProperties.__maxMipLevel = mipmaps.length - 1;

}

if ( textureNeedsGenerateMipmaps( texture, isPowerOfTwoImage ) ) {

_gl.generateMipmap( _gl.TEXTURE_CUBE_MAP );
generateMipmap( texture, _gl.TEXTURE_CUBE_MAP );

}

Expand Down Expand Up @@ -514,10 +534,12 @@ function WebGLTextures( _gl, extensions, state, properties, capabilities, utils,
}

texture.generateMipmaps = false;
textureProperties.__maxMipLevel = mipmaps.length - 1;

} else {

state.texImage2D( _gl.TEXTURE_2D, 0, glFormat, image.width, image.height, 0, glFormat, glType, image.data );
textureProperties.__maxMipLevel = 0;

}

Expand Down Expand Up @@ -547,6 +569,8 @@ function WebGLTextures( _gl, extensions, state, properties, capabilities, utils,

}

textureProperties.__maxMipLevel = mipmaps.length - 1;

} else {

// regular Texture (image, video, canvas)
Expand All @@ -565,16 +589,22 @@ function WebGLTextures( _gl, extensions, state, properties, capabilities, utils,
}

texture.generateMipmaps = false;
textureProperties.__maxMipLevel = mipmaps.length - 1;

} else {

state.texImage2D( _gl.TEXTURE_2D, 0, glFormat, glFormat, glType, image );
textureProperties.__maxMipLevel = 0;

}

}

if ( textureNeedsGenerateMipmaps( texture, isPowerOfTwoImage ) ) _gl.generateMipmap( _gl.TEXTURE_2D );
if ( textureNeedsGenerateMipmaps( texture, isPowerOfTwoImage ) ) {

generateMipmap( texture, _gl.TEXTURE_2D );

}

textureProperties.__version = texture.version;

Expand Down Expand Up @@ -754,7 +784,12 @@ function WebGLTextures( _gl, extensions, state, properties, capabilities, utils,

}

if ( textureNeedsGenerateMipmaps( renderTarget.texture, isTargetPowerOfTwo ) ) _gl.generateMipmap( _gl.TEXTURE_CUBE_MAP );
if ( textureNeedsGenerateMipmaps( renderTarget.texture, isTargetPowerOfTwo ) ) {

generateMipmap( renderTarget.texture, _gl.TEXTURE_CUBE_MAP );

}

state.bindTexture( _gl.TEXTURE_CUBE_MAP, null );

} else {
Expand All @@ -763,7 +798,12 @@ function WebGLTextures( _gl, extensions, state, properties, capabilities, utils,
setTextureParameters( _gl.TEXTURE_2D, renderTarget.texture, isTargetPowerOfTwo );
setupFrameBufferTexture( renderTargetProperties.__webglFramebuffer, renderTarget, _gl.COLOR_ATTACHMENT0, _gl.TEXTURE_2D );

if ( textureNeedsGenerateMipmaps( renderTarget.texture, isTargetPowerOfTwo ) ) _gl.generateMipmap( _gl.TEXTURE_2D );
if ( textureNeedsGenerateMipmaps( renderTarget.texture, isTargetPowerOfTwo ) ) {

generateMipmap( renderTarget.texture, _gl.TEXTURE_2D );

}

state.bindTexture( _gl.TEXTURE_2D, null );

}
Expand All @@ -789,7 +829,7 @@ function WebGLTextures( _gl, extensions, state, properties, capabilities, utils,
var webglTexture = properties.get( texture ).__webglTexture;

state.bindTexture( target, webglTexture );
_gl.generateMipmap( target );
generateMipmap( texture, target );
state.bindTexture( target, null );

}
Expand Down