Fix #115727: Draw panel buttons in the node editor side bar #116936

Lukas Tönne · 2024-01-09T12:32:05+01:00

Lukas Tönne commented

2024-01-09 12:32:05 +01:00

In 13fac109 the node panels got support for individual option button
callbacks, but these were not included in the node editor side bar.
Only the older top-level buttons are drawn there.

The panel structure is currently not accessible in python since it is
part of the NodeDeclaration system. To draw node input sockets and
buttons in the correct panel order as they appear on the node, a new
template function uiTemplateNodeInputs has been added. This iterates
over declared panels and their contents in the appropriate order and
draws the buttons before sockets in the same panel.

In 13fac109 the node panels got support for individual option button callbacks, but these were not included in the node editor side bar. Only the older top-level buttons are drawn there. The panel structure is currently not accessible in python since it is part of the `NodeDeclaration` system. To draw node input sockets and buttons in the correct panel order as they appear on the node, a new template function `uiTemplateNodeInputs` has been added. This iterates over declared panels and their contents in the appropriate order and draws the buttons before sockets in the same panel. ![Screenshot_20240115_144911](/attachments/1c9a4a0c-50e5-40e9-bfd7-619246b48fad)

Screenshot_20240115_144911.png

112 KiB

Pedro A. commented

2024-01-13 12:30:35 +01:00

As far I tested here after merge this changes in my local branch, this seems to break the backward compatibility with custom nodes since 'draw_buttons' and 'draw_buttons_ext' have now a new parameter

As far I tested here after merge this changes in my local branch, this seems to break the backward compatibility with custom nodes since 'draw_buttons' and 'draw_buttons_ext' have now a new parameter ![imagen](/attachments/cb0cd8e8-0aa3-43a6-b36a-8679181c0ec3)

imagen.png

244 KiB

Lukas Tönne commented

2024-01-15 10:32:01 +01:00

I probably have to move the panel code to C++ since we don't have access to the panel hierarchy from python. It's defined in the NodeDeclaration but those declarations have no python API yet. Adding such an API is beyond the scope of a bug fix.

I probably have to move the panel code to C++ since we don't have access to the panel hierarchy from python. It's defined in the `NodeDeclaration` but those declarations have no python API yet. Adding such an API is beyond the scope of a bug fix.

Lukas Tönne changed title from ~~Fix #115727: Draw panel buttons in the node editor side bar~~ to WIP Fix #115727: Draw panel buttons in the node editor side bar

2024-01-15 10:32:16 +01:00

Lukas Tönne force-pushed node-sidebar-panel-buttons from 590ac1b48c to 24d8a47f68

2024-01-15 14:30:44 +01:00

Compare

Lukas Tönne added 1 commit 2024-01-15 14:32:00 +01:00

a701aa44dc Removed unused python code for node inputs.

Lukas Tönne added 1 commit 2024-01-15 14:36:14 +01:00

e7b25fd4fe Reverted RNA changes to `draw_buttons` functions.

Lukas Tönne changed title from ~~WIP Fix #115727: Draw panel buttons in the node editor side bar~~ to Fix #115727: Draw panel buttons in the node editor side bar

2024-01-15 14:39:19 +01:00

Lukas Tönne commented

2024-01-15 14:47:18 +01:00

I've added a new UI template function to take care of the node buttons in the right order now. We can't fully do this in python at this point due to the lack of an API for the node declarations, so this is the next best option.

Note that currently the UI template does not actually create panels, which is the way the sockets were previously drawn in the sidebar too. If that's a desirable feature i could add it, but only if really needed.

I've added a new UI template function to take care of the node buttons in the right order now. We can't fully do this in python at this point due to the lack of an API for the node declarations, so this is the next best option. Note that currently the UI template does not actually create panels, which is the way the sockets were previously drawn in the sidebar too. If that's a desirable feature i could add it, but only if really needed.

Lukas Tönne requested review from Brecht Van Lommel 2024-01-15 14:49:57 +01:00

Brecht Van Lommel commented

2024-01-15 17:15:01 +01:00

Note that currently the UI template does not actually create panels, which is the way the sockets were previously drawn in the sidebar too. If that's a desirable feature i could add it, but only if really needed.

I think it's the right design to have panels here too. Any particular reason not to do it?

I just tried and it seems relatively straightforward, though I don't think this handles nested panels correctly. I'm a bit unsure about how the iterator and recursion relate, I thought it would recurse for each panel but it doesn't seem to be the case.

Using the name as the idname is also a bit weak.

diff --git a/source/blender/editors/interface/interface_template_node_inputs.cc b/source/blender/editors/interface/interface_template_node_inputs.cc
index 3e7f1c66c13..2cddb860851 100644
--- a/source/blender/editors/interface/interface_template_node_inputs.cc
+++ b/source/blender/editors/interface/interface_template_node_inputs.cc
@@ -11,6 +11,7 @@
 #include "BKE_context.hh"
 #include "BKE_node.hh"
 #include "BKE_node_runtime.hh"
