Node panels: RNA for node group interfaces #110952

Lukas Tönne · 2023-08-09T12:57:13+02:00

Lukas Tönne commented

2023-08-09 12:57:13 +02:00

Defines the RNA API for the new node tree interfaces.

The bulk of the RNA definition lives in rna_node_tree_interface.cc. The legacy socket interfaces remain in place and will be removed later.

Since the socket items share the bNodeSocketValueXXX structs with the bNodeSocket types they also use the same RNA. The rna_def_node_socket_interface_subtypes function is exposed so that when defining the interface RNA it can use the same code as the regular socket RNA.

Part 2/3 of #109135, #110272 Defines the RNA API for the new node tree interfaces. The bulk of the RNA definition lives in `rna_node_tree_interface.cc`. The legacy socket interfaces remain in place and will be removed later. Since the socket items share the `bNodeSocketValueXXX` structs with the `bNodeSocket` types they also use the same RNA. The `rna_def_node_socket_interface_subtypes` function is exposed so that when defining the interface RNA it can use the same code as the regular socket RNA.

Lukas Tönne added 1 commit 2023-08-09 12:57:19 +02:00

1eec2012c7 Add RNA definition for the new node tree interfaces.

Lukas Tönne requested review from Jacques Lucke 2023-08-09 12:57:27 +02:00

Lukas Tönne requested review from Hans Goudey 2023-08-09 12:57:33 +02:00

Lukas Tönne added this to the Nodes & Physics project 2023-08-09 12:57:39 +02:00

Lukas Tönne reviewed 2023-08-09 13:00:04 +02:00

source/blender/makesrna/RNA_enum_types.h Outdated

						
				@ -50,6 +50,7 @@ const EnumPropertyItem *rna_node_tree_type_itemf(void *data,

				                                                 bool (*poll)(void *data, struct bNodeTreeType *),

				                                                 bool *r_free);

				int rna_node_socket_idname_to_enum(const char *idname);

Lukas Tönne commented

2023-08-09 13:00:04 +02:00

This was removed in an earlier commit, but interfaces use it to create selectable socket type buttons. The previous interface implementation used an operator with its own enum for this purpose, but the new interface can do this with a regular property.

Lukas Tönne reviewed 2023-08-09 13:01:07 +02:00

