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

WebGPURenderer & PhysicalLightingModel: Added clearcoat #26211

Merged
merged 14 commits into from
Jun 8, 2023

Conversation

sunag
Copy link
Collaborator

@sunag sunag commented Jun 7, 2023

Description

The same example of WebGL, I just made some adjustments in scale, mainly because the WebGL example version is based in legacy lighting.

https://raw.githack.com/mrdoob/three.js/dev/examples/webgpu_clearcoat.html

image

@sunag sunag added this to the r154 milestone Jun 7, 2023
@sunag sunag marked this pull request as ready for review June 7, 2023 23:19

// EVENTS

new OrbitControls( camera, renderer.domElement );
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is a good idea to get in the habit of limiting the controls zoom/pan/dolly to reasonable limits appropriate for the example.


// Both indirect specular and indirect diffuse light accumulate here

const singleScattering = temp( vec3() );
const multiScattering = temp( vec3() );
const singleScattering = vec3().temp();
Copy link
Owner

Choose a reason for hiding this comment

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

What is temp()? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

It allows to create a new temporary variable.

Copy link
Owner

Choose a reason for hiding this comment

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

would clone() work too?

Copy link
Contributor

Choose a reason for hiding this comment

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

No. I meant "variable" as not a JS variable, but a variable in the final GLSL/WGSL code.

Copy link
Owner

Choose a reason for hiding this comment

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

What is a temporary variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

temp just tells to explicitly create a new variable, so that we can do assigning to it.

Copy link
Collaborator Author

@sunag sunag Jun 8, 2023

Choose a reason for hiding this comment

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

The Node System tries to optimize the code automatically putting all code in a single line if possible. If a node is used more than two time they automatically create a temporary variable. Like:

vec3<f32> nodeVar2 = ...

When .temp() is used force the system to create a variable, it is useful to store a variable that the user can modify itself using addAssign(), subAssign(), or assign() for example. This syntax in WGSL is like:

// TSL: const node = vec3().temp()
vec3<f32> nodeVar2 = vec3<f32>( 0.0 );

// TSL: node.addAssign( 1.0 )
nodeVar2 = nodeVar2 + vec3<f32>( 1.0 );
// without temp
let node = vec3();
node = node.add( 1.0 );
// WGSL: ... ?= ( vec3<f32>( 0.0 ) +  vec3<f32>( 1.0 ) )

// using temp
const node = vec3().temp();
node.addAssign( 1.0 );

// WGSL:
// vec3<f32> nodeVar2 = vec3<f32>( 0.0 );
// nodeVar2 = nodeVar2 + vec3<f32>( 1.0 );

Without temp is used in most of the time because have a better syntax using chain but in some cases when an incremental variable is used like outgoingLight is used in differences functions seems better approach to avoid use async of variable.

Copy link
Owner

Choose a reason for hiding this comment

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

Got it. Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Jun 8, 2023

I just made some adjustments in scale, mainly because the WebGL example version is based in legacy lighting.

Oh, could you also update the WebGL example so it doesn't use legacy lighting?

const RE_IndirectSpecular_Physical = new ShaderNode( ( inputs ) => {
const LM_Init = new ShaderNode( ( context, stack, builder ) => {

if ( builder.includes( clearcoat ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a bit of a hack?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm trying to convince myself that yes, but I think that currently is the better approach. I created a class to contextualize LighingModel but it creates more classes that can replaced by .includes(), basically if the user does not declare/assign clearcoat in .constructVariants() we can use builder.includes( clearcoat ) to detect that clearcoat is not used in this material.

Object.assign( properties, reflectedLight, lighting );
Object.assign( context, lighting );

context.reflectedLight = reflectedLight;
context.lightingModelNode = lightingModelNode || context.lightingModelNode;
// @TODO: Call needed return a new node ( or rename the ShaderNodeInternal.call() function ), it's not moment to run
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if I understand this comment... but can't we do shader( ( {}, stack, builder ) => lightingModel.init.call( context, stack, builder ) ), similar as how it was done in other places?

Copy link
Collaborator Author

@sunag sunag Jun 8, 2023

Choose a reason for hiding this comment

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

The syntax shader( ( {}, stack, builder ) => {} ) is correct, but the call syntax should be just .call( { ...inputs } ) like FunctionCallNode for the users and not .call( {}, stack, builder ). It because call() should return a ShaderCallNode and not run the function immediately. When it's called in construct() process, we can send stack and builder parameters, but if the user calls the shader() outside construct() like material.colorNode = shader(..).call() we would not be able to pass the builder/stack because it is in the wrong builder stage.

@sunag sunag merged commit de8f146 into mrdoob:dev Jun 8, 2023
@sunag sunag deleted the dev-webgpu-clearcoat branch June 10, 2023 12:00
@sunag sunag mentioned this pull request Jan 11, 2024
26 tasks
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.

4 participants