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.

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

MeshMatcapMaterial: update default matcap #23126

Merged
merged 1 commit into from
Jan 3, 2022

Conversation

WestLangley
Copy link
Collaborator

Previously, if the matcap was not defined, the shader would render white:

Screen Shot 2021-12-30 at 5 49 14 PM

We can actually improve on the default with a small change to the shader:

Screen Shot 2021-12-30 at 5 51 22 PM

Just a suggestion... :-)

@mrdoob
Copy link
Owner

mrdoob commented Dec 30, 2021

Interesting... Another option would be to generate a default matcap DataTexture?

@WestLangley
Copy link
Collaborator Author

Another option would be to generate a default matcap DataTexture?

Yes, this PR was in lieu of doing something like this:

const matcap = new THREE.DataTexture( new Uint8Array( [ 50, 50, 50, 200, 200, 200 ] ), 1, 2, THREE.RGBFormat );
matcap.encoding = THREE.LinearEncoding;
matcap.minFilter = THREE.LinearFilter;
matcap.magFilter = THREE.LinearFilter;
matcap.generateMipmaps = false;
matcap.needsUpdate = true;

The result is similar.

@mrdoob
Copy link
Owner

mrdoob commented Dec 31, 2021

I think I prefer the DataTexture approach. The "dumber" the shaders are the better.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 31, 2021

const matcap = new THREE.DataTexture( new Uint8Array( [ 50, 50, 50, 200, 200, 200 ] ), 1, 2, THREE.RGBFormat );

For the reasons mentioned in #22293 (comment), I suggest to not use RGBFormat in new code anymore. At some point, we should even consider to possible deprecate all RGB formats.

WebGPU does not support them anyway, see gpuweb/gpuweb#66.

@WestLangley
Copy link
Collaborator Author

So, @Mugen87 @mrdoob, it would be the renderer's responsibility to populate a default matcap if none is specified. This would be checked per-frame. Is that right?

//

FYI, you do not require that approach for the MeshToonMaterial gradient texture.

vec3 getGradientIrradiance( vec3 normal, vec3 lightDirection ) {
// dotNL will be from -1.0 to 1.0
float dotNL = dot( normal, lightDirection );
vec2 coord = vec2( dotNL * 0.5 + 0.5, 0.0 );
#ifdef USE_GRADIENTMAP
return vec3( texture2D( gradientMap, coord ).r );
#else
return ( coord.x < 0.7 ) ? vec3( 0.7 ) : vec3( 1.0 );
#endif
}

I am implementing the same approach here.

@Mugen87
Copy link
Collaborator

Mugen87 commented Dec 31, 2021

The PR in its current form provides indeed the simpler solution. It would be nice to avoid the per-frame checks in the renderer.

@WestLangley
Copy link
Collaborator Author

@Mugen87 So, is that a tacit approval? :-)

@WestLangley WestLangley added this to the r137 milestone Jan 2, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 3, 2022

Changed my mind 🤓

@mrdoob mrdoob merged commit 860093d into mrdoob:dev Jan 3, 2022
@mrdoob
Copy link
Owner

mrdoob commented Jan 3, 2022

Thanks!

@WestLangley WestLangley deleted the dev_matcap_default branch January 3, 2022 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants