Nodes: Panels integration with blend files and UI #111348

Merged
Lukas Tönne merged 96 commits from LukasTonne/blender:node-panels-final into main 2023-08-30 12:37:28 +02:00
Member

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

  • enabling the new RNA API
  • using the new API in UI
  • read/write new interfaces from blend files
  • add versioning for backward compatibility
  • add forward-compatible writing code to reconstruct old interfaces

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 the ntree 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).

Screenshot_20230825_174544
Screenshot_20230825_174627

⚠️ script_load_addons test fails because of a required change in
Node Wrangler addon: #104849.
The addon fixes needs to be completed in sync with this PR.

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 * enabling the new RNA API * using the new API in UI * read/write new interfaces from blend files * add versioning for backward compatibility * add forward-compatible writing code to reconstruct old interfaces 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 the `ntree` 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). ![Screenshot_20230825_174544](/attachments/b191e2fa-537e-4f34-bea3-3ecc70698235) ![Screenshot_20230825_174627](/attachments/df322dc3-3835-4436-9179-395c2f02daf5) ⚠️ `script_load_addons` test fails because of a required change in Node Wrangler addon: [#104849](https://projects.blender.org/blender/blender-addons/pulls/104849). The addon fixes needs to be completed in sync with this PR.
Lukas Tönne added 7 commits 2023-08-21 16:04:42 +02:00
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
* enabling the new RNA API
* using the new API in UI
* read/write new interfaces from blend files
* add versioning for backward compatibility
* add forward-compatible writing code to reconstruct old interfaces

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 the `ntree` 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.
The socket added by the "Socket" drop down option was neither input nor
output, which is confusing. To get sensible defaults, the menu now has
separate "Input" and "Output" options.
Lukas Tönne requested review from Hans Goudey 2023-08-21 16:05:27 +02:00
Lukas Tönne requested review from Jacques Lucke 2023-08-21 16:05:33 +02:00
Lukas Tönne added the
Module
Nodes & Physics
label 2023-08-21 16:05:42 +02:00
Lukas Tönne added this to the Nodes & Physics project 2023-08-21 16:05:48 +02:00
Lukas Tönne added 1 commit 2023-08-21 16:39:55 +02:00
This is a recent regression: The original int uid was copied implicitly,
the string was only a transient identifier for legacy sockets. Now all
sockets use string identifiers again.
Lukas Tönne added 1 commit 2023-08-21 16:46:32 +02:00
Add a little bit of space between node items, match previous layout.
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
f1ef48e373
Author
Member

@blender-bot build

@blender-bot build
Hans Goudey requested changes 2023-08-21 17:09:50 +02:00
Hans Goudey left a comment
Member

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.

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")
Member

This can probably describe the purpose of the method a bit better :P

This can probably describe the purpose of the method a bit better :P
Author
Member

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 a default_value property.

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 a `default_value` property.
LukasTonne marked this conversation as resolved
@ -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;
Member

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.

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.
Author
Member

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.

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.
LukasTonne marked this conversation as resolved
@ -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);
Member

Should be possible to remove these, right? Except after versioning and file writing, these lists should be empty I'd think.

Should be possible to remove these, right? Except after versioning and file writing, these lists should be empty I'd think.
LukasTonne marked this conversation as resolved
@ -301,1 +296,3 @@
MEM_freeN(sock);
ntree->tree_interface.free_data();
/* Free legacy interface data */
LISTBASE_FOREACH_MUTABLE (bNodeSocket *, socket, &ntree->inputs_legacy) {
Member

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.

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.
LukasTonne marked this conversation as resolved
@ -473,1 +469,4 @@
namespace blender::bke::forward_compat {
static void write_node_socket_interface(BlendWriter *writer, bNodeSocket *sock)
Member

const socket pointer?

const socket pointer?
Author
Member

Yeah, old legacy code ...

Yeah, old legacy code ...
LukasTonne marked this conversation as resolved
@ -3936,3 +3870,3 @@
static void update_socket_declarations(ListBase *sockets,
Span<blender::nodes::SocketDeclarationPtr> declarations)
Span<blender::nodes::SocketDeclaration *> declarations)
Member

Hmm, doesn't seem obvious to me to change SocketDeclarationPtr to a raw pointer here, what's the reason for that?

Hmm, doesn't seem obvious to me to change `SocketDeclarationPtr` to a raw pointer here, what's the reason for that?
Author
Member

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 main items vector (using unique_ptr).
https://projects.blender.org/LukasTonne/blender/src/branch/node-panels-final/source/blender/nodes/NOD_node_declaration.hh#L533-L539

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 main `items` vector (using `unique_ptr`). https://projects.blender.org/LukasTonne/blender/src/branch/node-panels-final/source/blender/nodes/NOD_node_declaration.hh#L533-L539
LukasTonne marked this conversation as resolved
@ -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));
Member

A comment justifying the const cast would be nice here.

A comment justifying the const cast would be nice here.
Author
Member

