Page MenuHome

Add RNA classes for shader and compositor python nodes
ClosedPublic

Authored by Miguel Porces (cmporces) on Feb 18 2019, 7:46 PM.

Details

Summary

Blender had a node class available in the API that could be used to write custom pynodes for rendering in Cycles. This diff expands that possibility to create python nodes that can work with Cycles, Eevee, and Compositor render engines.

Diff Detail

Repository
rB Blender

Event Timeline

Noted some quick things below, but not sure why you assigned this to me? Am not particularly familiar with nodes code, think maybe @Brecht Van Lommel (brecht) would be best dev here?

source/blender/blenkernel/intern/material.c
1040

Better to use ELEM macro for that kind of things (also other similar cases below in patch):

if (ELEM(node->type, NODE_GROUP, NODE_CUSTOM_GROUP)) {

source/blender/blenkernel/intern/node.c
141–143

Why this move?

1009

Why this change?

Change node->type verifications to use ELEM()

Miguel Porces (cmporces) marked an inline comment as done and 2 inline comments as not done.Feb 19 2019, 11:47 AM
Miguel Porces (cmporces) added inline comments.
source/blender/blenkernel/intern/node.c
141–143

Because if we left it after the initfunc_api() (which is the case of python nodes that have a nodetree as id), the nodetree_users get incremented twice. The rna_NodeGroup_node_tree_set in rna_nodetree.c[2491] already increments the users there.
In other cases, the node->id should be set in the initfunc(), and the user increment will work ok.

1009

It makes little sense to call copyfunc_api when we are copying the node from the COW routines...

If we leave it as it was, we need to change the node_free_node_ex in node.c[1681] because we deliberately block freefunc_api there with use_api_free_cb=true.

I was able to port oslpy to eeve with very little effort using this.

however there is a constant warning

WARN (bpy.rna): K:\BlenderGit\blender\source\blender\python\intern\bpy_rna.c:1415 pyrna_enum_to_py: current value '9' matches no enum in 'ShaderNodeOSLPY', 'OSLPY', 'type'

not sure what that is all about....

Fixed updating (changes to the internal nodetree weren't propagating)
Add ui_hide option to nodetrees for allowing hiding them from the add menu

Correct last revision for allowing python nodes to receive update calls

Brecht Van Lommel (brecht) requested changes to this revision.Feb 26 2019, 1:48 PM

NODE_CUSTOM_GROUP should be added in rna_node_static_type_itemf.

intern/cycles/blender/blender_shader.cpp
1053

This could still check for RNA_NodeCustomGroup as well? To preserve API compatibility.

source/blender/blenkernel/intern/node.c
1010

Agree that localize should not copy Python data.

However it's not obvious from the code that LIB_ID_COPY_LOCALIZE copy always matches a use_api_free_cb == false free.

node_free_node_ex should check LIB_TAG_LOCALIZED probably.

source/blender/editors/space_node/node_edit.c
1575–1576 ↗(On Diff #13842)

What is the purpose of this change?

Python API users do not have to be aware of ID user management, so it's not clear to me why a Python node defining this method or not should have an impact here.

source/blender/makesrna/intern/rna_nodetree.c
8486–8488

What is the purpose of this setting, when is it intended to be used? The description does not make this clear.

It's probably not checked in all the places that show nodes in the UI, and it's not obvious to me why this is something that you can change dynamically.

8674–8675

Perhaps these should be named ShaderNodeCustomGroup and CompositorNodeCustomGroup for consistency, since there is also NODE_CUSTOM which is different.

Also, these could be subclasses of ShaderNode and CompositorNode I think?

source/blender/nodes/NOD_composite.h
139

Rename custom -> custom_group here and in other places.

This revision now requires changes to proceed.Feb 26 2019, 1:48 PM
source/blender/makesrna/intern/rna_nodetree.c
8486–8488

I can answer the why but not the implementation questions on this.

This was an informal request from me, the nodegroups used as backing storage for a custom node, show in the add menu in the node editor, which is slightly annoying if the addon makes 1 or 2 , quite annoying if you make 10's, and get super annoying when you have 10s that change often (in case of oslpy people switch to a different osl script) and blender goes 'yeah i saw you just deleted this group, but I'm gonna go ahead hold on to this until the next save/load cycle' so i'd prefer hiding the group rather than cluttering the add menu.

Miguel Porces (cmporces) marked 4 inline comments as done.Feb 27 2019, 10:25 AM
Miguel Porces (cmporces) updated this revision to Diff 13906.

Sugested Changes applied.

New diff uploaded with Brecht sugestions.

intern/cycles/blender/blender_shader.cpp
1053

I was thinking in making that change while reading old versions.
Old nodes based in RNA_NodeCustomGroup need to be rewritten for 2.80 anyway due to changes in the API.

source/blender/blenkernel/intern/node.c
1010

node_free_node_ex## changed.

source/blender/editors/space_node/node_edit.c
1575–1576 ↗(On Diff #13842)

I was reluctant with this change..
The main reason was to keep nodes that require unique nodetrees per instance, from filling up the ListBase with nodetrees.

Perhaps we could have an unique nodetree per instance, even in the cases where a shared one can be used; and create a specific free_function just to deal with these nodes.

I must think about this a bit deeper

source/blender/makesrna/intern/rna_nodetree.c
8674–8675

I haven't done it just because by making them subclasses of ShaderNode and CompositorNode, also makes the nodes subclasses of NodeInternal.
Don't know if that's important or relevant...

Brecht Van Lommel (brecht) requested changes to this revision.Feb 27 2019, 3:11 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/blenkernel/intern/node.c
1693

The line above checks if ntree is NULL, either it's not needed for either or it's needed for both.

source/blender/editors/space_node/node_edit.c
1575–1576 ↗(On Diff #13842)

Can't the node delete the node group in its free method, if there are no other users?

source/blender/makesrna/intern/rna_nodetree.c
8486–8488

Ok, this is a bit ill-defined as there are many places in the interface where you can access node groups: outliner, link/append, on a group node,, etc.

Probably we should not hide them everywhere, I guess it would be sufficient to do it in the node Add menu as done already, and also do it for the menu in node_templates.c that is used for properties editor node linking.

The name could be changed from ui_hide to internal with description "Node group is for internal use by another node, and hidden from node add menus."

8674–8675

I think it's fine to be a subclass of NodeInternal here, as these group nodes are internally defined node types.

This revision now requires changes to proceed.Feb 27 2019, 3:11 PM
source/blender/blenkernel/intern/node.c
1693

Ohh.. so it's better to check LIB_TAG_COPIED_ON_WRITE instead?

source/blender/editors/space_node/node_edit.c
1575–1576 ↗(On Diff #13842)

That's what i was doing prior to the new changes in the depsgraph... since then, it started to give a variety of errors, from user_decrements with no users, to copies and copies of the nodetree.

Some of those errors are fixed here, but there're some things that must be taken into account..

For example, in this line we decrement id_users, but after it we call freefunc_api, and it can happen that the writer of the node sets the self.nodetree=None in there, which decrements the id_users again...

It's possible to put in the documentation a "Things Not To Do" list, but I'd prefer to have a stable class, without fear of someone crashing the system.

source/blender/makesrna/intern/rna_nodetree.c
8486–8488

The idea was to just hide it in the Node Editor interface and nowhere else. Having the nodetrees showing up in the Outliner is ok, if not desirable. They are no secret, and it's good to know what is stored in the blend file.

I'll make the changes and check node_templates.c later.

source/blender/blenkernel/intern/node.c
1693

Most efficient and simplest would probably be to never call copyfunc_api or freefunc_api for copy-on-write or localize.

That could be done as follows:

  • Don't call copyfunc_api when LIB_ID_CREATE_NO_MAIN or LIB_ID_COPY_LOCALIZE is set.
  • Remove use_api_free_cb parameter from node_free_node_ex.
  • Rename remove_animdata to remove_main_data and used that instead of use_api_free_cb.
source/blender/editors/space_node/node_edit.c
1575–1576 ↗(On Diff #13842)

id_us_min(node->id) should really happen somewhere in nodeDeleteNode(), after calling freefunc_api but before freeing node itself.

Then the user counting should always be correct no matter what the script does.

You'd need to go over all places where nodeDeleteNode() is called to ensure the user counting is correct for each.

Move freefunc_api call to nodeDeleteNode; and some minor changes

Miguel Porces (cmporces) marked 2 inline comments as done.Mar 1 2019, 10:00 PM
Miguel Porces (cmporces) added inline comments.
source/blender/blenkernel/intern/node.c
1693

It seems to be working correctly now... but I found a flaw in the node_clipboard_clear functions, that needs to be fixed. Should we copy the nodetree referenced by the node into the node_clipboard, or just do a more extensive check in BKE_node_clipboard_validate?

source/blender/makesrna/intern/rna_nodetree.c
8486–8488

After looking to the UI_TemplateID code, I noticed that nodetrees get filtered there if their name starts with '.' , so I reverted this change and modified nodeitems_utils for doing the same filtering. Sounds simpler.

Miguel Porces (cmporces) marked an inline comment as done.Mar 5 2019, 6:55 PM

Fix node_clipboard_clear on exit.
Thought perhaps should be better to rewrite the clipboard functions so that we also keep a copy of the ID's. For now, python nodes that require unique nodetrees should check if the original nodetree still exists when pasting is called.

Looks good to me now, will do a bit more refactoring to simplify code and commit.

This revision is now accepted and ready to land.Mar 16 2019, 8:26 PM