+#include "BKE_screen.hh"
 
 #include "BLT_translation.h"
 
@@ -63,7 +64,8 @@ static void draw_node_input(bContext *C,
 }
 
 static void draw_node_declaration_items(bContext *C,
-                                        uiLayout *layout,
+                                        uiLayout *root_layout,
+                                        uiLayout *&layout,
                                         PointerRNA *node_ptr,
                                         ItemIterator &item_iter,
                                         const ItemIterator item_end)
@@ -75,20 +77,31 @@ static void draw_node_declaration_items(bContext *C,
     if (const SocketDeclaration *socket_decl = dynamic_cast<const SocketDeclaration *>(item_decl))
     {
       if (socket_decl->in_out == SOCK_IN) {
-        draw_node_input(C, layout, node_ptr, socket_decl->identifier);
+        if (layout) {
+          draw_node_input(C, layout, node_ptr, socket_decl->identifier);
+        }
       }
     }
     else if (const PanelDeclaration *panel_decl = dynamic_cast<const PanelDeclaration *>(
                  item_decl))
     {
-      /* Draw panel buttons at the top of each panel section. */
-      if (panel_decl->draw_buttons) {
-        panel_decl->draw_buttons(layout, C, node_ptr);
-      }
+      Panel *panel = uiLayoutGetRootPanel(root_layout);
+      LayoutPanelState *state = BKE_panel_layout_panel_state_ensure(
+          panel, panel_decl->name.c_str(), panel_decl->default_collapsed);
+      PointerRNA state_ptr = RNA_pointer_create(nullptr, &RNA_LayoutPanelState, state);
+      layout = uiLayoutPanel(
+          C, root_layout, IFACE_(panel_decl->name.c_str()), &state_ptr, "is_open");
 
-      const ItemIterator panel_item_end = item_iter + panel_decl->num_child_decls;
-      BLI_assert(panel_item_end <= item_end);
-      draw_node_declaration_items(C, layout, node_ptr, item_iter, panel_item_end);
+      if (layout) {
+        /* Draw panel buttons at the top of each panel section. */
+        if (panel_decl->draw_buttons) {
+          panel_decl->draw_buttons(layout, C, node_ptr);
+        }
+
+        const ItemIterator panel_item_end = item_iter + panel_decl->num_child_decls;
+        BLI_assert(panel_item_end <= item_end);
+        draw_node_declaration_items(C, root_layout, layout, node_ptr, item_iter, panel_item_end);
+      }
     }
   }
 }
@@ -115,7 +128,7 @@ void uiTemplateNodeInputs(uiLayout *layout, bContext *C, PointerRNA *ptr)
     /* Draw socket inputs and panel buttons in the order of declaration panels. */
     ItemIterator item_iter = node.declaration()->items.begin();
     const ItemIterator item_end = node.declaration()->items.end();