comment added

  /* const_cast needed because the cache stores mutable item pointers, but needs a mutable
   * interface in order to get them. The interface itself is not modified here. */
comment added ``` /* const_cast needed because the cache stores mutable item pointers, but needs a mutable * interface in order to get them. The interface itself is not modified here. */ ```
LukasTonne marked this conversation as resolved
@ -577,2 +607,4 @@
FOREACH_NODETREE_END;
/* Convert old socket lists into new interface items. */
FOREACH_NODETREE_BEGIN (bmain, ntree, id) {
Member

Looks like this should be at the bottom of blo_do_versions_400, in a new check for the file version.

Looks like this should be at the bottom of `blo_do_versions_400`, in a new check for the file version.
Author
Member

Another merge fuckup i guess ...

Another merge fuckup i guess ...
LukasTonne marked this conversation as resolved
@ -0,0 +156,4 @@
std::unique_ptr<TreeViewItemDropTarget> create_drop_target() override;
private:
bNodeTree &nodetree_;
Member

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)

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)
LukasTonne marked this conversation as resolved
@ -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
Member

I think these formatting changes could be avoided.

I think these formatting changes could be avoided.
Author
Member

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 ...

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 ...
LukasTonne marked this conversation as resolved
@ -462,0 +489,4 @@
/* Parent panel stack with count of items still to be added. */
struct PanelUpdate {
int remaining_items;
bool is_collapsed;
Member

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.

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.
Author
Member

Added a comment.

  /* The panel stack keeps track of the hierarchy of panels. When a panel declaration is found a
   * new #PanelUpdate is added to the stack. Items in the declaration are added to the top panel of
   * the stack. Each panel expects a number of items to be added, after which the panel is removed
   * from the stack again. */

Could also do this with a recursive function, just a matter of preference.

Added a comment. ``` /* The panel stack keeps track of the hierarchy of panels. When a panel declaration is found a * new #PanelUpdate is added to the stack. Items in the declaration are added to the top panel of * the stack. Each panel expects a number of items to be added, after which the panel is removed * from the stack again. */ ``` Could also do this with a recursive function, just a matter of preference.
LukasTonne marked this conversation as resolved
Member

Unnecessary formatting changes?

Unnecessary formatting changes?
Author
Member

Gitea lost track of the code here, still relevant?

Gitea lost track of the code here, still relevant?
Author
Member

Went over all the unwanted formatting changes, should be ok.

Went over all the unwanted formatting changes, should be ok.
LukasTonne marked this conversation as resolved
@ -1629,6 +1753,130 @@ static void node_draw_sockets(const View2D &v2d,
}
Member

C++ style casts here

C++ style casts here
LukasTonne marked this conversation as resolved
@ -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);
Member

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.

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.
Author
Member

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).

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).
LukasTonne marked this conversation as resolved
@ -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.
Member

Unnecessary formatting change

Unnecessary formatting change
LukasTonne marked this conversation as resolved
Lukas Tönne added 5 commits 2023-08-22 10:39:50 +02:00
Lukas Tönne added 5 commits 2023-08-22 11:15:18 +02:00
Author
Member

Found 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 the ExtensionRNA inside the bNodeSocketType. When C++ code calls the callback it then needs a way too find that bNodeSocketType 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.

Found one more issue with custom socket types (we should turn the custom_nodes.py script into a test!): The [`typeinfo->draw_color`](https://projects.blender.org/LukasTonne/blender/src/commit/8693f1fdd2b82c3697b880492878400340ced96a/source/blender/blenkernel/BKE_node.h#L167) 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 the `ExtensionRNA` inside the `bNodeSocketType`. When C++ code calls the callback it then needs a way too find that `bNodeSocketType` 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.
Author
Member

I'll add a bNodeSocketType argument to that callback to make it a classmethod

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 think staticmethod/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 #109288

> I'll add a `bNodeSocketType` argument to that callback to make it a classmethod 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 think `staticmethod`/`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 #109288
Hans Goudey requested changes 2023-08-22 23:05:08 +02:00
Hans Goudey left a comment
Member

I 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.

  • Deleting an interface item in the list gives me no active item afterwards. The new active item should be the item with the same index afterwards, so it appears not to move in the list
  • In the list, both "is input" and "is output" can be disabled. That probably shouldn't be possible
  • I wonder if we really want to use the green color for the backdrop. It seems like many node trees will be very green that way. Did we discuss this already?
  • Looks like reordering panels resets the collapsed status of all nodes using that group. Is that a known limitation?

image

  • It seems like panels with only hidden sockets shouldn't display at all

image

This UI doesn't exactly look finished:

  • Booleans are usually displayed as checkboxes, and shouldn't use the word "Is" because that's implicit
  • Property split is missing now

image

  • The rectangle here looks slightly too wide.
  • I'm not sure the gap between panels is necessary
  • Similarly with the gap below a closed panel, this feels unnecessary to me

image

  • This doesn't seem quite right. The socket buttons should use the full width
I 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. - Deleting an interface item in the list gives me no active item afterwards. The new active item should be the item with the same index afterwards, so it appears not to move in the list - In the list, both "is input" and "is output" can be disabled. That probably shouldn't be possible - I wonder if we really want to use the green color for the backdrop. It seems like many node trees will be very green that way. Did we discuss this already? - Looks like reordering panels resets the collapsed status of all nodes using that group. Is that a known limitation? ![image](/attachments/e0663f32-074c-41d9-bb54-105f74a94692) - It seems like panels with only hidden sockets shouldn't display at all ![image](/attachments/6f9c716c-e0e7-447e-b32b-aeff4d3c0c39) This UI doesn't exactly look finished: - Booleans are usually displayed as checkboxes, and shouldn't use the word "Is" because that's implicit - Property split is missing now ![image](/attachments/5c2f7eb0-ad12-4d8d-a7c4-218e2b018b42) - The rectangle here looks slightly too wide. - I'm not sure the gap between panels is necessary - Similarly with the gap below a closed panel, this feels unnecessary to me ![image](/attachments/31ff9866-8f6b-4888-9970-96941e3fcc68) - This doesn't seem quite right. The socket buttons should use the full width
@ -1872,2 +1872,4 @@
return;
}
ntree->ensure_topology_cache();
Member

Not sure about this here TBH, it probably makes more sense to put this directly above the code that actually needs the 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.
LukasTonne marked this conversation as resolved
@ -88,3 +88,3 @@
bool supports_renaming() const override;
bool rename(StringRefNull new_name) override;
bool rename(const bContext &C, StringRefNull new_name) override;
Member

Maybe the new context argument could be added in a separate commit? That would help to make this diff smaller.

Maybe the new context argument could be added in a separate commit? That would help to make this diff smaller.
Author
Member
#111522
LukasTonne marked this conversation as resolved
Jacques Lucke reviewed 2023-08-23 08:44:31 +02:00
@ -876,3 +863,1 @@
class NodeTreeInterfacePanel(Panel):
class NODE_PT_node_tree_declaration(Panel):
Member

declaration -> interface?
same for template_node_tree_declaration maybe

`declaration` -> `interface`? same for `template_node_tree_declaration` maybe
LukasTonne marked this conversation as resolved
Jacques Lucke requested changes 2023-08-23 09:51:32 +02:00
Jacques Lucke left a comment
Member
  • The socket order if built-in nodes is different now, this is not expected to me nor mentioned in the description.
  • Exception when attempting to move an interface socket past the end.
  • Moving a socket that's in a panel does not seem to work.
  • Got a crash when linking a node tree from a .blend file. Might be related to the fact that the idname of an input is NodeSocketVectorTranslation (I connected the offset input of the Set Position node to the Group Input).
  • The versioning code currently looses default values.
  • Lots of regression tests are failing currently.

  • I'm a bit confused why nested panels are not allowed now. I thought we ended up saying that we do want to support them.
  • I'm unsure whether we want versioning code to call the methods to add interface sockets. The problem with that is that it is calling into code that might change over time. We do that in a few other cases, but in general it would be good to avoid if possible. Not sure how much boilerplate it would add to avoid using this method. Maybe that can also be solved later though.
* [x] The socket order if built-in nodes is different now, this is not expected to me nor mentioned in the description. * [x] Exception when attempting to move an interface socket past the end. * [x] Moving a socket that's in a panel does not seem to work. * [x] Got a crash when linking a node tree from a .blend file. Might be related to the fact that the idname of an input is `NodeSocketVectorTranslation` (I connected the offset input of the Set Position node to the Group Input). * [x] The versioning code currently looses default values. * [x] Lots of regression tests are failing currently. --- * I'm a bit confused why nested panels are not allowed now. I thought we ended up saying that we do want to support them. * I'm unsure whether we want versioning code to call the methods to add interface sockets. The problem with that is that it is calling into code that might change over time. We do that in a few other cases, but in general it would be good to avoid if possible. Not sure how much boilerplate it would add to avoid using this method. Maybe that can also be solved later though.
@ -876,3 +863,1 @@
class NodeTreeInterfacePanel(Panel):
class NODE_PT_node_tree_declaration(Panel):
Member

declaration -> interface?

