-
-
Notifications
You must be signed in to change notification settings - Fork 36.2k
TextureNode: Manage y-flip as a uniform. #31929
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
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
src/nodes/accessors/TextureNode.js
Outdated
| if ( this.sampler ) { | ||
|
|
||
| uvNode = uvNode.flipY(); | ||
| uvNode = this._flipYUniform.equal( true ).select( uvNode.flipY(), uvNode ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some examples fail e.g. webgpu_materials_matcap and it seems it has to do with the new select() statement. For some reasons, the compiled GLSL ends up wrong.
From SpectorJS:
nodeVar1 = bool( f_nodeUniform3 );
if ( ( nodeVar1 == true ) ) {
positionViewDirection = normalize( v_positionViewDirection );
nodeVar2 = normalize( vec3( positionViewDirection.z, 0.0, ( - positionViewDirection.x ) ) );
normalViewGeometry = normalize( v_normalViewGeometry );
NORMAL_normalView = normalViewGeometry;
normalView = NORMAL_normalView;
matcapUV = ( ( vec2( dot( nodeVar2, normalView ), dot( cross( positionViewDirection, nodeVar2 ), normalView ) ) * vec2( 0.495 ) ) + vec2( 0.5 ) );
nodeVar3 = ( f_nodeUniform4 * vec3( matcapUV, 1.0 ) ).xy;
nodeVar0 = vec2( nodeVar3.x, 1.0 - nodeVar3.y );
}
else {
matcapUV = ( ( vec2( dot( nodeVar2, normalView ), dot( cross( positionViewDirection, nodeVar2 ), normalView ) ) * vec2( 0.495 ) ) + vec2( 0.5 ) );
nodeVar0 = ( f_nodeUniform4 * vec3( matcapUV, 1.0 ) ).xy;
}
Um, it seems the if/else statement from the select holds way too much code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in the short-term this could potentially be resolved by using the uniformFlow context in your highlighted line of code, but the default behavior is certainly not correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a short test and got this as output in the fragment shader. There's probably some optimizations that can be done to only evaluate the f_nodeUniform4 * matcapUV operation once...
nodeVar17 = texture( nodeUniform2, ( nodeVar1 == true ) ? vec2( nodeVar16.x, 1.0 - nodeVar16.y ) : ( f_nodeUniform4 * vec3( matcapUV, 1.0 ) ).xy );There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you update the TSL source to get this output?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this._flipYUniform.equal( true ).select( uvNode.flipY(), uvNode ).uniformFlow();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I like the fact that the if/else blocks are replaced with a ternary operation.
Unfortunately, none of the suggested versions work right now. Both the uniformFlow() code as well as the uvNode.toVar() definitions break environment maps in different ways.
E.g. add uniformFlow() and then test webgpu_loader_gltf_transmission or webgpu_loader_gltf_sheen. The environment maps end up way too bright.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mugen87 I'm checking this bug, the caching and optimization system is failing at some point in the conditionals.
Fixed #31750.
Description
The PR makes sure
TextureNodemanages the y-flip as a uniform which allows to reuse a texture node with textures with different flip-y requirements.WebGPU is not affected by this change. The new uniform value is only created when the builder requests it which is only true for WebGL.