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

Add label and temp functions to ShaderNode #23546

Merged
merged 11 commits into from
Mar 2, 2022
Merged

Add label and temp functions to ShaderNode #23546

merged 11 commits into from
Mar 2, 2022

Conversation

LeviPesin
Copy link
Contributor

@LeviPesin LeviPesin commented Feb 21, 2022

Related issue: - (mentioned in #23534)

Description

This PR adds the label and temp functions to the ShaderNode.

@sunag sunag self-assigned this Feb 21, 2022
@sunag
Copy link
Collaborator

sunag commented Mar 1, 2022

@LeviPesin do you think we got another name for it? Maybe setvar?

@LeviPesin
Copy link
Contributor Author

I actually think that makeVar is better, because it should be mainly used to create a new variable from a value to stop it from recalculating in further calculations (e.g. something like const four = makeVar(add(2, 2)); const eight = add(four, four)). setVar could be a good name for assign, but I like its current name too.

@sunag
Copy link
Collaborator

sunag commented Mar 2, 2022

I think that temp to define a variable and label to define a variable name?
It's just that for me the variable concept in ShaderNode should be the JS const or let.

About add( 2, 2 ) I think that this should be optimized by the NodeBuilder compiler and the variable should be created automatically if used more than once? At least it was supposed to work that way.

// 1. force variable creation
const four = temp( add( 2, 2 ) );

// 2.  define a var name in NodeBuilder -> GLSL or WGSL
label( four, 'FOUR' ); 

// 3. create a named variable
const four = label( add( 2, 2 ), 'FOUR' );

@LeviPesin
Copy link
Contributor Author

I think that temp to define a variable and label to define a variable name?

I like this approach!

I think that this should be optimized by the NodeBuilder compiler and the variable should be created automatically if used more than once? At least it was supposed to work that way.

Here is an example:
https://jsfiddle.net/aopLv2cx/
There is a duplicate calculation of dot(nodeVar2, nodeVar3). If you comment line 61 and uncomment 60, it will work properly.

@LeviPesin LeviPesin changed the title Add makeVar function to ShaderNode Add label and temp functions to ShaderNode Mar 2, 2022
@sunag sunag added this to the r139 milestone Mar 2, 2022
@sunag
Copy link
Collaborator

sunag commented Mar 2, 2022

@LeviPesin It seems that exist some conflicts in this PR.

@sunag
Copy link
Collaborator

sunag commented Mar 2, 2022

There is a duplicate calculation of dot(nodeVar2, nodeVar3). If you comment line 61 and uncomment 60, it will work properly.

This shouldn't happen, I will investigate.
Thanks for the example.

@LeviPesin
Copy link
Contributor Author

@sunag I think I resolved the conflicts.

@sunag sunag merged commit 5439159 into mrdoob:dev Mar 2, 2022
@sunag
Copy link
Collaborator

sunag commented Mar 2, 2022

Thanks! :)

@LeviPesin LeviPesin deleted the patch-1 branch March 2, 2022 19:01
donmccurdy pushed a commit to donmccurdy/three.js that referenced this pull request Mar 10, 2022
* Add makeVar function to ShaderNode

* Fix

* Update VarNode

* Update makeVar

* Add temp and label functions

* Refactor label function

* Try to resolve merge conflicts

* Try to resolve merge conflicts

* Try to resolve merge conflicts

* Try to resolve merge conflicts
@LeviPesin
Copy link
Contributor Author

@sunag Is there any progress on fixing this example? Should I make a new issue for it?

@LeviPesin
Copy link
Contributor Author

#23728

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
* Add makeVar function to ShaderNode

* Fix

* Update VarNode

* Update makeVar

* Add temp and label functions

* Refactor label function

* Try to resolve merge conflicts

* Try to resolve merge conflicts

* Try to resolve merge conflicts

* Try to resolve merge conflicts
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.

2 participants