Replace 8 with real maxMipLevel in lights_fragment_maps.glsl #13501

takahirox · 2018-03-05T16:50:41Z

This PR is still WIP but you can test if this change fixes #13107
I calculate max mip level by log2( max( image.width, image.height ) ) for gl.generateMipmap.

I'll add comments later about what I'm worried about in this impl.

emackey · 2018-03-05T17:05:27Z

src/renderers/shaders/ShaderChunk/lights_fragment_maps.glsl


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

-	// TODO, replace 8 with the real maxMIPLevel


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

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

Updated, thanks!

takahirox · 2018-03-06T14:18:14Z

src/renderers/webgl/WebGLTextures.js

+
+		_gl.generateMipmap( target );
+
+		var image = Array.isArray( texture.image ) ? texture.image[ 0 ] : texture.image;


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

It's not guaranteed, but lets assume that.

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

takahirox · 2018-03-06T14:32:08Z

Changes and notes.

Using uniforms for maxMipLevel in glsl rather than using #define because texture can be updated.
Calculating max mip level by log2( max( image.width, image.height ) ) for gl.generateMipmap.
Holding maxMipLevel in textureProperties.

I add some questions inline.

takahirox · 2018-03-06T14:40:59Z

src/renderers/webgl/WebGLTextures.js

+
+		var image = Array.isArray( texture.image ) ? texture.image[ 0 ] : texture.image;
+		var textureProperties = properties.get( texture );
+		textureProperties.__maxMipLevel = Math.max( Math.log2( Math.max( image.width, image.height ) ), textureProperties.__maxMipLevel );


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.

@kenrussell do you know the answer for this?

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

It says that levels level_base+1 through q are derived from the contents of level_base. 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.

@kenrussell thanks!

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.

mrdoob · 2018-03-07T02:47:40Z

Merging for now.

mrdoob · 2018-03-07T03:35:29Z

It'd be nice to compare how different is this from PMREMGenerator.

But, for people not using it, rough reflections look much better indeed!

mrdoob · 2018-03-08T06:56:09Z

In ChromeOS seems like it gets blurred too quickly?

mrdoob · 2018-03-08T19:13:31Z

On windows...

I see the same behavior with Microsoft Edge:

Chrome seems fine:

And so seems Firefox:

takahirox · 2018-03-08T19:24:22Z

Confirmed it with Windows + Edge here, too.

Hm, I guess it could be from the WebGL (extensions) capability?

mrdoob · 2018-03-08T21:04:46Z

Which extension would it be?

takahirox · 2018-03-08T21:13:42Z

I supposed float texture but Edge has. Hm, I might be wrong?

Chrome

Edge

mrdoob · 2018-03-08T23:12:23Z

I was not aware this code relied in floating point textures...?

mrdoob · 2018-03-08T23:14:21Z

I guess maxMipLevel that gets computed is different in Edge/Chrome OS?

takahirox · 2018-03-08T23:26:56Z

I was not aware this code relied in floating point textures...?

Ah, sorry. I just said floating texture without thinking deeply because I haven't looked into the related shader code yet.

takahirox · 2018-03-08T23:33:49Z

I guess maxMipLevel that gets computed is different in Edge/Chrome OS?

You meant, you guess the behavior of gl.generateMipmap() could be different?

takahirox · 2018-03-08T23:35:00Z

This issue seems appearing even in r90. I'm really not sure if it's related to mipmap yet.

mrdoob · 2018-03-08T23:58:46Z

This issue seems appearing even in r90. I'm really not sure if it's related to mipmap yet.

That is a good point.

mrdoob · 2018-03-09T00:06:33Z

Wild guess... Maybe this is related to window.devicePixelRatio?

mrdoob · 2018-03-09T00:07:58Z

@takahirox is window.devicePixelRatio the same value in Edge and Chrome in your setup?

WestLangley · 2018-03-09T00:09:49Z

Another related point:

Our material.roughness is intended to be "perceptual" roughness. I would not be surprised if metallic and non-metallic materials responded differently to that property value. In any event, this is going to be somewhat subjective.

How the shader selects the mipmap level based on the roughness property is based on code copied from another source. That code can -- and may have to -- be changed.

takahirox · 2018-03-09T00:26:41Z

is window.devicePixelRatio the same value in Edge and Chrome in your setup?

Same.

Replace 8 with real maxMipLevel in lights_fragment_maps.glsl

f860006

takahirox mentioned this pull request Mar 5, 2018

MeshStandardMaterial: Reflections visible for moderate roughness values #13107

Closed

emackey reviewed Mar 5, 2018

View reviewed changes

takahirox added 3 commits March 6, 2018 04:14

Fix typo

ddc3538

Replace another 8 with maxMipLevel in lights_fragment_maps.glsl

07256a3

Hold maxMipMapLevel in textureProperties

33ecb12

takahirox commented Mar 6, 2018

View reviewed changes

mrdoob merged commit 51fe8f7 into mrdoob:dev Mar 7, 2018

mrdoob added this to the r91 milestone Mar 7, 2018

takahirox mentioned this pull request Mar 7, 2018

Clean up WebGLTexture #13519

Merged

Mugen87 changed the title ~~[WIP] Replace 8 with real maxMipLevel in lights_fragment_maps.glsl~~ Replace 8 with real maxMipLevel in lights_fragment_maps.glsl Mar 11, 2018

donmccurdy mentioned this pull request Mar 12, 2018

GLTFExporter GLB compatible with Facebook viewer #13397

Closed

5 tasks

donmccurdy mentioned this pull request May 27, 2018

[WIP] GLTFLoader: Use node materials. #14149

Closed

donmccurdy mentioned this pull request Jun 8, 2018

StandardNodeMaterial: Rough metals are too reflective. #14238

Closed

donmccurdy mentioned this pull request Jul 10, 2018

SpecGloss samples KhronosGroup/glTF-Sample-Models#181

Closed

Mugen87 mentioned this pull request Mar 16, 2021

envMap doesnt work correctly in r91 #13823

Closed

12 tasks

WestLangley mentioned this pull request Jan 4, 2022

Shaders: restore support for standard cube maps in physical shader #23144

Closed


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

		// TODO, replace 8 with the real maxMIPLevel


		_gl.generateMipmap( target );

		var image = Array.isArray( texture.image ) ? texture.image[ 0 ] : texture.image;

Uh oh!

Replace 8 with real maxMipLevel in lights_fragment_maps.glsl #13501

Replace 8 with real maxMipLevel in lights_fragment_maps.glsl #13501

Uh oh!

Conversation

takahirox commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

takahirox Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

takahirox commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

takahirox Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrdoob commented Mar 7, 2018

Uh oh!

mrdoob commented Mar 7, 2018

Uh oh!

mrdoob commented Mar 8, 2018

Uh oh!

mrdoob commented Mar 8, 2018

Uh oh!

takahirox commented Mar 8, 2018

Uh oh!

mrdoob commented Mar 8, 2018

Uh oh!

takahirox commented Mar 8, 2018

Uh oh!

mrdoob commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrdoob commented Mar 8, 2018

Uh oh!

takahirox commented Mar 8, 2018

Uh oh!

takahirox commented Mar 8, 2018

Uh oh!

takahirox commented Mar 8, 2018

Uh oh!

mrdoob commented Mar 8, 2018

Uh oh!

mrdoob commented Mar 9, 2018

Uh oh!

mrdoob commented Mar 9, 2018

Uh oh!

WestLangley commented Mar 9, 2018

Uh oh!

takahirox commented Mar 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

takahirox commented Mar 5, 2018 •

edited

Loading

takahirox Mar 5, 2018 •

edited

Loading

takahirox commented Mar 6, 2018 •

edited

Loading

takahirox Mar 6, 2018 •

edited

Loading

mrdoob commented Mar 8, 2018 •

edited

Loading