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

Shaders: Initialize geometryClearcoatNormal to suppress OpenGL warning. #27002

Merged
merged 1 commit into from Oct 18, 2023

Conversation

c-smile
Copy link
Contributor

@c-smile c-smile commented Oct 18, 2023

Fix of 0(1578) : 'warning C7050: "geometryClearcoatNormal" might be used before being initialized'

Fixed #26994

Description

This warning appears while running ThreeJS on top of WebGL implementation that uses system OpenGL directly.
In particular in Sciter Engine on Windows with OpenGL 4.6 / NVIDIA drivers.

This warning manifests on any attempt to use "physical" materials like:

const material = new THREE.MeshStandardMaterial( { color: 0xFFFFFF } );

Note, no such warning in non-physical materials like MeshNormalMaterial

Fix of 0(1578) : 'warning C7050: "geometryClearcoatNormal" might be used before being initialized' mrdoob#26994 issue.
@github-actions
Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
652.2 kB (161.6 kB) 652.3 kB (161.6 kB) +14 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
444.9 kB (107.6 kB) 444.9 kB (107.6 kB) +14 B

@Mugen87 Mugen87 added this to the r158 milestone Oct 18, 2023
@Mugen87 Mugen87 merged commit eb397f8 into mrdoob:dev Oct 18, 2023
20 checks passed
@Mugen87 Mugen87 changed the title Update lights_fragment_begin.glsl.js Shaders: Initialize geometryClearcoatNormal to suppress OpenGL warning. Oct 18, 2023
@WestLangley
Copy link
Collaborator

Initializing a normal to zero raises a red flag: normals are of unit length.

Looking into this, vec3 geometryClearcoatNormal should be declared and initialized inside #ifdef USE_CLEARCOAT.

But you can't do that because of the refactoring in done in #26805.

RE_Direct( directLight, geometryPosition, geometryNormal, geometryViewDir, geometryClearcoatNormal, material, reflectedLight );

//

Also, CSMShader appears to have the same issue.

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 18, 2023

This initialization is purely technical. As long as the code doesn't use the normal, I see no issue, tbh.

Mugen87 added a commit that referenced this pull request Oct 18, 2023
Applying the same change like in #27002.
@mrdoob
Copy link
Owner

mrdoob commented Oct 26, 2023

@WestLangley Would it be better to initialize it as vec3( 0.0, 0.0, 1.0 )?

@WestLangley
Copy link
Collaborator

@mrdoob The real problem is it shouldn't be instantiated at all if it is not going to be used.

@mrdoob
Copy link
Owner

mrdoob commented Oct 26, 2023

Ah, so we could do this instead?

#ifdef USE_CLEARCOAT
	vec3 geometryClearcoatNormal = clearcoatNormal;
#endif

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 26, 2023

This does not work which is explained here: #27002 (comment)

@Mugen87
Copy link
Collaborator

Mugen87 commented Oct 26, 2023

I have the feeling we overthink this issue a bit. As long as geometryClearcoatNormal isn't used, it's value really does not matter.

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.

Getting warning in custom WebGL implementation running ThreeJS
4 participants