Nodes: Add PanelDeclarationBuilder for panels in builtin nodes #111695

Lukas Tönne · 2023-08-30T13:59:36+02:00

Lukas Tönne commented

2023-08-30 13:59:36 +02:00

Node groups already have panels, but they modify the node declaration
directly, which is not something we want to do for builtin nodes. For
those the PanelDeclarationBuilder should be used.

PanelDeclarationBuilder has add_input/add_output methods just like NodeDeclarationBuilder. Adding sockets to a panel increases its size by one. All sockets must be added in order: Adding sockets or panels to the root NodeDeclarationBuilder after a panel will complete the panel and adding more sockets to it after that will fail. This is to enforce a stable item order where indices don't change after adding a socket, which is important for things like field dependencies.

Example:

static void node_declare(NodeDeclarationBuilder &b)
{
  // Currently this is necessary to enable custom layouts and panels.
  // Will go away eventually when most nodes uses custom layout.
  b.use_custom_socket_order();

  // Create a panel.
  PanelDeclarationBuilder &pb = b.add_panel("My Panel").description("A demo panel").default_closed(true);
  // Add to the panel instead of the root layout.
  pb.add_input<decl::Color>("Color").default_value({0.8f, 0.8f, 0.8f, 1.0f});
  pb.add_input<decl::Float>("Weight").unavailable();

  // Continue socket declarations as usual.
  b.add_output<decl::Shader>("BSDF");

  // !!! Warning: continuing the panel after other items is not allowed and will show an error.
  pb.add_output<decl::Float>("Bad Socket");
}

Node groups already have panels, but they modify the node declaration directly, which is not something we want to do for builtin nodes. For those the `PanelDeclarationBuilder` should be used. `PanelDeclarationBuilder` has `add_input`/`add_output` methods just like `NodeDeclarationBuilder`. Adding sockets to a panel increases its size by one. All sockets must be added in order: Adding sockets or panels to the root `NodeDeclarationBuilder` after a panel will complete the panel and adding more sockets to it after that will fail. This is to enforce a stable item order where indices don't change after adding a socket, which is important for things like field dependencies. Example: ```cpp static void node_declare(NodeDeclarationBuilder &b) { // Currently this is necessary to enable custom layouts and panels. // Will go away eventually when most nodes uses custom layout. b.use_custom_socket_order(); // Create a panel. PanelDeclarationBuilder &pb = b.add_panel("My Panel").description("A demo panel").default_closed(true); // Add to the panel instead of the root layout. pb.add_input<decl::Color>("Color").default_value({0.8f, 0.8f, 0.8f, 1.0f}); pb.add_input<decl::Float>("Weight").unavailable(); // Continue socket declarations as usual. b.add_output<decl::Shader>("BSDF"); // !!! Warning: continuing the panel after other items is not allowed and will show an error. pb.add_output<decl::Float>("Bad Socket"); } ```

❤️ 1

Lukas Tönne added 1 commit 2023-08-30 13:59:43 +02:00

196ee02947 Added a PanelDeclarationBuilder for adding panels to builtin nodes.

Node groups already have panels, but they modify the node declaration
directly, which is not something we want to do for builtin nodes. For
those the PanelDeclarationBuilder should be used.

Example:
```cpp
static void node_declare(NodeDeclarationBuilder &b)
{
  // Currently this is necessary to enable custom layouts and panels.
  // Will go away eventually when most nodes uses custom layout.
  b.use_custom_socket_order();

  // Put the next two items into this panel.
  b.add_panel("My Panel").description("A demo panel").items(2);

  // Socket declarations as usual.
  b.add_input<decl::Color>("Color").default_value({0.8f, 0.8f, 0.8f, 1.0f});
  b.add_input<decl::Float>("Weight").unavailable();
  b.add_output<decl::Shader>("BSDF");
}
```

Lukas Tönne added this to the Nodes & Physics project 2023-08-30 14:00:08 +02:00

Hans Goudey commented

2023-08-30 14:04:40 +02:00

Not that it will be used too much, but I think it might be nice to have an API which is a bit more obvious:

Something like this might be clearer:

panel = b.panel("Name");
b.input("Input").panel(panel);

or even

panel = b.panel("Name");
panel.input("Input")

Not that it will be used _too_ much, but I think it might be nice to have an API which is a bit more obvious: Something like this might be clearer: ``` panel = b.panel("Name"); b.input("Input").panel(panel); ``` or even ``` panel = b.panel("Name"); panel.input("Input") ```

Lukas Tönne commented

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

I'm undecided on what the API should look like in this area. Being able to "assign" a panel to a socket seems nice, but it also adds complexity because socket order may not be immediately obvious. If i add a socket to an earlier panel it can move above other sockets. Items appearing in the order they are declared is straightforward. Then again, defining panel content purely via size is also not great.

Utilizing C++ scopes was suggested by Jacques and i quite like that. I'll put this PR as WIP and see if i can make scoped panel declarations work.

I'm undecided on what the API should look like in this area. Being able to "assign" a panel to a socket seems nice, but it also adds complexity because socket order may not be immediately obvious. If i add a socket to an earlier panel it can move above other sockets. Items appearing in the order they are declared is straightforward. Then again, defining panel content purely via size is also not great. Utilizing C++ scopes was suggested by Jacques and i quite like that. I'll put this PR as WIP and see if i can make scoped panel declarations work.

👍 1

Lukas Tönne changed title from ~~Add a PanelDeclarationBuilder for adding panels to builtin nodes~~ to WIP: Add a PanelDeclarationBuilder for adding panels to builtin nodes

2023-08-30 14:13:16 +02:00

Lukas Tönne commented

2023-08-30 14:15:53 +02:00

Adding sockets directly to PanelDeclaration instead of NodeDeclaration is also nice, especially since it's so familiar from the way we use uiLayout.

Adding sockets directly to `PanelDeclaration` instead of `NodeDeclaration` is also nice, especially since it's so familiar from the way we use `uiLayout`.

Lukas Tönne added 1 commit 2023-09-07 17:33:24 +02:00

30bce48da3 Merge branch 'main' into panels-for-builtin-nodes

Lukas Tönne commented

2023-09-07 17:58:35 +02:00

I'm trying to figure out if/how to add sockets to panel declarations, at least as an API function that could still store a flattened list internally. One problem is the use of indices throughout socket declarations (e.g. for field dependencies), which requires that the index remains stable once a socket has been declared because subsequent declarations might depend on it. Adding sockets to a panel declaration can break this easily because the panel child item can be inserted higher up than the previous declarations.

A simple solution would be to just disallow adding stuff to panels once they are done, i.e. once a new panel is started or a socket added outside a panel (*) you'd get an error trying to add more sockets to it. This is what 99% of node declarations would do anyway.

(*) Adding sockets after panels is also not allowed because of our "panels after sockets" rule.

I'm trying to figure out if/how to add sockets to panel declarations, at least as an API function that could still store a flattened list internally. One problem is the use of indices throughout socket declarations (e.g. for field dependencies), which requires that the index remains stable once a socket has been declared because subsequent declarations might depend on it. Adding sockets to a panel declaration can break this easily because the panel child item can be inserted higher up than the previous declarations. A simple solution would be to just disallow adding stuff to panels once they are done, i.e. once a new panel is started or a socket added outside a panel (*) you'd get an error trying to add more sockets to it. This is what 99% of node declarations would do anyway. (*) Adding sockets after panels is also not allowed because of our "panels after sockets" rule.

Lukas Tönne added 1 commit 2023-09-08 11:44:10 +02:00

fe57bd66fb Mark panel declarations as 'complete' when adding at root level.

socket_decl_builder->index_ =

Lukas Tönne commented

2023-09-08 12:20:34 +02:00

I've changed the API so sockets can be directly added to a PanelDeclarationBuilder (see updated snippet in the description). This is not 100% fool-proof, but should catch the most common mistakes and adds only minor changes to declarations.

I've changed the API so sockets can be directly added to a `PanelDeclarationBuilder` (see updated snippet in the description). This is not 100% fool-proof, but should catch the most common mistakes and adds only minor changes to declarations.

Lukas Tönne added 2 commits 2023-09-08 12:33:40 +02:00

110cfe8b0f Merge branch 'main' into panels-for-builtin-nodes

c9b8bd68db More reliable locking of panel builders after adding items.

Lukas Tönne changed title from ~~WIP: Add a PanelDeclarationBuilder for adding panels to builtin nodes~~ to Nodes: Add PanelDeclarationBuilder for panels in builtin nodes

2023-09-08 12:34:18 +02:00

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

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

Dalai Felinto commented

2023-09-08 12:59:02 +02:00

Hi Lukas, this is great progress. I have an issue though.

The moment I enable b.use_custom_socket_order() I can no longer have the properties between the outputs and inputs.

This is not blocking to this patch as long as we don't use this feature for nodes that have properties.