-    blender::ui::nodes::draw_node_declaration_items(C, layout, ptr, item_iter, item_end);
+    blender::ui::nodes::draw_node_declaration_items(C, layout, layout, ptr, item_iter, item_end);
   }
   else {
     /* Draw socket values using the flat inputs list. */

> Note that currently the UI template does not actually create panels, which is the way the sockets were previously drawn in the sidebar too. If that's a desirable feature i could add it, but only if really needed. I think it's the right design to have panels here too. Any particular reason not to do it? I just tried and it seems relatively straightforward, though I don't think this handles nested panels correctly. I'm a bit unsure about how the iterator and recursion relate, I thought it would recurse for each panel but it doesn't seem to be the case. Using the name as the idname is also a bit weak. ```diff diff --git a/source/blender/editors/interface/interface_template_node_inputs.cc b/source/blender/editors/interface/interface_template_node_inputs.cc index 3e7f1c66c13..2cddb860851 100644 --- a/source/blender/editors/interface/interface_template_node_inputs.cc +++ b/source/blender/editors/interface/interface_template_node_inputs.cc @@ -11,6 +11,7 @@ #include "BKE_context.hh" #include "BKE_node.hh" #include "BKE_node_runtime.hh" +#include "BKE_screen.hh" #include "BLT_translation.h" @@ -63,7 +64,8 @@ static void draw_node_input(bContext *C, } static void draw_node_declaration_items(bContext *C, - uiLayout *layout, + uiLayout *root_layout, + uiLayout *&layout, PointerRNA *node_ptr, ItemIterator &item_iter, const ItemIterator item_end) @@ -75,20 +77,31 @@ static void draw_node_declaration_items(bContext *C, if (const SocketDeclaration *socket_decl = dynamic_cast<const SocketDeclaration *>(item_decl)) { if (socket_decl->in_out == SOCK_IN) { - draw_node_input(C, layout, node_ptr, socket_decl->identifier); + if (layout) { + draw_node_input(C, layout, node_ptr, socket_decl->identifier); + } } } else if (const PanelDeclaration *panel_decl = dynamic_cast<const PanelDeclaration *>( item_decl)) { - /* Draw panel buttons at the top of each panel section. */ - if (panel_decl->draw_buttons) { - panel_decl->draw_buttons(layout, C, node_ptr); - } + Panel *panel = uiLayoutGetRootPanel(root_layout); + LayoutPanelState *state = BKE_panel_layout_panel_state_ensure( + panel, panel_decl->name.c_str(), panel_decl->default_collapsed); + PointerRNA state_ptr = RNA_pointer_create(nullptr, &RNA_LayoutPanelState, state); + layout = uiLayoutPanel( + C, root_layout, IFACE_(panel_decl->name.c_str()), &state_ptr, "is_open"); - const ItemIterator panel_item_end = item_iter + panel_decl->num_child_decls; - BLI_assert(panel_item_end <= item_end); - draw_node_declaration_items(C, layout, node_ptr, item_iter, panel_item_end); + if (layout) { + /* Draw panel buttons at the top of each panel section. */ + if (panel_decl->draw_buttons) { + panel_decl->draw_buttons(layout, C, node_ptr); + } + + const ItemIterator panel_item_end = item_iter + panel_decl->num_child_decls; + BLI_assert(panel_item_end <= item_end); + draw_node_declaration_items(C, root_layout, layout, node_ptr, item_iter, panel_item_end); + } } } } @@ -115,7 +128,7 @@ void uiTemplateNodeInputs(uiLayout *layout, bContext *C, PointerRNA *ptr) /* Draw socket inputs and panel buttons in the order of declaration panels. */ ItemIterator item_iter = node.declaration()->items.begin(); const ItemIterator item_end = node.declaration()->items.end(); - blender::ui::nodes::draw_node_declaration_items(C, layout, ptr, item_iter, item_end); + blender::ui::nodes::draw_node_declaration_items(C, layout, layout, ptr, item_iter, item_end); } else { /* Draw socket values using the flat inputs list. */ ```

Lukas Tönne commented

2024-01-15 17:20:38 +01:00

Any particular reason not to do it?

Just because it's a complication that i want to avoid unless its needed. The panel feature has already added so much technical debt because people said "wouldn't it be nice to have X" and then drop it later ...

> Any particular reason not to do it? Just because it's a complication that i want to avoid unless its needed. The panel feature has already added so much technical debt because people said "wouldn't it be nice to have X" and then drop it later ...

Brecht Van Lommel commented

2024-01-15 17:23:22 +01:00

OK, the way I see it is that this node properties sidebar is an alternative so people can work with collapsed nodes and separate properties (as in some other applications). For this use case having panel organization is equally helpful.

Lukas Tönne commented

2024-01-15 18:14:03 +01:00

Shouldn't the open/closed state of the panel in the sidebar be the same as that of the node itself? All the properties represent the state of a specific node instance after all.

In that case it should use the bNodePanelState array from node.panel_states() (in the same depth-first order they are drawn).

Shouldn't the open/closed state of the panel in the sidebar be the same as that of the node itself? All the properties represent the state of a specific node instance after all. In that case it should use the `bNodePanelState` array from `node.panel_states()` (in the same depth-first order they are drawn).

Brecht Van Lommel commented

2024-01-15 18:37:17 +01:00

I'm guessing users would not want these to be coupled. It seems a bit unexpected to have browsing the sidebar affect the shape of the node, causing it potentially overlap others.

Lukas Tönne added 1 commit 2024-01-16 10:21:14 +01:00

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

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

82a26c89c7 Draw panel layouts in the node editor sidebar for node input buttons.

Lukas Tönne commented

2024-01-16 10:24:17 +01:00

I've added the panel layouts now.

I just tried and it seems relatively straightforward, though I don't think this handles nested panels correctly. I'm a bit unsure about how the iterator and recursion relate, I thought it would recurse for each panel but it doesn't seem to be the case.

The node.declaration()->items array contains all the items in depth-first drawing order. The panels just have to consume those items whether they draw them or not.

Using the name as the idname is also a bit weak.

Yes, the panel name can change in node groups. The int identifier is a more stable option.

I've added the panel layouts now. > I just tried and it seems relatively straightforward, though I don't think this handles nested panels correctly. I'm a bit unsure about how the iterator and recursion relate, I thought it would recurse for each panel but it doesn't seem to be the case. The `node.declaration()->items` array contains all the items in depth-first drawing order. The panels just have to consume those items whether they draw them or not. > Using the name as the idname is also a bit weak. Yes, the panel name can change in node groups. The int identifier is a more stable option.

Lukas Tönne commented

2024-01-16 10:24:43 +01:00

@blender-bot build

Brecht Van Lommel approved these changes 2024-01-16 12:56:31 +01:00

Brecht Van Lommel commented

2024-01-16 12:56:53 +01:00

Thanks, I think this is now a really nice UI improvement.

Lukas Tönne merged commit 53e34dba80 into main

2024-01-16 13:38:07 +01:00

Lukas Tönne deleted branch node-sidebar-panel-buttons

2024-01-16 13:38:09 +01:00

Lukas Tönne referenced this issue from a commit