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

Nodes: Add more exports to ShaderNode #23901

Merged
merged 30 commits into from
Apr 29, 2022
Merged

Conversation

LeviPesin
Copy link
Contributor

@LeviPesin LeviPesin commented Apr 15, 2022

Fixes #23564

Description

This PR adds more exports to the ShaderNode.

Breaking changes:

  • Renamed MathNode.RAD and MathNode.DEG to MathNode.RADIANS and MathNode.DEGREES.
  • Removed join() function from the ShaderNode (use vec2/vec3/vec4/etc instead),
  • viewMatrix now should be used as viewMatrix().

@sunag Not sure about how arrayUniform() should work, what do you suggest?

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Apr 15, 2022

So, there are two problems with the circular inclusion:

  1. "Top-level problem". If a file B imports something from file A, while file A imports e.g. a class B from B, A must not use B on the top-level. It can be solved by changing nodeImmutables of some classes (ReflectNode, CubeTextureNode, etc) to nodeProxyies (but then reflectVector() and reflectCube() must be used instead of reflectVector and reflectCube, which is not very good), but...
  2. ...it will not solve the problem because of the "non-lazy evaluations top-level problem". Because JavaScript does not support lazy evaluations in any form, if we write nodeProxy( ReflectNode ), it will try to evaluate ReflectNode before executing the functions, again running into the top-level problem. The only solution to this is to inline the nodeProxy code (or use eval).

Obviously these problems can be circumvented by creating one more ShaderNode file containing e.g. core, math, and basic accessors... But it is not a very reliable solution (I can easily imagine a node importing a node importing a node, so on, all of which should be exported by the ShaderNode).

@sunag
Copy link
Collaborator

sunag commented Apr 19, 2022

@LeviPesin I think that we can have a ShaderNodeBaseElements.js for bringing all GLSL functions and ShaderNodeElements.js for all others Three.js features like reflectCube and reflectVec.

@LeviPesin
Copy link
Contributor Author

Yes, I now also think it would be better and easier (but still can cause some troubles)... OK, will do that tomorrow.

@LeviPesin
Copy link
Contributor Author

Done.

@sunag
Copy link
Collaborator

sunag commented Apr 22, 2022

@sunag What do you think about arrayUniform()? What should be its signature?

I think that: uniforms( nodes ).

@LeviPesin
Copy link
Contributor Author

I am asking because I am not sure if something like [ vec3(), mat4() ] would even work... Actually, what is the usecase for ArrayUniformNode? There are no examples of using it.

@sunag
Copy link
Collaborator

sunag commented Apr 22, 2022

I am asking because I am not sure if something like [ vec3(), mat4() ] would even work... Actually, what is the usecase for ArrayUniformNode? There are no examples of using it.

I see... the type of array should be optional in this case, but we can be added this in the second argument like:
uniforms( nodes, type = null )

@LeviPesin
Copy link
Contributor Author

Still not sure about the usecases...

@LeviPesin
Copy link
Contributor Author

@sunag Maybe it would be better to merge this PR and decide later whether and how arrayUniform should be implemented?

@sunag
Copy link
Collaborator

sunag commented Apr 25, 2022

Seems that this PR broke webgpu_compute example.

@sunag
Copy link
Collaborator

sunag commented Apr 26, 2022

Try test with:

uniform( new THREE.Vector2() );

@LeviPesin
Copy link
Contributor Author

Should be fixed now.

@sunag
Copy link
Collaborator

sunag commented Apr 26, 2022

Seems that this PR broke webgpu_compute example.

See this: d4cbbee maybe you need revert it too

@LeviPesin
Copy link
Contributor Author

So, I found a workaround, but it feels quite hacky... It seems that I do not quite understand how the system of the TempNodes work...

@sunag
Copy link
Collaborator

sunag commented Apr 26, 2022

For this case is better use context:

context( add( particle, velocity ), { temp: false } )

@LeviPesin LeviPesin changed the title Add more exports to ShaderNode Nodes: Add more exports to ShaderNode Apr 27, 2022
@sunag sunag added this to the r140 milestone Apr 29, 2022
@sunag sunag merged commit 6e51472 into mrdoob:dev Apr 29, 2022
@sunag
Copy link
Collaborator

sunag commented Apr 29, 2022

Thanks @LeviPesin!

@LeviPesin LeviPesin deleted the add-more-exports branch April 29, 2022 06:41
abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
* Add more exports to ShaderNode

* Fix brackets

* Fix one more bracket

* Fixes

* Another fix

* Cleanup

* Fix quotes

* Fixes

* Fix

* Another fix

* One more fix

* New fix

* Fix utils

* Cleanup

* Change `func` signature

* Fix

* Add `storage` and `compute`

* Fixes

* Fix

* `functionCall` -> `call`

* Remove `arrayElement`

* Update `compute` signature

* Update webgpu_compute.html

* Fix `getConstNodeType`

* Workaround

* Use `context`

* Radians, degrees, inversesqrt

* Remove 'inverseSqrt' alias
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.

Nodes: Add more exports to ShaderNode
2 participants