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: Rework ShaderNodeElements and make some other refactorings #25498

Merged
merged 8 commits into from
Feb 17, 2023

Conversation

LeviPesin
Copy link
Contributor

@LeviPesin LeviPesin commented Feb 13, 2023

Fixes #25240, partially fixes #23666
Implements #25240 (comment) and #25240 (comment)

Description

Changes:

  • Remove ShaderNodeElements and ShaderNodeBaseElements -- now the corresponding functions are exported by nodes files themself. Nothing changes for importing from three/nodes,
  • Now some nodes assume that all nodes passed to them are in nodeObject form (user should manage that, but it's mostly managed by the nodes system),
  • Introduce addNodeElement/addLightNode/addNodeClass/addNodeMaterial functions,
  • Some refactoring introducing more of node chaining (it especially greatly simplifies functions) in most nodes files (examples can be refactored in a separate PR),
  • Rename VarNode.add/sub/mul/div() to ShaderNode.addAssign()/subAssign()/etc (with semantics changed very slightly).

Tested all examples -- they seem to work.

@LeviPesin
Copy link
Contributor Author

/ping @sunag @epreston This PR should resolve #25240.

@sunag
Copy link
Collaborator

sunag commented Feb 13, 2023

@LeviPesin Awesome! It just broke webgpu_lights_custom example. I'm investigating...

@sunag sunag added this to the r150 milestone Feb 13, 2023
@sunag
Copy link
Collaborator

sunag commented Feb 14, 2023

Seems that we have just one last circular dependency in NodeLoader.js

npx madge examples/jsm/nodes/Nodes.js --circular --warning

image

@LeviPesin
Copy link
Contributor Author

I think we can move nodeLib and fromType to a separate file and this should resolve the issue... Will do that.

@LeviPesin
Copy link
Contributor Author

Done!

@sunag
Copy link
Collaborator

sunag commented Feb 14, 2023

@LeviPesin We should give the option to expand the serialization, Node.fromType() is a good API, I think we should use a nodeLib = new WeakMap inside Node.js add method like addNodeClass( SomeNode ) and addNodeElement().

@LeviPesin
Copy link
Contributor Author

Agreed! And same for materials.
Will do that soon...

@LeviPesin
Copy link
Contributor Author

LeviPesin commented Feb 15, 2023

Done!
Remaining questions:

  • What are the best names for addNodeElement/addLightNode/addNodeClass/addNodeMaterial? Maybe register, not add?
  • Shouldn't we move createNodeFromType and createMaterialFromType to corresponding classes so that they would be static methods?
  • Shouldn't we also move addNodeElement/addLightNode/addNodeClass/addNodeMaterial to classes?

@sunag
Copy link
Collaborator

sunag commented Feb 15, 2023

@mrdoob do you have some suggestion about this #25498 (comment)? I'm thinking of merge and we deciding these definitions soon.

@sunag
Copy link
Collaborator

sunag commented Feb 17, 2023

Modifications are more internal, so not affect the external API (examples/projects) or very little. Merging...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants