Nodes: Panels integration with blend files and UI #111348
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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
6 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#111348
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "LukasTonne/blender:node-panels-final"
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 3/3 of #109135, #110272
Switch to new node group interfaces and deprecate old DNA and API.
This completes support for panels in node drawing and in node group
interface declarations in particular.
The new node group interface DNA and RNA code has been added in parts
1 and 2 (#110885, #110952) but has not be enabled yet. This commit
completes the integration by
All places accessing node group interface declarations should now be
using the new API. A runtime cache has been added that allows simple
linear access to socket inputs and outputs even when a panel hierarchy
is used.
Old DNA has been deprecated and should only be accessed for versioning
(inputs/outputs renamed to inputs_legacy/outputs_legacy to catch
errors). Versioning code ensures both backward and forward
compatibility of existing files.
The API for old interfaces is removed. The new API is very similar but
is defined on the
ntree.interface
instead of thentree
directly.Breaking change notifications and detailed instructions for migrating
will be added.
A python test has been added for the node group API functions. This
includes new functionality such as creating panels and moving items
between different levels.
This patch does not yet contain panel representations in the modifier
UI. This has been tested in a separate branch and will be added with a
later PR (#108565).
⚠️
script_load_addons
test fails because of a required change inNode Wrangler addon: #104849.
The addon fixes needs to be completed in sync with this PR.
@blender-bot build
Didn't compile and test yet, but here are some initial comments. It would also be nice to have a couple screenshots in the PR description.
@ -55,0 +66,4 @@
layout.prop(self, "randomize")
def init_socket(self, node, socket, data_path):
print("I am doing the socket thing")
This can probably describe the purpose of the method a bit better :P
I've simplified the custom_nodes.py template for sockets a bit to show the actual functionality without being too convoluted. Custom socket type now has a simple
input_value
properties whose default is defined by its interface class with adefault_value
property.@ -219,0 +223,4 @@
* node tree hasn't been drawn yet. In the node tree's "world space" (the same as
* #bNode::runtime::totr).
*/
float2 location;
It seems like this could just be a float-- the vertical position of the panel, right? If so, it might be nice to justs store them in an array.
This is ancient drawing code, the panel is just a simplified version of sockets (see
bNodeSocketRuntime
above). I'd rather not add new code here unless necessary, this can be changed later. The location may also include horizontal offsets like indentation.@ -206,1 +201,3 @@
ntree_dst->tree_interface.copy_data(ntree_src->tree_interface, flag);
/* Legacy inputs/outputs lists may contain unmanaged pointers, don't copy these. */
BLI_listbase_clear(&ntree_dst->inputs_legacy);
Should be possible to remove these, right? Except after versioning and file writing, these lists should be empty I'd think.
@ -301,1 +296,3 @@
MEM_freeN(sock);
ntree->tree_interface.free_data();
/* Free legacy interface data */
LISTBASE_FOREACH_MUTABLE (bNodeSocket *, socket, &ntree->inputs_legacy) {
Same here, I'd think these shouldn't be around for most of the time-- only while converting old files and writing in the old format.
@ -473,1 +469,4 @@
namespace blender::bke::forward_compat {
static void write_node_socket_interface(BlendWriter *writer, bNodeSocket *sock)
const socket pointer?
Yeah, old legacy code ...
@ -3936,3 +3870,3 @@
static void update_socket_declarations(ListBase *sockets,
Span<blender::nodes::SocketDeclarationPtr> declarations)
Span<blender::nodes::SocketDeclaration *> declarations)
Hmm, doesn't seem obvious to me to change
SocketDeclarationPtr
to a raw pointer here, what's the reason for that?Previously the node declaration stored a
Vector<SocketDeclarationPtr>
which could be passed straight to this function.Now the node declaration is more complex, it can contain panel declarations as well as interleaved input/output sockets. For that reason the node declaration additionally stores two plain
Vector<SocketDeclaration *>
where the socket declarations are owned by the mainitems
vector (usingunique_ptr
).https://projects.blender.org/LukasTonne/blender/src/branch/node-panels-final/source/blender/nodes/NOD_node_declaration.hh#L533-L539
@ -29,3 +29,2 @@
bNodeTreeRuntime &tree_runtime = *ntree.runtime;
tree_runtime.interface_inputs = ntree.inputs;
tree_runtime.interface_outputs = ntree.outputs;
tree_runtime.interface_cache.rebuild(const_cast<bNodeTreeInterface &>(ntree.tree_interface));
A comment justifying the const cast would be nice here.
comment added
@ -577,2 +607,4 @@
FOREACH_NODETREE_END;
/* Convert old socket lists into new interface items. */
FOREACH_NODETREE_BEGIN (bmain, ntree, id) {
Looks like this should be at the bottom of
blo_do_versions_400
, in a new check for the file version.Another merge fuckup i guess ...
@ -0,0 +156,4 @@
std::unique_ptr<TreeViewItemDropTarget> create_drop_target() override;
private:
bNodeTree &nodetree_;
Style guide mentions that member variables should be above functions in a class declaration. (helpful to see that it's easy to see what the class actually stores)
@ -1564,3 +1581,3 @@
GPU_matrix_push();
/* The draw manager is used to draw the backdrop image. */
/* The draw manager is used to draw the
I think these formatting changes could be avoided.
Not sure where these changes are coming from, pretty sure i didn't add them deliberately and make format doesn't care either way. Could be some weird merge artifact ...
@ -462,0 +489,4 @@
/* Parent panel stack with count of items still to be added. */
struct PanelUpdate {
int remaining_items;
bool is_collapsed;
I don't have a great understanding of why a stack is necessary here. Maybe a brief high-level description of the process would be helpful here.
Added a comment.
Could also do this with a recursive function, just a matter of preference.
Unnecessary formatting changes?
Gitea lost track of the code here, still relevant?
Went over all the unwanted formatting changes, should be ok.
@ -1629,6 +1753,130 @@ static void node_draw_sockets(const View2D &v2d,
}
C++ style casts here
@ -896,2 +915,4 @@
RNA_def_property_clear_flag(prop, PROP_EDITABLE);
RNA_def_property_ui_text(prop, "Items", "Items in the node panel");
prop = RNA_def_property(srna, "is_child_panel_allowed", PROP_BOOLEAN, PROP_NONE);
Does this need to be exposed in RNA yet? Especially if it's not editable, I think it could just be hardcoded for now unless there's a reason not to.
Not sure, just felt right to give users some way to tell if adding a panel is allowed or not. Could leave it out and just rely on the API method returning
None
(and printing a warning).@ -935,3 +960,3 @@
"Socket Type",
"Type of socket generated on nodes");
/* Note: itemf callback works for the function parameter, it does not require a data pointer. */
/* Note: itemf callback works for the function parameter, it does not require a data pointer.
Unnecessary formatting change
is_child_panel_allowed
property. 623c92ff74Found one more issue with custom socket types (we should turn the custom_nodes.py script into a test!):
The
typeinfo->draw_color
callback now officially can be called with a null socket pointer, basically acting like a static method with optional contextual data.However, registerable python callbacks work by storing the actual callable
PyObject
in theExtensionRNA
inside thebNodeSocketType
. When C++ code calls the callback it then needs a way too find thatbNodeSocketType
to get to the actual py callable. Without a socket pointer it has no typeinfo currently.I'll add a
bNodeSocketType
argument to that callback to make it a classmethod. This is redundant when a socket is specified, but the socket is optional and the typeinfo is not.Aaaand unfortunately this also does not work: The callback is a full member of the
NodeSocket
class and as such cannot be called without a valid socket pointer. I don't thinkstaticmethod
/classmethod
are supported as registerable python methods. That means we will always need a socket instance first before we can get its color from python.Think we're going to need the simple
base_color
here to make this work #109288I focused on testing the UI this time, didn't look too much at the code.
The commit message should be clear that this doesn't include the changes to the modifier UI, especially since it's labeled 3/3 of the design.
I think we should probably talk about the UI in a meeting this week, to make sure we're on the same page now. I left some initial comments below.
This UI doesn't exactly look finished:
@ -1872,2 +1872,4 @@
return;
}
ntree->ensure_topology_cache();
Not sure about this here TBH, it probably makes more sense to put this directly above the code that actually needs the topology cache.
@ -88,3 +88,3 @@
bool supports_renaming() const override;
bool rename(StringRefNull new_name) override;
bool rename(const bContext &C, StringRefNull new_name) override;
Maybe the new context argument could be added in a separate commit? That would help to make this diff smaller.
#111522
@ -876,3 +863,1 @@
class NodeTreeInterfacePanel(Panel):
class NODE_PT_node_tree_declaration(Panel):
declaration
->interface
?same for
template_node_tree_declaration
maybeNodeSocketVectorTranslation
(I connected the offset input of the Set Position node to the Group Input).@ -876,3 +863,1 @@
class NodeTreeInterfacePanel(Panel):
class NODE_PT_node_tree_declaration(Panel):
declaration
->interface
?@ -204,6 +204,8 @@ static AnonymousAttributeInferencingResult analyse_anonymous_attribute_usages(
{
BLI_assert(!tree.has_available_link_cycle());
const bNodeTreeInterfaceCache &cache = tree.interface_cache();
cache
->interface_cache
, because justcache
seems to be a bit too generic here.@ -850,1 +855,4 @@
{
/* Are child panels allowed? */
BLI_assert(item.item_type != NODE_INTERFACE_PANEL ||
(flag & NODE_INTERFACE_PANEL_ALLOW_CHILD_PANELS));
Use
this->flag
. Reading the code like this I expected theflag
to be an input parameter.@ -107,2 +116,4 @@
/* UI name of the panel. */
char *name;
char *description;
/* eNodeTreeInterfacePanelFlag */
Remove
e
.@ -281,1 +296,3 @@
bNodeTreeInterfacePanel *add_panel(blender::StringRefNull name, bNodeTreeInterfacePanel *parent);
bNodeTreeInterfacePanel *add_panel(blender::StringRefNull name,
blender::StringRefNull description,
const NodeTreeInterfacePanelFlag flag,
Unnecessary
const
in declaration.@ -309,0 +319,4 @@
typedef struct bNodePanelState {
/* Unique identifier for validating state against panels in node declaration. */
short uid;
use
int16_t
instead ofshort
Changed to int for consistency with
PanelDeclaration
andbNodeTreeInterfacePanel
where this identifier is copied from. Also using "identifier" consistently now instead of "uid".Thanks for the reviews, i know it's a lot of code.
The mockups we had were all fairly basic and not true to color.
#108895
Note that i can't currently cover this in the python test because there is no API of the panel state on node instances yet. The panel state is only weakly referencing the panel interface/declaration, it would not be straightforward to find what the panel name etc. is for a given state.
I thought we want to disable them because it's confusing and rarely needed. The DNA/RNA and tree view UI support it, but drawing would need some way to distinguish levels (current background drawing needs changes anyway). Again, the mockups and designs are not explicit about this.
Would have to duplicate all the code for initializing/copying socket default values. Ugly but possible, no strong opinion.
I'd rather disable having sockets between and after panels for the time being, as that seems more problematic, especially in the modifier. I'm somewhat ok with only having one level of panels for now if it makes things easier, but I'm pretty sure we want to lift that limitation soonish.
@blender-bot build
@blender-bot build
My bad, i didn't realize that, because the order in which declarations are added is now relevant, it changes the built-in node order. Since i don't suppose we want to update all the existing nodes, i suggest to add a declaration flag which makes existing built-in nodes use the old socket layouts by default. When we start adding panels and custom socket order to some node types we can reorder the socket declarations as needed.
Node panels: Integration with blend files and UIto WIP: Node panels: Integration with blend files and UIIssue here is inconsistency of "global index" vs. "position in panel". The
to_index
passed to the RNA function is supposed to be the global index, but is used as a local panel position.I'm going to make this consistent by always expecting a local panel position when moving items around. The API gets another utility method and RNA readonly property for the items position relative to its parent panel. IMO this is more useful in most situations and avoids getting into ambiguous situations where its not clear whether a global index or local position is required.
I wonder why we need the Up/Down arrows in the socket tree view at all if we have drag and drop. Currently, I find the behavior of these buttons fairly unpredictable when there are panels (and I also don't know what the right behavior should be in all cases).
FUNC_USE_SELF_ID
flag for the draw method, doesn't do anything. 16996a7c58I've added a
draw_color_simple
callback that works reliably without a context or socket pointer. Bothdraw_color
anddraw_color_simple
are optional, but at least one should be implemented by addons.@blender-bot build
After some fixes these buttons are more reliable now. They only move an item within its panel. I'd suggest we keep them around for now and decide later how the UI can be improved, to not drag out this PR any more than necessary. Perhaps a copy/paste mechanism is more suitable for such hierarchical structure? Drag & drop is also not ideal yet, the drop location feature is a bit fiddly and the target location isn't immediately obvious.
Great implementation so far. I gathered some suggestions. I will bring this list up to the next Nodes & Physics daily, since I want the buy-in from the rest of the team.
General changes:
Need input from UI team:
Is Input
,Is Output
could be replaced by an Enum (Input, Output, Both).@blender-bot build
is_input
/is_output
flags with an enum forin_out
flags. 93aa416fa6declaration
in node group ui template tointerface
. f4015e5b53interface_cache
to avoid ambiguity. de9a259181this->
prefix for flag to avoid ambiguity with parameters. f977a66909e
prefix has been removed from flag type names. a7aa5a9c8cdefault_closed
property for panels with "Closed by Default" text.@blender-bot build
WIP: Node panels: Integration with blend files and UIto Node panels: Integration with blend files and UI@blender-bot build
@ -423,0 +457,4 @@
MEM_SAFE_FREE(new_socket->default_attribute_name);
new_socket->default_attribute_name = BLI_strdup_null(socket->default_attribute_name);
node_socket_copy_default_value_data(
Even though it duplicates code, I think it might be better not to call the function from the nodes module in versioning code. The nodes module code is likely to change, and conceptually lives on top of
blenloader
-- so better to avoid the cyclic dependency I think.@ -1031,3 +1028,2 @@
for (bNode *node : nodes_to_move) {
for (bNodeSocket *input_socket : node->input_sockets()) {
for (bNodeLink *link : input_socket->directly_linked_links()) {
for (bNodeSocket *output_socket : node->output_sockets()) {
Any particular reason the inputs and outputs are reversed here? Before this diff, the inputs came first, then the outputs.
Because we want to retain the conventional outputs..inputs socket order when making a group.
#111348 (comment)
@ -545,6 +548,12 @@ inline blender::Span<bNestedNodeRef> bNodeTree::nested_node_refs_span() const
return {this->nested_node_refs, this->nested_node_refs_num};
}
inline const blender::bke::bNodeTreeInterfaceCache &bNodeTree::interface_cache() const
How about replacing this with three methods:
interface_inputs()
,interface_outputs()
andinterface_items()
. The fact that they're stored in the same cache isn't that helpful to the code that uses these spans.https://blender.chat/channel/nodes-physics-module?msg=autuWPzLXsGcCaZYa
done
I can't repro. Was this saved in the panels branch/main/3.6? What's supposed to be there? Can you attach the file?
To be clear: you mean remove the "Name: XXX" button below the tree view since it's already renameable through double-clicking?
can't repro this one either, seems to work for me. Any particular circumstances?
interface_cache
with getters for inputs, outputs, and items. c896561262Changed to "Input/Output Type" but this is an awkward label. Can be changed later if some native speaker has a better idea ("Kind"?)
Can do that, but just a remark: I dislike the way these special "geometry only" properties are tacked onto the generic socket drawing code. If geometry trees need special socket attributes they should have their own socket types. Yes, it raises issues of compatibility between tree types, and this draw order would require putting the general socket details in the same callback. Just would be nice to have a less leaky abstraction ... rant over
Added subtype properties to the float, int, vector socket types that support it. Previously we did this only with an operator, which had its own
subtype
property and we never had asubtype
property on the socket value structs themselves. That's because changing the subtype would have invalidated the resolved RNA struct type of the very thing owning the property, with unclear consequences. Now we are ignoring the subtype when determining the interface socket type, so this property is safe to use.@blender-bot build
Looking good! I think this will be the last time I request changes :)
@ -1001,0 +910,4 @@
if active_item.item_type == 'SOCKET':
layout.prop(active_item, "socket_type", text="Type")
layout.prop(active_item, "description")
layout.prop(active_item, "in_out", text="Input/Output Type")
Input/Output Type
->Input/Output
"Type" is implied with the enum dropdown.
@ -46,6 +46,8 @@
#include "ANIM_armature_iter.hh"
#include "ANIM_bone_collections.h"
#include "NOD_socket.hh"
Unused include now. Thanks
@ -583,6 +585,77 @@ static void version_replace_principled_hair_model(bNodeTree *ntree)
}
}
static void legacy_socket_move_to_interface(bNodeTreeInterfaceItem *&new_item_ptr,
A reference to a pointer is a bit weird here. Simpler to just return the item pointer IMO
@ -586,0 +646,4 @@
{
/* Clear legacy sockets after conversion.
* Internal data pointers have been moved or freed already. */
LISTBASE_FOREACH_MUTABLE (bNodeSocket *, sock, &ntree->inputs_legacy) {
Two calls to
BLI_freelistN
can replace this function@ -1879,6 +1879,7 @@ void DepsgraphNodeBuilder::build_nodetree(bNodeTree *ntree)
if (built_map_.checkIsBuiltAndTag(ntree)) {
return;
}
Unnecessary whitespace change
Again with these mysterious changes ... i suspect clang format glitch, will check the
make format
results a bit more closely next time.Your patch used to ensure the topology cache here, I expect when you moved that the extra new line remained.
@ -575,6 +623,44 @@ static void rna_NodeTreeInterfaceItems_move_to_parent(ID *id,
/* ******** Node Socket Subtypes ******** */
static const EnumPropertyItem *rna_subtype_filter_itemf(const blender::Set<int> subtypes,
Should be a const reference
Node panels: Integration with blend files and UIto Node: Panels integration with blend files and UINode: Panels integration with blend files and UIto Nodes: Panels integration with blend files and UIFound one potentially blocking issue. Other than that, looks good to me.
main
yet.@ -16,3 +16,2 @@
group = bpy.data.node_groups.new(name, 'GeometryNodeTree')
group.inputs.new('NodeSocketGeometry', data_("Geometry"))
group.outputs.new('NodeSocketGeometry', data_("Geometry"))
group.interface.new_socket(data_("Geometry"), in_out={'INPUT'}, socket_type='NodeSocketGeometry')
Change order.
@ -263,0 +310,4 @@
if active_item:
# Insert into active panel if possible, otherwise insert after active item.
if active_item.item_type == 'PANEL' and item.item_type != 'PANEL':
interface.move_to_parent(item, active_item, len(active_item.interface_items))
Not part of this patch, but might be reasonable to rename this to
move_to_panel
. Maybe keep a note somewhere.Added a note to "Future Tasks" in #110272. Can make tasks for some of these later.
@ -736,0 +748,4 @@
break;
}
if (items[test_pos]->item_type != NODE_INTERFACE_PANEL) {
/* Found valid position, insert after the last socket item */
Minor: missing dot in comment, also check the comment below.
@ -586,0 +613,4 @@
legacy_socket.prop = nullptr;
/* Unused data */
MEM_delete(legacy_socket.runtime);
Also set the pointer to null to avoid a dangling pointer.
@ -914,6 +971,29 @@ void blo_do_versions_400(FileData *fd, Library * /*lib*/, Main *bmain)
}
}
if (!MAIN_VERSION_FILE_ATLEAST(bmain, 400, 18)) {
Update the subversion.
I can't repro that on my laptop (win11, msvc build). Tried Release and RelWithDebInfo builds. Any more info in logs?
Hmmm, I was able to reproduce it reliably, but now I can't reproduce it anymore, so maybe forget about it for now.
@blender-bot build
@blender-bot build
Panels is a great feature, thanks. Also checkboxes in modifiers are great!
Will you add panels in modifier as well please?
And I think it would be great to have 3 buttons to add: input, output and panel instead one "+" button. One click is better than 2 clicks