Node panels: New DNA and C++ API for node group interfaces #110885

Merged
Lukas Tönne merged 24 commits from LukasTonne/blender:node-panels-dna into main 2023-08-09 10:06:40 +02:00
Member

Part 1/3 of #109135, #110272

Adds a new DNA structure for defining node group interfaces without
using bNodeSocket and with additional node UI item types.

Node group interfaces are organized as a hierarchy of "items", which
can be sockets or panels. Panels can contain both sockets and other
panels (although nested panels beyond the root panel may be disabled to
avoid complexity on the user level).

Sockets can be added to the interface in any order, not just the
conventional outputs..inputs order. Sockets can be marked as both input
and output, generating 2 sockets on node instances.

The C++ API in the DNA struct allows manipulating the interface
declaration by adding and removing items, moving them inside the
interface or into a different panel.

Part 1/3 of #109135, #110272 Adds a new DNA structure for defining node group interfaces without using `bNodeSocket` and with additional node UI item types. Node group interfaces are organized as a hierarchy of "items", which can be sockets or panels. Panels can contain both sockets and other panels (although nested panels beyond the root panel may be disabled to avoid complexity on the user level). Sockets can be added to the interface in any order, not just the conventional outputs..inputs order. Sockets can be marked as both input and output, generating 2 sockets on node instances. The C++ API in the DNA struct allows manipulating the interface declaration by adding and removing items, moving them inside the interface or into a different panel.
Lukas Tönne added 1 commit 2023-08-07 12:28:41 +02:00
2a2ca32626 Node panels: New DNA and C++ API for node group interfaces.
Part 1/3 of #109135, #110272

Adds a new DNA structure for defining node group interfaces without
using `bNodeSocket` and with additional node UI item types.

Node group interfaces are organized as a hierarchy of "items", which
can be sockets or panels. Panels can contain both sockets and other
panels (although nested panels beyond the root panel may be disabled to
avoid complexity on the user level).

Sockets can be added to the interface in any order, not just the
conventional outputs..inputs order. Sockets can be marked as both input
and output, generating 2 sockets on node instances.

The C++ API in the DNA struct allows manipulating the interface
declaration by adding and removing items, moving them inside the
interface or into a different panel.
Lukas Tönne added this to the Nodes & Physics project 2023-08-07 12:29:02 +02:00
Lukas Tönne requested review from Hans Goudey 2023-08-07 12:29:08 +02:00
Lukas Tönne requested review from Jacques Lucke 2023-08-07 12:29:16 +02:00

On the design level this seems fine to me, will leave others to do line by line review.

On the design level this seems fine to me, will leave others to do line by line review.
Jacques Lucke requested changes 2023-08-07 16:21:57 +02:00
@ -0,0 +32,4 @@
} // namespace blender::bke
inline bNodeTreeInterfaceItem *bNodeTreeInterface::active_item()
Member

This and some other functions probably don't need to be inline.