So I'm fine to have it in as incremental change towards our final tests and validations before we get the rules enforced on the API level. But should be used carefully.

Hi Lukas, this is great progress. I have an issue though. The moment I enable `b.use_custom_socket_order()` I can no longer have the properties between the outputs and inputs. This is not blocking to this patch as long as we don't use this feature for nodes that have properties. So I'm fine to have it in as incremental change towards our final tests and validations before we get the rules enforced on the API level. But should be used carefully.

Jacques Lucke approved these changes 2023-09-08 16:23:59 +02:00

Jacques Lucke left a comment

Generally seems fine. I think we should use the convention that we put panels into a separate nested { ... } block.

Generally seems fine. I think we should use the convention that we put panels into a separate nested `{ ... }` block.

source/blender/nodes/NOD_node_declaration.hh Outdated

						
				@ -808,3 +876,4 @@

				  return socket_decl_builder_ref;

				}

				inline PanelDeclarationBuilder &NodeDeclarationBuilder::add_panel(StringRef name, int identifier)

Jacques Lucke commented

2023-09-08 16:23:43 +02:00

This doesn't have to be in the header.

LukasTonne marked this conversation as resolved

Hans Goudey approved these changes 2023-09-08 16:28:26 +02:00

source/blender/nodes/NOD_node_declaration.hh Outdated

						
				@ -516,2 +516,4 @@

				  using Self = PanelDeclarationBuilder;

				  NodeDeclarationBuilder *node_decl_builder_ = nullptr;

				  PanelDeclaration *decl_;

				  /* Panel is complete once items are added after it.

Hans Goudey commented

2023-09-08 16:27:47 +02:00

Proper doxygen syntax for multi-line class variable documentation (just for consistency)

/** 
 * Panel is complete once items are added after it.
 * Completed panels are locked and no more items can be added.
 */

Proper doxygen syntax for multi-line class variable documentation (just for consistency) ``` /** * Panel is complete once items are added after it. * Completed panels are locked and no more items can be added. */

LukasTonne marked this conversation as resolved

Lukas Tönne commented

2023-09-08 16:31:25 +02:00

This does not currently enforce the "panels below sockets" rule (that's only done by the node group interfaces). Panel above sockets has some side effects in drawing, e.g. the panel background extends to all the bottom sockets when the panel is open.

This does not currently enforce the "panels below sockets" rule (that's only done by the node group interfaces). Panel above sockets has some side effects in drawing, e.g. the panel background extends to all the bottom sockets when the panel is open. ![Screenshot_20230908_163032](/attachments/c236b850-db47-4bfb-b106-40c8a07a90c2)

Screenshot_20230908_163032.png

46 KiB

Jacques Lucke commented

2023-09-08 16:36:18 +02:00

I think we could just have an assert that checks for invalid declarations after declarations have been created.

Lukas Tönne commented

2023-09-08 17:45:07 +02:00

I think we should use the convention that we put panels into a separate nested { ... } block.

Yeah as a convention that's fine, it helps make sure a panel builder is not used accidentally when it's not valid any more. I've tried to think of ways of using C++ syntax for a scoped builder, but wouldn't know how to prevent adding to the root interface while a panel is being built just by means of syntax. Don't want to get too fancy - as long as most simple errors are prevented it should be ok.

> I think we should use the convention that we put panels into a separate nested { ... } block. Yeah as a convention that's fine, it helps make sure a panel builder is not used accidentally when it's not valid any more. I've tried to think of ways of using C++ syntax for a scoped builder, but wouldn't know how to prevent adding to the root interface while a panel is being built just by means of syntax. Don't want to get too fancy - as long as most simple errors are prevented it should be ok.

Jacques Lucke commented

2023-09-08 17:53:54 +02:00

It's fine to use the convention. If there is an assert somewhere that can catch bad declarations, that's a great addition as well, even if it's not guaranteed at compile time.

Lukas Tönne added 1 commit 2023-09-11 11:06:48 +02:00

4e09508ef0 Merge branch 'main' into panels-for-builtin-nodes

Lukas Tönne added 1 commit 2023-09-11 11:09:07 +02:00

2afcda55b9 Removed test case.

Lukas Tönne added 1 commit 2023-09-11 12:41:58 +02:00

7c410f93f6 Added a validation function to test node declarations in debug mode.

Lukas Tönne added 2 commits 2023-09-11 12:49:49 +02:00

f2d28049b9 add_panel method does not need to be inlined.

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

aab8eb821c Cleanup: comment style.

Lukas Tönne commented

2023-09-11 12:50:39 +02:00