Node panels: New DNA and C++ API for node group interfaces #110885
No reviewers
Labels
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 project
No Assignees
5 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#110885
Loading…
Reference in New Issue
No description provided.
Delete Branch "LukasTonne/blender:node-panels-dna"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
On the design level this seems fine to me, will leave others to do line by line review.
@ -0,0 +32,4 @@
} // namespace blender::bke
inline bNodeTreeInterfaceItem *bNodeTreeInterface::active_item()
This and some other functions probably don't need to be
inline
.@ -0,0 +83,4 @@
bool match = false;
switch (item.item_type) {
case NODE_INTERFACE_SOCKET: {
match |= std::is_same<T, bNodeTreeInterfaceSocket>::value;
use
_v
instead of::value
@ -0,0 +273,4 @@
#endif
#ifdef __cplusplus
No need for
extern "C"
.@ -0,0 +52,4 @@
/** \name ID user increment in socket data
* \{ */
[[maybe_unused]] static void socket_data_id_user_increment(bNodeSocketValueObject &data)
validate that
maybe_unused
is necessary@ -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) :
Add a utility like
BLI_strdup_null
.In addition to adding
BLI_strdup_null
, would it be a good idea to put an assert inBLI_strdup
to ensure it does not get called with a nullptr?Yes
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 thestr
input.@ -0,0 +1016,4 @@
if (include_self && fn(this->item) == false) {
return;
}
stack.push(items());
Use
this->
for methods.@ -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;
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.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-styleconst char *
? (so basically everything since we're talking to DNA a lot)Your choice, either use
StringRefNull
orBLI_strdupn
.@ -0,0 +35,4 @@
struct uiLayout;
/** Type of interface item. */
typedef enum eNodeTreeInterfaceItemType {
Remove
e
prefix.@ -0,0 +70,4 @@
char *default_attribute_name;
/* Unique id for constructing socket identifiers. */
int uid;
identifier
@ -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. */
Give an example for the compat string.
@ -0,0 +73,4 @@
int uid;
char _pad[4];
/* String identifier for backwards compatibility, not used for new sockets. */
char *uid_compat;
identifier_compat
Maybe consider just using the string as identifier to avoid having two identifiers.
Yeah i'm just using a string identifier now, for the sake of simplicity and compatibility.
@ -0,0 +75,4 @@
/* String identifier for backwards compatibility, not used for new sockets. */
char *uid_compat;
void *socket_data;
Add comment for what data can be expected here. An example to a struct that this points to would be helpful.
@ -0,0 +77,4 @@
void *socket_data;
IDProperty *prop;
properties
Add comment.
@ -0,0 +84,4 @@
bNodeSocketType *socket_typeinfo() const;
blender::ColorGeometry4f socket_color() const;
bool set_socket_type(const char *new_socket_type);
Add example string that can be passed into the function.
@ -0,0 +86,4 @@
bool set_socket_type(const char *new_socket_type);
void init_from_socket_instance(const bNodeSocket *socket);
Comment would be nice, like does this overwrite the current type or can this only be called once?
@ -0,0 +93,4 @@
typedef struct bNodeTreeInterfacePanel {
bNodeTreeInterfaceItem item;
char *name;
Comment, identifier or ui name?
Which field does this refer to?
I'm guessing bNodeTreeInterfacePanel::name
@ -0,0 +115,4 @@
/**
* Check if the item is a direct child of the panel.
*/
bool contains_item(const bNodeTreeInterfaceItem &item) const;
contains
@ -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;
contains_recursive
, maybe consider usingrecursive
in more method names.@ -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);
The behavior could be made more obvious, and the parent can be returned as pointer.
@ -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);
maybe
copy_from
? extend or replace?@ -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);
Needs a comment for ownership handling.
@ -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.
include_root
->include_self
?But "self" would be the interface, not a panel
I wrote that comment because the parameter name and the name in the comment are out of sync.
Oh, got it, fixed.
@ -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.
Same as above but const.
@ -0,0 +170,4 @@
typedef struct bNodeTreeInterface {
bNodeTreeInterfacePanel root_panel;
int active_index;
Comment for the kind of index (global, local)
@ -0,0 +174,4 @@
int next_uid;
#ifdef __cplusplus
void copy_data(const bNodeTreeInterface &src, int flag);
What is the flag? does it extend or replace?
@ -0,0 +175,4 @@
#ifdef __cplusplus
void copy_data(const bNodeTreeInterface &src, int flag);
void free_data();
What is data here, all the items? Maybe
clear
is a better name?I'd like to stick to the BKE naming scheme for freeing/copying ID datablocks here
@ -0,0 +697,4 @@
return nodeSocketTypeFind(socket_type);
}
blender::ColorGeometry4f bNodeTreeInterfaceSocket::socket_color() const
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.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.
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.@mod_moder I switched to
StringRefNull
because that guarantees a null-terminated string and we wouldn't have to check internally if aconst 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 (likeATTR_NONNULL
to at least have a warning).Node panels: New DNA and C++ API for node group interfaces.to Node panels: New DNA and C++ API for node group interfaces@ -0,0 +9,4 @@
#include "BKE_node.h"
#ifdef __cplusplus
A C++ header shouldn't need these
#ifdef
blocks, since it likely isn't (and shouldn't be) included in any C files.@ -0,0 +22,4 @@
namespace blender::bke {
/* Runtime topology cache for linear access to items. */
typedef struct bNodeTreeInterfaceCache {
->
That's all you need to define a struct in C++
@ -0,0 +183,4 @@
}
} // namespace blender::bke::node_interface
Probably isn't necessary to close the namespace and open it again?
@ -0,0 +40,4 @@
return nullptr;
}
/* For builtin socket types only the base type is supported. */
if (blender::bke::nodeIsStaticSocketType(typeinfo)) {
blender::bke::
should be unnecessary here, since we're already inside that namespace@ -0,0 +43,4 @@
if (blender::bke::nodeIsStaticSocketType(typeinfo)) {
return nodeStaticSocketType(typeinfo->type, PROP_NONE);
}
else {
else after return is unnecessary
@ -0,0 +127,4 @@
{
data.value = false;
}
template<> void socket_data_init_impl(bNodeSocketValueRotation & /*data*/) {}
I would think that rotation values would need zero initialization if the boolean value does
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.
@ -0,0 +330,4 @@
/** \} */
/* -------------------------------------------------------------------- */
/** \name Read ID pointer data
Title case for doxygen sections
fine ...
"Blend File"? "blend File"? ".blend File"?
If we want to be pedantic ...
Sticking to "Blend File"
@ -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) {
A simple capture of
[&]
should work fine, since the lambda is just executed here.@blender-bot build
@blender-bot build