`declaration` -> `interface`?
LukasTonne marked this conversation as resolved
@ -204,6 +204,8 @@ static AnonymousAttributeInferencingResult analyse_anonymous_attribute_usages(
{
BLI_assert(!tree.has_available_link_cycle());
const bNodeTreeInterfaceCache &cache = tree.interface_cache();
Member

cache -> interface_cache, because just cache seems to be a bit too generic here.

`cache` -> `interface_cache`, because just `cache` seems to be a bit too generic here.
LukasTonne marked this conversation as resolved
@ -850,1 +855,4 @@
{
/* Are child panels allowed? */
BLI_assert(item.item_type != NODE_INTERFACE_PANEL ||
(flag & NODE_INTERFACE_PANEL_ALLOW_CHILD_PANELS));
Member

Use this->flag. Reading the code like this I expected the flag to be an input parameter.

Use `this->flag`. Reading the code like this I expected the `flag` to be an input parameter.
LukasTonne marked this conversation as resolved
@ -107,2 +116,4 @@
/* UI name of the panel. */
char *name;
char *description;
/* eNodeTreeInterfacePanelFlag */
Member

Remove e.

Remove `e`.
LukasTonne marked this conversation as resolved
@ -281,1 +296,3 @@
bNodeTreeInterfacePanel *add_panel(blender::StringRefNull name, bNodeTreeInterfacePanel *parent);
bNodeTreeInterfacePanel *add_panel(blender::StringRefNull name,
blender::StringRefNull description,
const NodeTreeInterfacePanelFlag flag,
Member

Unnecessary const in declaration.

Unnecessary `const` in declaration.
LukasTonne marked this conversation as resolved
@ -309,0 +319,4 @@
typedef struct bNodePanelState {
/* Unique identifier for validating state against panels in node declaration. */
short uid;
Member

use int16_t instead of short

use `int16_t` instead of `short`
Author
Member

Changed to int for consistency with PanelDeclaration and bNodeTreeInterfacePanel where this identifier is copied from. Also using "identifier" consistently now instead of "uid".

Changed to int for consistency with `PanelDeclaration` and `bNodeTreeInterfacePanel` where this identifier is copied from. Also using "identifier" consistently now instead of "uid".
LukasTonne marked this conversation as resolved
Author
Member

Thanks for the reviews, i know it's a lot of code.

I wonder if we really want to use the green color for the backdrop. It seems like many node trees will be very green that way. Did we discuss this already?

The mockups we had were all fairly basic and not true to color.
#108895

Looks like reordering panels resets the collapsed status of all nodes using that group. Is that a known limitation?
Huh, i thought i had this working now. The panel states store the uid of the panel, so they should be able to validate against a changed list of panels.

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'm a bit confused why nested panels are not allowed now. I thought we ended up saying that we do want to support them.

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.

I'm unsure whether we want versioning code to call the methods to add interface sockets

Would have to duplicate all the code for initializing/copying socket default values. Ugly but possible, no strong opinion.

Thanks for the reviews, i know it's a lot of code. > I wonder if we really want to use the green color for the backdrop. It seems like many node trees will be very green that way. Did we discuss this already? The mockups we had were all fairly basic and not true to color. #108895 > Looks like reordering panels resets the collapsed status of all nodes using that group. Is that a known limitation? Huh, i thought i had this working now. The panel states store the `uid` of the panel, so they should be able to validate against a changed list of panels. 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'm a bit confused why nested panels are not allowed now. I thought we ended up saying that we do want to support them. 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. > I'm unsure whether we want versioning code to call the methods to add interface sockets Would have to duplicate all the code for initializing/copying socket default values. Ugly but possible, no strong opinion.
Lukas Tönne added 1 commit 2023-08-23 11:04:54 +02:00
Member

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.

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.

> 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. 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.
Lukas Tönne added 1 commit 2023-08-23 13:14:07 +02:00
The legacy input/output sockets ListBases must be constructed _before_
the ID struct of the bNodeTree is written, since it writes the
first/last pointer addresses of these lists.

Versioning code now also avoids calling BKE functions.
Lukas Tönne added 2 commits 2023-08-23 14:44:05 +02:00
Author
Member

@blender-bot build

@blender-bot build
Author
Member

@blender-bot build

@blender-bot build
Author
Member

The socket order if built-in nodes is different now, this is not expected to me nor mentioned in the description.

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.

> The socket order if built-in nodes is different now, this is not expected to me nor mentioned in the description. 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.
Lukas Tönne added 1 commit 2023-08-23 16:38:27 +02:00
Custom socket order and panels can be enabled by setting the
`use_custom_socket_order` flag on node declarations. This is currently
only enabled for group nodes.
Lukas Tönne added 1 commit 2023-08-23 16:52:06 +02:00
Lukas Tönne changed title from Node panels: Integration with blend files and UI to WIP: Node panels: Integration with blend files and UI 2023-08-23 17:07:06 +02:00
Lukas Tönne added 1 commit 2023-08-23 17:43:58 +02:00
The `socket_data_is_type` function was only considering base types,
but can be called for socket subtypes in some circumstances.
Use the same list as RNA to find a match among the subtypes too.
Lukas Tönne added 1 commit 2023-08-23 17:50:55 +02:00
The `bNodeTreeInterface` DNA type has been added in main a while ago,
and new test files have been added in the meantime. This makes the
DNA check for versioning unreliable, it will skip versioning test files
which already have the DNA struct but need to be versioned.
Using a subversion bump is much more reliable.
Lukas Tönne added 1 commit 2023-08-23 18:09:23 +02:00
Lukas Tönne added 1 commit 2023-08-23 18:35:08 +02:00
The declaration code for these nodes calculates the output field
dependency index based on the number of existing inputs, which requires
that input declarations are added to the list right away. It was
producing -1 index because the inputs list lagged behind by 1.
Author
Member

Moving a socket that's in a panel does not seem to work.

Issue 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.

> Moving a socket that's in a panel does not seem to work. Issue 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.
Lukas Tönne added 1 commit 2023-08-23 19:47:46 +02:00
Member
  • When using Ctrl+G to create a new node group, the outputs are currently at the bottom. Would be good to keep them at the top for the time being.

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).

* [x] When using Ctrl+G to create a new node group, the outputs are currently at the bottom. Would be good to keep them at the top for the time being. 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).
Lukas Tönne added 1 commit 2023-08-24 10:46:05 +02:00
Lukas Tönne added 2 commits 2023-08-24 11:53:20 +02:00
The new `draw_color_simple` callback does not depend on context or
a socket pointer or a node pointer, it only requires the socket
typeinfo (`bNodeSocketType`). In Python it's a registerable classmethod.
The existing `draw_color` callback also becomes optional in order to not
break existing scripts. One of the two functions should be implemented
by custom python nodes, preferably the `draw_color_simple` variant.
Lukas Tönne added 1 commit 2023-08-24 12:38:24 +02:00
This is very confusing RNA behavior: When a registerable method is
supposed to be a @classmethod it needs to have the `FUNC_NO_SELF` flag
set but NOT the `FUNC_USE_SELF_TYPE` flag (see
`bpy_class_validate_recursive` TODO comments). The overridden function
in the `NodeSocketStandard` subclass that has a built-in implementation,
however, needs the `FUNC_USE_SELF_TYPE` flag.
Lukas Tönne added 3 commits 2023-08-24 12:57:31 +02:00
This can happen when loading a file with custom python nodes and the
script defining them has not run or has failed. The forward-
compatibility code that creates equivalent socket cannot create copies
for unknown socket types, so it has to ignore such sockets.
Author
Member

I've added a draw_color_simple callback that works reliably without a context or socket pointer. Both draw_color and draw_color_simple are optional, but at least one should be implemented by addons.

I've added a `draw_color_simple` callback that works reliably without a context or socket pointer. Both `draw_color` and `draw_color_simple` are optional, but at least one should be implemented by addons.
Lukas Tönne added 1 commit 2023-08-24 13:15:15 +02:00
Fix versioning: outputs/inputs socket order and missing attribute info.
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
6e689bd2d5
Author
Member

@blender-bot build

@blender-bot build
Author
Member

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).

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.

> 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). 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:

  1. Items "outside" a panel should only be possible on top, before any panel is declared.
  2. Empty panels should not be shown 😢:
    image
  3. Until we support inline drawing better to follow Blender and have the Output before the Input (when the socket is both input and output) 😢:
    image
  4. nitpick: I would expect the collapsed sockets to be drawn on top of each other 😢:
    image
    image
  5. The colors of the panels should not follow the node color. In fact the panel get the same color as the background. The nested region is the one that is supposed to get darker 😊:
    image
  6. When having an active panel, new sockets should be added to it.

Need input from UI team:

  1. Default Closed -> Closed by Default
  2. The Is Input, Is Output could be replaced by an Enum (Input, Output, Both).
  3. rename Sockets > Interface (since it is not only Sockets now)
  4. Overall the look of the sockets "UIList" could be improved (at least for its alignment). I have no concrete suggestion, better to ask @pablovazquez about that 😢:
    image
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:** 1. Items "outside" a panel should only be possible on top, before any panel is declared. 2. Empty panels should not be shown 😢: ![image](/attachments/1a6331df-fbd0-4d5d-9dcb-ace685ded2a0) 3. Until we support inline drawing better to follow Blender and have the Output before the Input (when the socket is both input and output) 😢: ![image](/attachments/d6fe79d9-133c-43de-a1d5-ad53986ae316) 4. nitpick: I would expect the collapsed sockets to be drawn on top of each other 😢: ![image](/attachments/e72f70e3-1b3f-428b-8c37-0bf34cbacdcf) ![image](/attachments/423c186d-c95b-43e6-bb37-8be36972b627) 5. The colors of the panels should not follow the node color. In fact the panel get the same color as the background. The nested region is the one that is supposed to get darker 😊: ![image](/attachments/a7025180-234e-4bce-bf64-1aa7a485c54a) 6. When having an active panel, new sockets should be added to it. --- **Need input from UI team:** 1. Default Closed -> Closed by Default 2. The `Is Input`, `Is Output` could be replaced by an Enum (Input, Output, Both). 3. rename Sockets > Interface (since it is not only Sockets now) 4. Overall the look of the sockets "UIList" could be improved (at least for its alignment). I have no concrete suggestion, better to ask @pablovazquez about that 😢: ![image](/attachments/62fca694-ae09-443c-afad-11deb6904611)
Author
Member

@blender-bot build

@blender-bot build
Lukas Tönne added 1 commit 2023-08-24 16:02:57 +02:00
Drag+drop can be the primary method for moving items in the UI now.
Lukas Tönne added 1 commit 2023-08-24 16:18:16 +02:00
This slips through testing easily because the actual node instance
input values remain the same, only the node group settings for new
nodes are affected.
Lukas Tönne added 1 commit 2023-08-24 16:22:46 +02:00
Node group items can be specified as both input and output. This will
eventually generate sockets in the same vertical slot, but for now
generate sockets in the conventional layout.
Lukas Tönne added 1 commit 2023-08-24 18:48:24 +02:00
This is just another panel flag, so it could be disabled or removed
easily.
Lukas Tönne added 1 commit 2023-08-25 11:05:57 +02:00
Lukas Tönne added 2 commits 2023-08-25 12:06:34 +02:00
Lukas Tönne added 1 commit 2023-08-25 12:26:15 +02:00
The last+1 index of the items list is a valid insert position. Indices
of items are shifted down when move from a lower position.
Lukas Tönne added 3 commits 2023-08-25 13:01:26 +02:00
Item references for the callback have to be captured by reference to get
comparable pointers. Ideally bNodeTreeInterfaceSocket would not be
copyable in the first place, but it's still a C DNA struct.
Lukas Tönne added 1 commit 2023-08-25 13:53:54 +02:00
This also avoids users setting neither input nor output flags.
Lukas Tönne added 1 commit 2023-08-25 15:29:39 +02:00
Also renamed `uid` in bNodePanelState and PanelDeclaration to
`identifier`.
Lukas Tönne added 1 commit 2023-08-25 15:35:46 +02:00
Lukas Tönne added 2 commits 2023-08-25 16:01:43 +02:00
Lukas Tönne added 1 commit 2023-08-25 16:10:25 +02:00
Lukas Tönne added 4 commits 2023-08-25 16:20:24 +02:00
Lukas Tönne added 1 commit 2023-08-25 17:42:09 +02:00
- Use agreed-upon colors for the panel background.
- Fix overlap with the node boundary
- Don't draw panel background when collapsed.
Lukas Tönne added 1 commit 2023-08-25 18:04:58 +02:00
Lukas Tönne added 1 commit 2023-08-25 18:09:27 +02:00
Lukas Tönne added 1 commit 2023-08-28 09:48:09 +02:00
Lukas Tönne added 1 commit 2023-08-28 10:31:01 +02:00
Lukas Tönne added 1 commit 2023-08-28 10:40:21 +02:00
Previously the order didn't matter because inputs and outputs were
always separate. Now we have to take care to add sockets in the desired
order. Node groups will keep the familiar outputs..inputs order by
default.
Lukas Tönne added 1 commit 2023-08-28 10:50:33 +02:00
Lukas Tönne added 1 commit 2023-08-28 10:54:58 +02:00
Show default_closed property for panels with "Closed by Default" text.
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
42556bbb75
Author
Member

@blender-bot build

@blender-bot build
Lukas Tönne changed title from WIP: Node panels: Integration with blend files and UI to Node panels: Integration with blend files and UI 2023-08-28 10:59:04 +02:00
Lukas Tönne requested review from Hans Goudey 2023-08-28 10:59:20 +02:00
Lukas Tönne requested review from Jacques Lucke 2023-08-28 10:59:32 +02:00
Lukas Tönne added 2 commits 2023-08-28 11:05:45 +02:00
Fixed typo in the bl_node_group_interface test.
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
6342365c0c
Author
Member

@blender-bot build

@blender-bot build
Hans Goudey requested changes 2023-08-28 22:54:20 +02:00
Hans Goudey left a comment
Member
  • A simple test file is opening without any group inputs/outputs
    • image
  • I think it would be better to skip "Socket" in these UI labels, it's not really helpful and makes things feel more complex.
    • image
  • Similarly, I think I'd put "Duplicate" in a dropdown menu rather than between the + and - buttons. I think that would be more consistent with other areas in Blender
  • Formatting is a bit of a mess in the interace panel. I'll suggest some improvements:
    • image
    • Use property split
    • Avoid duplicating the name (I know it was there before, but that was just me with reasoning I disagree with now, and this is a nice opportunity to be more consistent with other areas which don't have the name).
    • Add label to the "Input/Output Type"
    • Put "Default Attribute Name" next to the regular default value (underneath description)
    • Change "Socket Type" label to just "Type", put above all other properties
  • Subtype dropdown is missing for float sockets
  • Adding a panel fails
    • image
- [ ] A simple test file is opening without any group inputs/outputs - ![image](/attachments/7c0902ad-a126-4a0b-a3c6-5a268ee454ed) - [x] I think it would be better to skip "Socket" in these UI labels, it's not really helpful and makes things feel more complex. - ![image](/attachments/85c4dc79-2b65-404d-9a38-c7608858e75e) - Similarly, I think I'd put "Duplicate" in a dropdown menu rather than between the + and - buttons. I think that would be more consistent with other areas in Blender - Formatting is a bit of a mess in the interace panel. I'll suggest some improvements: - ![image](/attachments/3d3bcb92-b89a-43ac-a63e-9a5e3c922fc8) - Use property split - Avoid duplicating the name (I know it was there before, but that was just me with reasoning I disagree with now, and this is a nice opportunity to be more consistent with other areas which don't have the name). - Add label to the "Input/Output Type" - Put "Default Attribute Name" next to the regular default value (underneath description) - Change "Socket Type" label to just "Type", put above all other properties - [x] Subtype dropdown is missing for float sockets - [ ] Adding a panel fails - ![image](/attachments/bba1b5ba-7efe-4040-97ea-971cdaa33f11)
@ -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(
Member

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.

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.
LukasTonne marked this conversation as resolved
@ -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()) {
Member

Any particular reason the inputs and outputs are reversed here? Before this diff, the inputs came first, then the outputs.

Any particular reason the inputs and outputs are reversed here? Before this diff, the inputs came first, then the outputs.
Author
Member

Because we want to retain the conventional outputs..inputs socket order when making a group.
#111348 (comment)

Because we want to retain the conventional outputs..inputs socket order when making a group. https://projects.blender.org/blender/blender/pulls/111348#issuecomment-1005128
LukasTonne marked this conversation as resolved
Hans Goudey reviewed 2023-08-28 22:59:58 +02:00
@ -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
Member

How about replacing this with three methods: interface_inputs(), interface_outputs() and interface_items(). The fact that they're stored in the same cache isn't that helpful to the code that uses these spans.

How about replacing this with three methods: `interface_inputs()`, `interface_outputs()` and `interface_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
Author
Member

done

done
LukasTonne marked this conversation as resolved
Author
Member

A simple test file is opening without any group inputs/outputs

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?

Avoid duplicating the name

To be clear: you mean remove the "Name: XXX" button below the tree view since it's already renameable through double-clicking?

Adding a panel fails

can't repro this one either, seems to work for me. Any particular circumstances?

> A simple test file is opening without any group inputs/outputs 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? > Avoid duplicating the name To be clear: you mean remove the "Name: XXX" button below the tree view since it's already renameable through double-clicking? > Adding a panel fails can't repro this one either, seems to work for me. Any particular circumstances?
Lukas Tönne added 11 commits 2023-08-29 13:02:41 +02:00
Lukas Tönne added 1 commit 2023-08-29 13:03:59 +02:00
Cleanup: Formatting.
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
eaeb6c0ef4
Author
Member

Add label to the "Input/Output Type"

Changed to "Input/Output Type" but this is an awkward label. Can be changed later if some native speaker has a better idea ("Kind"?)

Put "Default Attribute Name" next to the regular default value (underneath description)

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

Subtype dropdown is missing for float sockets

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 a subtype 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.

> Add label to the "Input/Output Type" Changed to "Input/Output Type" but this is an awkward label. Can be changed later if some native speaker has a better idea ("Kind"?) > Put "Default Attribute Name" next to the regular default value (underneath description) 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 > Subtype dropdown is missing for float sockets 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 a `subtype` 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.
Author
Member

@blender-bot build

@blender-bot build
Lukas Tönne added 1 commit 2023-08-29 16:13:08 +02:00
Hans Goudey requested changes 2023-08-29 16:34:00 +02:00
Hans Goudey left a comment
Member

Looking good! I think this will be the last time I request changes :)

  • The RNA properties "in out type", "subtype", and "socket type" probably shouldn't be animateable
  • When I have a panel selected in the list, and add an item, the item should go inside the panel
Looking good! I think this will be the last time I request changes :) - [x] The RNA properties "in out type", "subtype", and "socket type" probably shouldn't be animateable - [x] When I have a panel selected in the list, and add an item, the item should go inside the panel
@ -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")
Member

Input/Output Type -> Input/Output

"Type" is implied with the enum dropdown.

`Input/Output Type` -> `Input/Output` "Type" is implied with the enum dropdown.
LukasTonne marked this conversation as resolved
@ -46,6 +46,8 @@
#include "ANIM_armature_iter.hh"
#include "ANIM_bone_collections.h"
#include "NOD_socket.hh"
Member

Unused include now. Thanks

Unused include now. Thanks
LukasTonne marked this conversation as resolved
@ -583,6 +585,77 @@ static void version_replace_principled_hair_model(bNodeTree *ntree)
}
}
static void legacy_socket_move_to_interface(bNodeTreeInterfaceItem *&new_item_ptr,
Member

A reference to a pointer is a bit weird here. Simpler to just return the item pointer IMO

A reference to a pointer is a bit weird here. Simpler to just return the item pointer IMO
LukasTonne marked this conversation as resolved
@ -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) {
Member

Two calls to BLI_freelistN can replace this function

Two calls to `BLI_freelistN` can replace this function
LukasTonne marked this conversation as resolved
@ -1879,6 +1879,7 @@ void DepsgraphNodeBuilder::build_nodetree(bNodeTree *ntree)
if (built_map_.checkIsBuiltAndTag(ntree)) {
return;
}
Member

Unnecessary whitespace change

Unnecessary whitespace change
Author
Member

Again with these mysterious changes ... i suspect clang format glitch, will check the make format results a bit more closely next time.

Again with these mysterious changes ... i suspect clang format glitch, will check the `make format` results a bit more closely next time.
Member

Your patch used to ensure the topology cache here, I expect when you moved that the extra new line remained.

Your patch used to ensure the topology cache here, I expect when you moved that the extra new line remained.
LukasTonne marked this conversation as resolved
@ -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,
Member

Should be a const reference

Should be a const reference
LukasTonne marked this conversation as resolved
Lukas Tönne added 8 commits 2023-08-29 17:30:55 +02:00
Lukas Tönne requested review from Hans Goudey 2023-08-29 17:32:15 +02:00
Hans Goudey approved these changes 2023-08-29 18:05:51 +02:00
Hans Goudey changed title from Node panels: Integration with blend files and UI to Node: Panels integration with blend files and UI 2023-08-29 18:06:09 +02:00
Hans Goudey changed title from Node: Panels integration with blend files and UI to Nodes: Panels integration with blend files and UI 2023-08-29 18:06:16 +02:00
Jacques Lucke requested changes 2023-08-29 18:51:42 +02:00
Jacques Lucke left a comment
Member

Found one potentially blocking issue. Other than that, looks good to me.

  • I get a crash in a release build when doing the following. I don't get the issue in a debug build currently. Haven't checked whether a similar issue exists in main yet.
    1. Add geometry nodes.
    2. Add new input socket in the interface panel.
    3. Drag&Drop the new socket to the top of the interface list.
Found one potentially blocking issue. Other than that, looks good to me. * [x] I get a crash in a release build when doing the following. I don't get the issue in a debug build currently. Haven't checked whether a similar issue exists in `main` yet. 1. Add geometry nodes. 2. Add new input socket in the interface panel. 3. Drag&Drop the new socket to the top of the interface list.
@ -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')
Member

Change order.

Change order.
LukasTonne marked this conversation as resolved
@ -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))
Member

Not part of this patch, but might be reasonable to rename this to move_to_panel. Maybe keep a note somewhere.

Not part of this patch, but might be reasonable to rename this to `move_to_panel`. Maybe keep a note somewhere.
Author
Member

Added a note to "Future Tasks" in #110272. Can make tasks for some of these later.

Added a note to "Future Tasks" in #110272. Can make tasks for some of these later.
LukasTonne marked this conversation as resolved
@ -736,0 +748,4 @@
break;
}
if (items[test_pos]->item_type != NODE_INTERFACE_PANEL) {
/* Found valid position, insert after the last socket item */
Member

Minor: missing dot in comment, also check the comment below.

Minor: missing dot in comment, also check the comment below.
LukasTonne marked this conversation as resolved
@ -586,0 +613,4 @@
legacy_socket.prop = nullptr;
/* Unused data */
MEM_delete(legacy_socket.runtime);
Member

Also set the pointer to null to avoid a dangling pointer.

Also set the pointer to null to avoid a dangling pointer.
LukasTonne marked this conversation as resolved
@ -914,6 +971,29 @@ void blo_do_versions_400(FileData *fd, Library * /*lib*/, Main *bmain)
}
}
if (!MAIN_VERSION_FILE_ATLEAST(bmain, 400, 18)) {
Member

Update the subversion.

Update the subversion.
LukasTonne marked this conversation as resolved
Author
Member

I get a crash in a release build when doing the following.

I can't repro that on my laptop (win11, msvc build). Tried Release and RelWithDebInfo builds. Any more info in logs?

> I get a crash in a release build when doing the following. I can't repro that on my laptop (win11, msvc build). Tried Release and RelWithDebInfo builds. Any more info in logs?
Member

Hmmm, I was able to reproduce it reliably, but now I can't reproduce it anymore, so maybe forget about it for now.

Hmmm, I was able to reproduce it reliably, but now I can't reproduce it anymore, so maybe forget about it for now.
Lukas Tönne added 5 commits 2023-08-30 10:57:39 +02:00
Jacques Lucke approved these changes 2023-08-30 11:31:12 +02:00
Jacques Lucke left a comment
Member

@blender-bot build

@blender-bot build
Lukas Tönne added 1 commit 2023-08-30 11:32:24 +02:00
Removed unused headers.
Some checks failed
buildbot/vexp-code-patch-coordinator Build done.
f02bef4918
Author
Member

@blender-bot build

@blender-bot build
Lukas Tönne merged commit e071288ab2 into main 2023-08-30 12:37:28 +02:00
Lukas Tönne deleted branch node-panels-final 2023-08-30 12:37:30 +02:00
First-time contributor

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

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
Sign in to join this conversation.
No reviewers
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 Assignees
6 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#111348
No description provided.