This and some other functions probably don't need to be `inline`.
LukasTonne marked this conversation as resolved
@ -0,0 +83,4 @@
bool match = false;
switch (item.item_type) {
case NODE_INTERFACE_SOCKET: {
match |= std::is_same<T, bNodeTreeInterfaceSocket>::value;
Member

use _v instead of ::value

use `_v` instead of `::value`
LukasTonne marked this conversation as resolved
@ -0,0 +273,4 @@
#endif
#ifdef __cplusplus
Member

No need for extern "C".

No need for `extern "C"`.
LukasTonne marked this conversation as resolved
@ -0,0 +52,4 @@
/** \name ID user increment in socket data
* \{ */
[[maybe_unused]] static void socket_data_id_user_increment(bNodeSocketValueObject &data)
Member

validate that maybe_unused is necessary

validate that `maybe_unused` is necessary
LukasTonne marked this conversation as resolved
@ -0,0 +508,4 @@
BLI_assert(src_socket.socket_type != nullptr);
dst_socket.name = BLI_strdup(src_socket.name);
dst_socket.description = src_socket.description ? BLI_strdup(src_socket.description) :
Member

Add a utility like BLI_strdup_null.

Add a utility like `BLI_strdup_null`.
Author
Member

In addition to adding BLI_strdup_null, would it be a good idea to put an assert in BLI_strdup to ensure it does not get called with a nullptr?

In addition to adding `BLI_strdup_null`, would it be a good idea to put an assert in `BLI_strdup` to ensure it does not get called with a nullptr?
Member

Yes

Yes
Author
Member

Unfortunately this creates a compiler warning because there is already a ATTR_NONNULL declaration for the input string. So the compiler assumes that users are diligent about the str input.

Unfortunately this creates a compiler warning because there is already a `ATTR_NONNULL` declaration for the input string. So the compiler assumes that users are diligent about the `str` input.
LukasTonne marked this conversation as resolved
@ -0,0 +1016,4 @@
if (include_self && fn(this->item) == false) {
return;
}
stack.push(items());
Member

Use this-> for methods.

Use `this->` for methods.
LukasTonne marked this conversation as resolved
@ -0,0 +1099,4 @@
new_socket->uid = uid;
new_socket->item.item_type = NODE_INTERFACE_SOCKET;
new_socket->name = BLI_strdup(name.data());
new_socket->description = description.data() ? BLI_strdup(description.data()) : nullptr;
Member

Don't use StringRef.data() to get a c string, because it may not be null terminated. c_str is for c strings. Same below.

Don't use `StringRef.data()` to get a c string, because it may not be null terminated. `c_str` is for c strings. Same below.
Author
Member

StringRef does not have c_str. Or was this referring to something else? Gitea seems to have lost track of what this comment refers to ...

Should i be using StringRefNull for anything that needs access to C-style const char *? (so basically everything since we're talking to DNA a lot)

`StringRef` does not have c_str. Or was this referring to something else? Gitea seems to have lost track of what this comment refers to ... Should i be using `StringRefNull` for anything that needs access to C-style `const char *`? (so basically everything since we're talking to DNA a lot)
Member

Your choice, either use StringRefNull or BLI_strdupn.

Your choice, either use `StringRefNull` or `BLI_strdupn`.
LukasTonne marked this conversation as resolved
@ -0,0 +35,4 @@
struct uiLayout;
/** Type of interface item. */
typedef enum eNodeTreeInterfaceItemType {
Member

Remove e prefix.

Remove `e` prefix.
LukasTonne marked this conversation as resolved
@ -0,0 +70,4 @@
char *default_attribute_name;
/* Unique id for constructing socket identifiers. */
int uid;
Member

identifier

`identifier`
LukasTonne marked this conversation as resolved
@ -0,0 +72,4 @@
/* Unique id for constructing socket identifiers. */
int uid;
char _pad[4];
/* String identifier for backwards compatibility, not used for new sockets. */
Member

Give an example for the compat string.

Give an example for the compat string.
LukasTonne marked this conversation as resolved
@ -0,0 +73,4 @@
int uid;
char _pad[4];
/* String identifier for backwards compatibility, not used for new sockets. */
char *uid_compat;
Member

identifier_compat
Maybe consider just using the string as identifier to avoid having two identifiers.

`identifier_compat` Maybe consider just using the string as identifier to avoid having two identifiers.
Author
Member

Yeah i'm just using a string identifier now, for the sake of simplicity and compatibility.

Yeah i'm just using a string identifier now, for the sake of simplicity and compatibility.
LukasTonne marked this conversation as resolved
@ -0,0 +75,4 @@
/* String identifier for backwards compatibility, not used for new sockets. */
char *uid_compat;
void *socket_data;
Member

Add comment for what data can be expected here. An example to a struct that this points to would be helpful.

Add comment for what data can be expected here. An example to a struct that this points to would be helpful.
LukasTonne marked this conversation as resolved
@ -0,0 +77,4 @@
void *socket_data;
IDProperty *prop;
Member

properties
Add comment.

`properties` Add comment.
LukasTonne marked this conversation as resolved
@ -0,0 +84,4 @@
bNodeSocketType *socket_typeinfo() const;
blender::ColorGeometry4f socket_color() const;
bool set_socket_type(const char *new_socket_type);
Member

Add example string that can be passed into the function.

Add example string that can be passed into the function.
LukasTonne marked this conversation as resolved
@ -0,0 +86,4 @@
bool set_socket_type(const char *new_socket_type);
void init_from_socket_instance(const bNodeSocket *socket);
Member

Comment would be nice, like does this overwrite the current type or can this only be called once?

Comment would be nice, like does this overwrite the current type or can this only be called once?
LukasTonne marked this conversation as resolved
@ -0,0 +93,4 @@
typedef struct bNodeTreeInterfacePanel {
bNodeTreeInterfaceItem item;
char *name;
Member

Comment, identifier or ui name?

Comment, identifier or ui name?
Author
Member

Which field does this refer to?

Which field does this refer to?
Author
Member

I'm guessing bNodeTreeInterfacePanel::name

I'm guessing bNodeTreeInterfacePanel::name
LukasTonne marked this conversation as resolved
@ -0,0 +115,4 @@
/**
* Check if the item is a direct child of the panel.
*/
bool contains_item(const bNodeTreeInterfaceItem &item) const;
Member

contains

`contains`
LukasTonne marked this conversation as resolved
@ -0,0 +121,4 @@
* Search for an item in the interface.
* \return True if the item was found.
*/
bool find_item(const bNodeTreeInterfaceItem &item) const;
Member

contains_recursive, maybe consider using recursive in more method names.

`contains_recursive`, maybe consider using `recursive` in more method names.
LukasTonne marked this conversation as resolved
@ -0,0 +136,4 @@
* \param r_parent: Parent containing the item.
* \return True if the item was found in any panel.
*/
bool find_item_parent(const bNodeTreeInterfaceItem &item, bNodeTreeInterfacePanel *&r_parent);
Member

The behavior could be made more obvious, and the parent can be returned as pointer.

The behavior could be made more obvious, and the parent can be returned as pointer.
LukasTonne marked this conversation as resolved
@ -0,0 +138,4 @@
*/
bool find_item_parent(const bNodeTreeInterfaceItem &item, bNodeTreeInterfacePanel *&r_parent);
void copy_items(blender::Span<const bNodeTreeInterfaceItem *> items_src, int flag);
Member

maybe copy_from? extend or replace?

maybe `copy_from`? extend or replace?
LukasTonne marked this conversation as resolved
@ -0,0 +141,4 @@
void copy_items(blender::Span<const bNodeTreeInterfaceItem *> items_src, int flag);
void clear_items(bool do_id_user);
void add_item(bNodeTreeInterfaceItem &item);
Member

Needs a comment for ownership handling.

Needs a comment for ownership handling.
LukasTonne marked this conversation as resolved
@ -0,0 +151,4 @@
* \note: The items are visited in drawing order from top to bottom.
*
* \param fn: Function to execute for each item, iterations stops if false is returned.
* \param include_root: Include the panel itself in the iteration.
Member

include_root -> include_self?

`include_root` -> `include_self`?
Author
Member

But "self" would be the interface, not a panel

But "self" would be the interface, not a panel
Member

I wrote that comment because the parameter name and the name in the comment are out of sync.

I wrote that comment because the parameter name and the name in the comment are out of sync.
Author
Member

Oh, got it, fixed.

Oh, got it, fixed.
LukasTonne marked this conversation as resolved
@ -0,0 +156,4 @@
void foreach_item(blender::FunctionRef<bool(bNodeTreeInterfaceItem &item)> fn,
bool include_self = false);
/**
* Apply a function to every item in the panel, including child panels.
Member

Same as above but const.

`Same as above but const.`
LukasTonne marked this conversation as resolved
@ -0,0 +170,4 @@
typedef struct bNodeTreeInterface {
bNodeTreeInterfacePanel root_panel;
int active_index;
Member

Comment for the kind of index (global, local)

Comment for the kind of index (global, local)
LukasTonne marked this conversation as resolved
@ -0,0 +174,4 @@
int next_uid;
#ifdef __cplusplus
void copy_data(const bNodeTreeInterface &src, int flag);
Member

What is the flag? does it extend or replace?

What is the flag? does it extend or replace?
LukasTonne marked this conversation as resolved
@ -0,0 +175,4 @@
#ifdef __cplusplus
void copy_data(const bNodeTreeInterface &src, int flag);
void free_data();
Member

What is data here, all the items? Maybe clear is a better name?

What is data here, all the items? Maybe `clear` is a better name?
Author
Member

I'd like to stick to the BKE naming scheme for freeing/copying ID datablocks here

I'd like to stick to the BKE naming scheme for freeing/copying ID datablocks here
LukasTonne marked this conversation as resolved
Lukas Tönne added 1 commit 2023-08-07 18:00:27 +02:00
Lukas Tönne added 1 commit 2023-08-07 18:10:00 +02:00
Lukas Tönne added 2 commits 2023-08-07 18:20:32 +02:00
Lukas Tönne added 2 commits 2023-08-08 11:04:58 +02:00
c4db8dd7bb Use template specialization for socket type implementation functions.
This avoids the need for [[maybe_unused]] attributes for silencing
compiler warnings.
Lukas Tönne added 1 commit 2023-08-08 11:35:39 +02:00
7fac55e690 Added BLI_strdup_null function that handles nullptr gracefully.
The BLI_strdup function requires a non-null string, which adds a bunch
of extra null checks for every call.
Lukas Tönne added 1 commit 2023-08-08 11:56:27 +02:00
Lukas Tönne added 1 commit 2023-08-08 12:22:44 +02:00
Lukas Tönne added 1 commit 2023-08-08 13:08:33 +02:00
ac5f63189a Use string identifiers for interface sockets for compatibility.
A simple integer would be better here, but we have to keep supporting
old sockets.
Lukas Tönne added 1 commit 2023-08-08 14:33:47 +02:00
Lukas Tönne reviewed 2023-08-08 14:55:07 +02:00
@ -0,0 +697,4 @@
return nodeSocketTypeFind(socket_type);
}
blender::ColorGeometry4f bNodeTreeInterfaceSocket::socket_color() const
Author
Member

As concluded in #109288 the draw_color callback should now support nullptr/None for both the context and the bNode pointer. This is technically a breaking change (note added to #110272) but very unlikely to cause trouble since very few addons implement interface classes and even fewer would have context-based colors.

As concluded in #109288 the `draw_color` callback should now support nullptr/None for both the context and the bNode pointer. This is technically a breaking change (note added to #110272) but very unlikely to cause trouble since very few addons implement interface classes and even fewer would have context-based colors.
LukasTonne marked this conversation as resolved
Author
Member

Wrt StringRefNull (can't find the comment any more):
Problem is that this thing does not accept nullptr (or rather: accepts it and asserts internally). That just puts the burden of null checks on the caller. Not a great help when passing around raw DNA char pointers.

I'll go back to using StringRef and checking nullptr internally, at least then users don't have to babysit every single DNA string.

Wrt `StringRefNull` (can't find the comment any more): Problem is that this thing does not accept nullptr (or rather: accepts it and asserts internally). That just puts the burden of null checks on the caller. Not a great help when passing around raw DNA char pointers. I'll go back to using StringRef and checking nullptr internally, at least then users don't have to babysit every single DNA string.

@LukasTonne The null in the name indicates the end-of-line terminal. It's still a wrapper for real strings only.

@LukasTonne The null in the name indicates the end-of-line terminal. It's still a wrapper for real strings only.
Jacques Lucke approved these changes 2023-08-08 15:28:23 +02:00
Jacques Lucke left a comment
Member

Generally seems fine now, if issues come up, they can also be fixed in main. This is not exposed to the user at all yet, so the risk of committing this is fairly low. We just have to make sure that we test the other patches that use the new data structures well.

Generally seems fine now, if issues come up, they can also be fixed in `main`. This is not exposed to the user at all yet, so the risk of committing this is fairly low. We just have to make sure that we test the other patches that use the new data structures well.
Author
Member

The null in the name indicates the end-of-line terminal. It's still a wrapper for real strings only.

@mod_moder I switched to StringRefNull because that guarantees a null-terminated string and we wouldn't have to check internally if a const char * is null. However, now the user instead has to do this check before calling the API and there is nothing enforcing non-null pointers here (like ATTR_NONNULL to at least have a warning).

> The null in the name indicates the end-of-line terminal. It's still a wrapper for real strings only. @mod_moder I switched to `StringRefNull` because that guarantees a null-terminated string and we wouldn't have to check internally if a `const char *` is null. However, now the user instead has to do this check before calling the API and there is nothing enforcing non-null pointers here (like `ATTR_NONNULL` to at least have a warning).
Hans Goudey changed title from Node panels: New DNA and C++ API for node group interfaces. to Node panels: New DNA and C++ API for node group interfaces 2023-08-08 15:53:34 +02:00
Hans Goudey reviewed 2023-08-08 16:04:53 +02:00
@ -0,0 +9,4 @@
#include "BKE_node.h"
#ifdef __cplusplus
Member

A C++ header shouldn't need these #ifdef blocks, since it likely isn't (and shouldn't be) included in any C files.

A C++ header shouldn't need these `#ifdef` blocks, since it likely isn't (and shouldn't be) included in any C files.
LukasTonne marked this conversation as resolved
@ -0,0 +22,4 @@
namespace blender::bke {
/* Runtime topology cache for linear access to items. */
typedef struct bNodeTreeInterfaceCache {
Member
typedef struct bNodeTreeInterfaceCache {
} bNodeTreeInterfaceCache;

->

struct bNodeTreeInterfaceCache {
};

That's all you need to define a struct in C++

``` typedef struct bNodeTreeInterfaceCache { } bNodeTreeInterfaceCache; ``` -> ``` struct bNodeTreeInterfaceCache { }; ``` That's all you need to define a struct in C++
LukasTonne marked this conversation as resolved
@ -0,0 +183,4 @@
}
} // namespace blender::bke::node_interface
Member

Probably isn't necessary to close the namespace and open it again?

Probably isn't necessary to close the namespace and open it again?
LukasTonne marked this conversation as resolved
@ -0,0 +40,4 @@
return nullptr;
}
/* For builtin socket types only the base type is supported. */
if (blender::bke::nodeIsStaticSocketType(typeinfo)) {
Member

blender::bke:: should be unnecessary here, since we're already inside that namespace

`blender::bke::` should be unnecessary here, since we're already inside that namespace
LukasTonne marked this conversation as resolved
@ -0,0 +43,4 @@
if (blender::bke::nodeIsStaticSocketType(typeinfo)) {
return nodeStaticSocketType(typeinfo->type, PROP_NONE);
}
else {
Member

else after return is unnecessary

else after return is unnecessary
LukasTonne marked this conversation as resolved
@ -0,0 +127,4 @@
{
data.value = false;
}
template<> void socket_data_init_impl(bNodeSocketValueRotation & /*data*/) {}
Member

I would think that rotation values would need zero initialization if the boolean value does

I would think that rotation values would need zero initialization if the boolean value does
Author
Member

I copied the init code from existing node_socket.cc, which also does nothing here. The socket values are zero-initialized, so that would be fine.

I copied the init code from existing node_socket.cc, which also [does nothing here](https://projects.blender.org/blender/blender/src/commit/e1b1b2a8b4a5e25d1eb9f78adab2924dd613e79b/source/blender/nodes/intern/node_socket.cc#L346). The socket values are zero-initialized, so that would be fine.
@ -0,0 +330,4 @@
/** \} */
/* -------------------------------------------------------------------- */
/** \name Read ID pointer data
Member

Title case for doxygen sections

Title case for doxygen sections
Author
Member

fine ...

fine ...
Author
Member

"Blend File"? "blend File"? ".blend File"?

If we want to be pedantic ...

"Blend File"? "blend File"? ".blend File"? If we want to be pedantic ...
Author
Member

Sticking to "Blend File"

Sticking to "Blend File"
LukasTonne marked this conversation as resolved
@ -0,0 +810,4 @@
bool found = false;
/* Have to capture item address here instead of just a reference,
* otherwise pointer comparison will not work. */
this->foreach_item([&item, &index, &found](const bNodeTreeInterfaceItem &titem) {
Member

A simple capture of [&] should work fine, since the lambda is just executed here.

A simple capture of `[&]` should work fine, since the lambda is just executed here.
LukasTonne marked this conversation as resolved
Lukas Tönne added 4 commits 2023-08-08 16:06:45 +02:00
Lukas Tönne added 1 commit 2023-08-08 16:07:27 +02:00
Lukas Tönne added 7 commits 2023-08-08 16:32:08 +02:00
Author
Member

@blender-bot build

@blender-bot build
Author
Member

@blender-bot build

@blender-bot build
Lukas Tönne merged commit 090f365cbd into main 2023-08-09 10:06:40 +02:00
Lukas Tönne deleted branch node-panels-dna 2023-08-09 10:06:41 +02:00
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#110885
No description provided.