source/blender/makesrna/intern/rna_node_socket.cc Outdated

						
				@ -561,2 +563,3 @@

				/* ******** Node Socket Subtypes ******** */

				static void rna_NodeSocketStandard_float_range(

				void rna_NodeSocketStandard_float_range(

Lukas Tönne commented

2023-08-09 13:01:07 +02:00

The interface definition uses the same range callbacks as node sockets, but gets defined in a different compilation unit, so they need to be exposed.

Hans Goudey reviewed 2023-08-11 19:06:45 +02:00

Hans Goudey left a comment

Generally seems okay. I think I might have been misreading some of the diff, since you mentioned in the description that you were keeping the old interface RNA definitions temporarily. But I guess it wasn't clear which parts you want to keep or remove long term.

source/blender/makesrna/intern/makesrna.cc Outdated

						
				@ -4727,6 +4727,7 @@ static RNAProcessItem PROCESS_ITEMS[] = {

				    {"rna_nla.cc", nullptr, RNA_def_nla},

				    {"rna_nodetree.cc", nullptr, RNA_def_nodetree},

				    {"rna_node_socket.cc", nullptr, RNA_def_node_socket_subtypes},

				    {"rna_node_tree_interface.cc", NULL, RNA_def_node_tree_interface},

Hans Goudey commented

2023-08-11 18:54:34 +02:00

nullptr

`nullptr`

LukasTonne marked this conversation as resolved

source/blender/makesrna/intern/rna_node_socket.cc Outdated

						
				@ -942,0 +992,4 @@

				      func, "color", 4, default_draw_color, 0.0f, 1.0f, "Color", "", 0.0f, 1.0f);

				  RNA_def_function_output(func, parm);

				  /* XXX Legacy socket interface, this will be removed. */

Hans Goudey commented

2023-08-11 18:56:27 +02:00

It's a bit confusing reading "legacy, will be removed" in new code in a PR for a system that's replacing the old interface-- is this comment still relevant? Maybe a chance to do it right or remove deprecated methods? Am I missing something?

Lukas Tönne commented

2023-08-17 10:17:37 +02:00

Yeah, splitting up PRs can get a bit confusing, i'm replacing the comment with

    /* Note: Legacy socket interface below.
     * The new interface RNA is defined in a separate file,
     * the NodeSocketInterface struct will be replaced. */

Reason this shows up as "new code" is that i've moved the final socket subtypes definitions. Previously rna_def_node_socket_standard_types included both the NodeSocketInterfaceStandard base struct and the final types (NodeSocketFloatUnsigned etc.).

Final types are now defined in rna_def_node_socket_subtypes, and a similar function rna_def_node_socket_interface_subtypes is called for defining the respective interface structs.

Yeah, splitting up PRs can get a bit confusing, i'm replacing the comment with ``` /* Note: Legacy socket interface below. * The new interface RNA is defined in a separate file, * the NodeSocketInterface struct will be replaced. */ ``` Reason this shows up as "new code" is that i've moved the final socket subtypes definitions. Previously `rna_def_node_socket_standard_types` included both the `NodeSocketInterfaceStandard` base struct and the final types (`NodeSocketFloatUnsigned` etc.). Final types are now defined in `rna_def_node_socket_subtypes`, and a similar function `rna_def_node_socket_interface_subtypes` is called for defining the respective interface structs.

👍 1

LukasTonne marked this conversation as resolved

source/blender/makesrna/intern/rna_node_socket.cc Outdated

						
				@ -1681,0 +2014,4 @@

				  const char *label;

				};

				/* XXX would like to have this in BKE and base all the subtype stuff (nodeStaticSocketType***)

Hans Goudey commented

2023-08-11 19:03:53 +02:00

I'd rephrase this comment as a "note" or something, or change it in this PR. Or maybe you had a different plan

LukasTonne marked this conversation as resolved

source/blender/makesrna/intern/rna_node_socket.cc Outdated

						
				@ -1681,0 +2018,4 @@

				 * on the same central list instead of repeating the same switch statements everywhere.

				 * But makesrna cannot have a dependency on BKE, so this list would have to live in RNA itself,

				 * with BKE etc. accessing the RNA API to get the subtypes info. */

				static blender::Span<bNodeSocketStaticTypeInfo> nodeSocketStaticTypeInfo()

Hans Goudey commented

2023-08-11 19:05:19 +02:00

Might be nice to use snake_case here like most other "new" functions

LukasTonne marked this conversation as resolved

source/blender/makesrna/intern/rna_node_socket.cc Outdated

						
				@ -1681,0 +2020,4 @@

				 * with BKE etc. accessing the RNA API to get the subtypes info. */

				static blender::Span<bNodeSocketStaticTypeInfo> nodeSocketStaticTypeInfo()

				{

				  static const blender::Array<bNodeSocketStaticTypeInfo> node_socket_subtypes = {

Hans Goudey commented

2023-08-11 19:04:28 +02:00

I think std::array is generally preferred for static arrays since it doesn't allocate or store the size

I think `std::array` is generally preferred for static arrays since it doesn't allocate or store the size

Lukas Tönne commented

2023-08-17 10:37:07 +02:00

Using a simple static const bNodeSocketStaticTypeInfo node_socket_subtypes[] now.
TIL C++ range-based loops actually also work for such plain arrays :)
https://stackoverflow.com/questions/7939399/how-does-the-range-based-for-work-for-plain-arrays

Using a simple `static const bNodeSocketStaticTypeInfo node_socket_subtypes[]` now. TIL C++ range-based loops actually also work for such plain arrays :) https://stackoverflow.com/questions/7939399/how-does-the-range-based-for-work-for-plain-arrays

LukasTonne marked this conversation as resolved

Lukas Tönne added 3 commits 2023-08-17 10:20:59 +02:00

72baa882f9 Merge branch 'main' into node-panels-rna

aa28a7dfa6 Fixed changed header names after merge.

3bb144060e Cleanup: Use "Note" instead of "XXX" for temporary comments.

Lukas Tönne added 1 commit 2023-08-17 10:32:58 +02:00

f89c86abe9 Use a simple static array for socket subtype names to avoid allocation.

Lukas Tönne added 1 commit 2023-08-17 10:34:56 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

8275674cd8 Cleanup: nullptr instead of NULL.

Lukas Tönne commented

2023-08-17 11:34:17 +02:00

@blender-bot build

Hans Goudey approved these changes 2023-08-19 23:24:52 +02:00

Hans Goudey left a comment

I find the way this is split from the other change confusing, with old code looking new in the diff, and new code commented out. But I don't have a good enough understanding to suggest a better way to organize things, and the next step will come soon anyway I suppose.

Only idea I'd have is possibly removing or re-adding some code in a separate commit.

I find the way this is split from the other change confusing, with old code looking new in the diff, and new code commented out. But I don't have a good enough understanding to suggest a better way to organize things, and the next step will come soon anyway I suppose. Only idea I'd have is possibly removing or re-adding some code in a separate commit.

source/blender/makesrna/intern/rna_node_tree_interface.cc

						
				@ -0,0 +32,4 @@

				#  include "WM_api.hh"

				/* Internal RNA function declarations, used to invoke registered callbacks. */

				extern "C" {

Hans Goudey commented

2023-08-19 23:13:19 +02:00

Do these have to be declared with "extern C" still?

Lukas Tönne commented

2023-08-21 10:47:37 +02:00

Correct, this extern "C" doesn't seem to be needed any more. makesrna generates .cc files now with regular C++ linkage for these functions (and for some reason declares these functions as extern but defines them in the same file anyway). Nodes RNA seems to be the last place using extern "C" declarations for registerable function implementations, will fix those. It probably works either way because these are free functions at root namespace level.

Correct, this `extern "C"` doesn't seem to be needed any more. makesrna generates .cc files now with regular C++ linkage for these functions (and for some reason declares these functions as `extern` but defines them in the same file anyway). Nodes RNA seems to be the last place using extern "C" declarations for registerable function implementations, will fix those. It probably works either way because these are free functions at root namespace level.

LukasTonne marked this conversation as resolved

source/blender/makesrna/intern/rna_node_tree_interface.cc

						
				@ -0,0 +59,4 @@

				      if (socket_typeinfo && socket_typeinfo->ext_interface.srna) {

				        return socket_typeinfo->ext_interface.srna;

				      }

				      else {

Hans Goudey commented

2023-08-19 23:12:36 +02:00

else after return

LukasTonne marked this conversation as resolved

Lukas Tönne added 3 commits 2023-08-21 10:54:46 +02:00

d412ba7213 Merge branch 'main' into node-panels-rna

8f3a01b9cf Removed unnecessary `extern "C"` from RNA function declarations.

Makesrna now generates fully C++ code, the "C" linkage declaration is
no longer necessary.

buildbot/vexp-code-patch-coordinator Build done. Details

9ba141b0b3 Cleanup: removed else after return.

Lukas Tönne commented

2023-08-21 11:03:18 +02:00

@blender-bot build

Lukas Tönne added 1 commit 2023-08-21 13:02:55 +02:00

buildbot/vexp-code-patch-coordinator Build done. Details

4f73c68cc6 Merge branch 'main' into node-panels-rna

Lukas Tönne commented

2023-08-21 13:03:09 +02:00

@blender-bot build

Lukas Tönne merged commit ea3d85b9d7 into main

2023-08-21 15:40:27 +02:00

Lukas Tönne referenced this issue from a commit

2023-08-21 15:40:27 +02:00

Node panels: RNA for node group interfaces

Lukas Tönne deleted branch node-panels-rna

2023-08-21 15:40:29 +02:00

Lukas Tönne referenced this pull request

2023-08-21 16:04:34 +02:00

Nodes: Panels integration with blend files and UI #111348

Lukas Tönne referenced this issue from a commit

2023-08-30 12:37:30 +02:00

Nodes: Panels integration with blend files and UI

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Node panels: RNA for node group interfaces #110952