Malt icon indicating copy to clipboard operation
Malt copied to clipboard

Node Groups

Open pragma37 opened this issue 2 years ago • 15 comments

Initial Node Groups implementation.

Each shader type now has a corresponding group type (ie. Mesh and Mesh Group). These groups can be used with the new Node Group node (Add Node > Other > Node Group).

Group graphs generate a function in a separate file that can be imported by the node trees that use them. All node group parameters are private and their values are stored in the tree itself (this greatly simplifies the implementation).

pragma37 avatar Sep 11 '22 22:09 pragma37

I did not have time to look into the implementation of groups but one thing that can be made possible using groups is a loop node. UI-wise they are always tricky but with groups you would be able to load in a group as your loop target and set an iterations number. Passing the iteration count to the group would basically open up the entire workflow using loops that was only available through code before.

I was wondering if the design of the groups in malt would allow for something like this. if it generates a callable function then maybe the wrapper loop node could output a loop block as its source code containing that function or something

Kolupsy avatar Sep 16 '22 14:09 Kolupsy

Also we should allow creating new group node trees from the Group Node UI as well as the ability to select certain nodes and compress them down to a node group

Kolupsy avatar Sep 16 '22 14:09 Kolupsy

if it generates a callable function then maybe the wrapper loop node could output a loop block as its source code containing that function or something

Yep, that should be doable.
If you look into the MaltGroupNode class, it doesn't even override the get_source_code function, it compiles into a regular function call. For a loop node, you just would need a group with an iteration count parameter.

Also we should allow creating new group node trees from the Group Node UI as well as the ability to select certain nodes and compress them down to a node group

Agree, but honestly, I'm somewhat tired of working on so much node/UI stuff lately.
I implemented the core group functionality because it requires a full understanding of the whole compilation process, but I would like to focus on rendering again.

pragma37 avatar Sep 16 '22 16:09 pragma37

Agree, but honestly, I'm somewhat tired of working on so much node/UI stuff lately.

I can take care of that if you want. Cant do it right this second but I hope I find some time this week. (so much stuff going on now I will need to make myself a todo list :D)

For a loop node, you just would need a group with an iteration count parameter.

I think that should be enforceable. bpy.props.PointerPropery allows you to poll objects that you want to reference. Needs to be clear to the user as well but from a code-security standpoint this can be done easily.

I would like to focus on rendering again.

I feel you! Do you already have in mind what you want to tackle?

Kolupsy avatar Sep 16 '22 16:09 Kolupsy

Do you already have in mind what you want to tackle?

A while ago, I implemented support for freestyle edges and geometry based line detection (like Freestyle/LineArt) for contours and creases. It would be nice to finish and merge that. I also want to improve shadow map filtering and add support for light probes and screen tracing.

There's plenty of other stuff I'd like to experiment with, but those are the main ones I think.

pragma37 avatar Sep 17 '22 14:09 pragma37

Been testing this out for a good bit, converting old shaders to node groups for efficiency's sake. I'll probably make a longer discussion thread later about some stuff I'd like to see, but figured I should bring this up asap due to how negatively it can affect the workflow.

You can't have input properties and output properties with the same name, you always get a redefinition error when doing so. Which it makes sense, I'm guessing this is just how GLSL works? But some sort of fail safe is needed I think, people should be able to call their properties what they want. Perhaps on the frontend, Malt displays the properties as you type them, but in the code it adds Input or Output to the beginning of the property, so that there can't be conflicts there?

Or maybe at the very least Malt could entirely prevent you from adding duplicate property names? I was really scratching my head for awhile last night trying to figure out what I was doing wrong with the Node Groups but yeah it was just duplicate properties 😄

dibli-goost avatar Sep 26 '22 18:09 dibli-goost

Fixed! Thanks for the testing.

pragma37 avatar Sep 26 '22 21:09 pragma37

Also we should allow creating new group node trees from the Group Node UI as well as the ability to select certain nodes and compress them down to a node group

I have implemented both these features in a rudimentary way. Do you have any requirements for any of those two functions?

Kolupsy avatar Oct 08 '22 13:10 Kolupsy

Awesome! You're pickier than me about node UX stuff, so if you are ok with the functionality I will probably be as well. 👍

pragma37 avatar Oct 08 '22 14:10 pragma37

another request was to be able to instantiate group nodes from existing groups similar to the 'Groups' category in blenders own node trees. What do you think about that. Can we maybe avoid adding a new category to save some space or would it be better that way?

Kolupsy avatar Oct 08 '22 14:10 Kolupsy

I think a new category is fine. IIRC that would also make it easier to populate the category with a callback?

pragma37 avatar Oct 08 '22 14:10 pragma37

ok here is the patch. wdyt?

Kolupsy avatar Oct 08 '22 15:10 Kolupsy

Looks good!

I see a few issues:

  • The nodes created with CRTL+G lose their property values.
  • IO nodes shouldn't be added to the group tree.
  • Ideally, one Input and Output node should be added by default to new group trees.

pragma37 avatar Oct 08 '22 15:10 pragma37

The nodes created with CRTL+G lose their property values.

hmm this could get tricky. I will see if I can come up with a solution for this

IO nodes shouldn't be added to the group tree.

yeah that makes sense. Should not be too difficult

Ideally, one Input and Output node should be added by default to new group trees.

In default blender nodes, when you create a group from just a single node, it will create inputs and outputs for all sockets of that node for the new group. Do you think we should have this too?

Kolupsy avatar Oct 08 '22 18:10 Kolupsy

Fixed the first 2.

Nodes CRTL+C/CRTL+V was already supported, but you need the original nodes to get something to copy from!

pragma37 avatar Oct 08 '22 20:10 pragma37