From 354915cf3c87bdebff6c63c8e88579e87eb0b8dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20T=C3=B6nne?= Date: Fri, 22 Sep 2023 16:56:59 +0200 Subject: [PATCH] Nodes: revert the inline (pass-through) socket feature Inlined sockets in the same vertical space are no longer supported. This removes `input_output` socket declarations, the inlining feature in node drawing, and the `Both` option for node group interface sockets. Versioning code splits existing node group sockets into individual sockets again. Unfortunately some links may get lost in versioning files using the feature, because of an unnoticed bug: Socket identifiers have to be unique in the node group items list but inlined input/output sockets have the same identifier. This still works for most situations because uniqueness is only required within input/output lists. Creating proper unique identifiers will discard any link from the previous output socket. This cannot easily be fixed without `after_linking` versioning code, which should be avoided. Pull Request: https://projects.blender.org/blender/blender/pulls/112560 --- .../blenloader/intern/versioning_400.cc | 131 ++++++++++++++++++ .../blender/editors/space_node/node_draw.cc | 33 +---- .../intern/rna_node_tree_interface.cc | 5 - source/blender/nodes/NOD_node_declaration.hh | 32 ----- source/blender/nodes/intern/node_common.cc | 10 +- .../blender/nodes/intern/node_declaration.cc | 26 ++-- tests/python/bl_node_group_interface.py | 5 - 7 files changed, 146 insertions(+), 96 deletions(-) diff --git a/source/blender/blenloader/intern/versioning_400.cc b/source/blender/blenloader/intern/versioning_400.cc index b27d32d0f57..872c7c75843 100644 --- a/source/blender/blenloader/intern/versioning_400.cc +++ b/source/blender/blenloader/intern/versioning_400.cc @@ -791,6 +791,115 @@ static void version_principled_bsdf_coat(bNodeTree *ntree) ntree, SH_NODE_BSDF_PRINCIPLED, "Clearcoat Normal", "Coat Normal"); } +static void version_copy_socket(bNodeTreeInterfaceSocket &dst, + const bNodeTreeInterfaceSocket &src, + char *identifier) +{ + /* Node socket copy function based on bNodeTreeInterface::item_copy to avoid using blenkernel. */ + dst.name = BLI_strdup(src.name); + dst.description = BLI_strdup_null(src.description); + dst.socket_type = BLI_strdup(src.socket_type); + dst.default_attribute_name = BLI_strdup_null(src.default_attribute_name); + dst.identifier = identifier; + if (src.properties) { + dst.properties = IDP_CopyProperty_ex(src.properties, 0); + } + if (src.socket_data != nullptr) { + dst.socket_data = MEM_dupallocN(src.socket_data); + /* No user count increment needed, gets reset after versioning. */ + } +} + +static int version_nodes_find_valid_insert_position_for_item(const bNodeTreeInterfacePanel &panel, + const bNodeTreeInterfaceItem &item, + const int initial_pos) +{ + const bool sockets_above_panels = !(panel.flag & + NODE_INTERFACE_PANEL_ALLOW_SOCKETS_AFTER_PANELS); + const blender::Span items = {panel.items_array, panel.items_num}; + + int pos = initial_pos; + + if (sockets_above_panels) { + if (item.item_type == NODE_INTERFACE_PANEL) { + /* Find the closest valid position from the end, only panels at or after #position. */ + for (int test_pos = items.size() - 1; test_pos >= initial_pos; test_pos--) { + if (test_pos < 0) { + /* Initial position is out of range but valid. */ + break; + } + if (items[test_pos]->item_type != NODE_INTERFACE_PANEL) { + /* Found valid position, insert after the last socket item. */ + pos = test_pos + 1; + break; + } + } + } + else { + /* Find the closest valid position from the start, no panels at or after #position. */ + for (int test_pos = 0; test_pos <= initial_pos; test_pos++) { + if (test_pos >= items.size()) { + /* Initial position is out of range but valid. */ + break; + } + if (items[test_pos]->item_type == NODE_INTERFACE_PANEL) { + /* Found valid position, inserting moves the first panel. */ + pos = test_pos; + break; + } + } + } + } + + return pos; +} + +static void version_nodes_insert_item(bNodeTreeInterfacePanel &parent, + bNodeTreeInterfaceSocket &socket, + int position) +{ + /* Apply any constraints on the item positions. */ + position = version_nodes_find_valid_insert_position_for_item(parent, socket.item, position); + position = std::min(std::max(position, 0), parent.items_num); + + blender::MutableSpan old_items = {parent.items_array, + parent.items_num}; + parent.items_num++; + parent.items_array = MEM_cnew_array(parent.items_num, __func__); + parent.items().take_front(position).copy_from(old_items.take_front(position)); + parent.items().drop_front(position + 1).copy_from(old_items.drop_front(position)); + parent.items()[position] = &socket.item; + + if (old_items.data()) { + MEM_freeN(old_items.data()); + } +} + +/* Node group interface copy function based on bNodeTreeInterface::insert_item_copy. */ +static void version_node_group_split_socket(bNodeTreeInterface &tree_interface, + bNodeTreeInterfaceSocket &socket, + bNodeTreeInterfacePanel *parent, + int position) +{ + if (parent == nullptr) { + parent = &tree_interface.root_panel; + } + + bNodeTreeInterfaceSocket *csocket = static_cast( + MEM_dupallocN(&socket)); + /* Generate a new unique identifier. + * This might break existing links, but the identifiers were duplicate anyway. */ + char *dst_identifier = BLI_sprintfN("Socket_%d", tree_interface.next_uid++); + version_copy_socket(*csocket, socket, dst_identifier); + + version_nodes_insert_item(*parent, *csocket, position); + + /* Original socket becomes output. */ + socket.flag &= ~NODE_INTERFACE_SOCKET_INPUT; + /* Copied socket becomes input. */ + csocket->flag &= ~NODE_INTERFACE_SOCKET_OUTPUT; +} + void blo_do_versions_400(FileData *fd, Library * /*lib*/, Main *bmain) { if (!MAIN_VERSION_FILE_ATLEAST(bmain, 400, 1)) { @@ -1220,6 +1329,28 @@ void blo_do_versions_400(FileData *fd, Library * /*lib*/, Main *bmain) } } } + + /* Convert sockets with both input and output flag into two separate sockets. */ + FOREACH_NODETREE_BEGIN (bmain, ntree, id) { + blender::Vector sockets_to_split; + ntree->tree_interface.foreach_item([&](bNodeTreeInterfaceItem &item) { + if (item.item_type == NODE_INTERFACE_SOCKET) { + bNodeTreeInterfaceSocket &socket = reinterpret_cast(item); + if ((socket.flag & NODE_INTERFACE_SOCKET_INPUT) && + (socket.flag & NODE_INTERFACE_SOCKET_OUTPUT)) { + sockets_to_split.append(&socket); + } + } + return true; + }); + + for (bNodeTreeInterfaceSocket *socket : sockets_to_split) { + const int position = ntree->tree_interface.find_item_position(socket->item); + bNodeTreeInterfacePanel *parent = ntree->tree_interface.find_item_parent(socket->item); + version_node_group_split_socket(ntree->tree_interface, *socket, parent, position + 1); + } + } + FOREACH_NODETREE_END; } /** diff --git a/source/blender/editors/space_node/node_draw.cc b/source/blender/editors/space_node/node_draw.cc index b6d141ca9d6..d1ba61e8dd9 100644 --- a/source/blender/editors/space_node/node_draw.cc +++ b/source/blender/editors/space_node/node_draw.cc @@ -588,44 +588,19 @@ static Vector node_build_item_data(bNode &node) if (const nodes::SocketDeclaration *socket_decl = dynamic_cast(item_decl->get())) { - bNodeSocket *used_input = nullptr; - bNodeSocket *used_output = nullptr; switch (socket_decl->in_out) { case SOCK_IN: BLI_assert(input != input_end); - used_input = *input; + result.append({socket_decl, *input, nullptr}); ++input; break; case SOCK_OUT: BLI_assert(output != output_end); - used_output = *output; + result.append({socket_decl, nullptr, *output}); ++output; break; } ++item_decl; - - if (socket_decl->inline_with_next && item_decl != item_decl_end) { - /* Consume the next item as well when inlining sockets. */ - const nodes::SocketDeclaration *next_socket_decl = - dynamic_cast(item_decl->get()); - if (next_socket_decl && next_socket_decl->in_out != socket_decl->in_out) { - switch (next_socket_decl->in_out) { - case SOCK_IN: - BLI_assert(input != input_end); - used_input = *input; - ++input; - break; - case SOCK_OUT: - BLI_assert(output != output_end); - used_output = *output; - ++output; - break; - } - ++item_decl; - } - } - - result.append({socket_decl, used_input, used_output}); } else if (const nodes::PanelDeclaration *panel_decl = dynamic_cast(item_decl->get())) @@ -723,8 +698,8 @@ static void node_update_basis_from_declaration( else if (item.is_valid_socket()) { if (item.input) { SET_FLAG_FROM_TEST(item.input->flag, is_parent_collapsed, SOCK_PANEL_COLLAPSED); - /* Draw buttons before the first input, unless it's inline with an output. */ - if (!item.socket_decl->inline_with_next && !buttons_drawn) { + /* Draw buttons before the first input. */ + if (!buttons_drawn) { buttons_drawn = true; need_spacer_after_item = node_update_basis_buttons( C, ntree, node, node.typeinfo->draw_buttons, block, locy); diff --git a/source/blender/makesrna/intern/rna_node_tree_interface.cc b/source/blender/makesrna/intern/rna_node_tree_interface.cc index 4ea2a0b103a..182eae07d9f 100644 --- a/source/blender/makesrna/intern/rna_node_tree_interface.cc +++ b/source/blender/makesrna/intern/rna_node_tree_interface.cc @@ -24,11 +24,6 @@ const EnumPropertyItem rna_enum_node_tree_interface_item_type_items[] = { static const EnumPropertyItem node_tree_interface_socket_in_out_items[] = { {NODE_INTERFACE_SOCKET_INPUT, "INPUT", 0, "Input", "Generate a input node socket"}, {NODE_INTERFACE_SOCKET_OUTPUT, "OUTPUT", 0, "Output", "Generate a output node socket"}, - {NODE_INTERFACE_SOCKET_INPUT | NODE_INTERFACE_SOCKET_OUTPUT, - "BOTH", - 0, - "Both", - "Generate both input and output node socket"}, {0, nullptr, 0, nullptr, nullptr}}; #ifdef RNA_RUNTIME diff --git a/source/blender/nodes/NOD_node_declaration.hh b/source/blender/nodes/NOD_node_declaration.hh index 34018d184af..32a74302d8e 100644 --- a/source/blender/nodes/NOD_node_declaration.hh +++ b/source/blender/nodes/NOD_node_declaration.hh @@ -185,7 +185,6 @@ class SocketDeclaration : public ItemDeclaration { bool is_unavailable = false; bool is_attribute_name = false; bool is_default_link_socket = false; - bool inline_with_next = false; InputSocketFieldType input_field_type = InputSocketFieldType::None; OutputFieldDependency output_field_dependency; @@ -635,10 +634,6 @@ class PanelDeclarationBuilder { typename DeclType::Builder &add_input(StringRef name, StringRef identifier = ""); template typename DeclType::Builder &add_output(StringRef name, StringRef identifier = ""); - template - typename DeclType::Builder &add_input_output(StringRef name, - StringRef identifier_in = "", - StringRef identifier_out = ""); }; using PanelDeclarationPtr = std::unique_ptr; @@ -708,10 +703,6 @@ class NodeDeclarationBuilder { typename DeclType::Builder &add_input(StringRef name, StringRef identifier = ""); template typename DeclType::Builder &add_output(StringRef name, StringRef identifier = ""); - template - typename DeclType::Builder &add_input_output(StringRef name, - StringRef identifier_in = "", - StringRef identifier_out = ""); PanelDeclarationBuilder &add_panel(StringRef name, int identifier = -1); aal::RelationsInNode &get_anonymous_attribute_relations() @@ -923,21 +914,6 @@ typename DeclType::Builder &PanelDeclarationBuilder::add_output(StringRef name, return node_decl_builder_->add_socket(name, "", identifier, SOCK_OUT); } -template -typename DeclType::Builder &PanelDeclarationBuilder::add_input_output(StringRef name, - StringRef identifier_in, - StringRef identifier_out) -{ - if (is_complete_) { - static typename DeclType::Builder dummy_builder = {}; - BLI_assert_unreachable(); - return dummy_builder; - } - ++this->decl_->num_child_decls; - return node_decl_builder_->add_socket( - name, identifier_in, identifier_out, SOCK_IN | SOCK_OUT); -} - /** \} */ /* -------------------------------------------------------------------- */ @@ -970,14 +946,6 @@ inline typename DeclType::Builder &NodeDeclarationBuilder::add_output(StringRef return this->add_socket(name, "", identifier, SOCK_OUT); } -template -inline typename DeclType::Builder &NodeDeclarationBuilder::add_input_output( - StringRef name, StringRef identifier_in, StringRef identifier_out) -{ - set_active_panel_builder(nullptr); - return this->add_socket(name, identifier_in, identifier_out, SOCK_IN | SOCK_OUT); -} - template inline typename DeclType::Builder &NodeDeclarationBuilder::add_socket(StringRef name, StringRef identifier_in, diff --git a/source/blender/nodes/intern/node_common.cc b/source/blender/nodes/intern/node_common.cc index 6ac435f8006..26131c626bd 100644 --- a/source/blender/nodes/intern/node_common.cc +++ b/source/blender/nodes/intern/node_common.cc @@ -387,18 +387,14 @@ void node_group_declare_dynamic(const bNodeTree & /*node_tree*/, declaration_for_interface_socket( *group, socket, SOCK_OUT) : nullptr; - /* Inline with the output socket if using input+output mode. */ - if (input_decl && output_decl) { - input_decl->inline_with_next = true; + if (output_decl) { + r_declaration.outputs.append(output_decl.get()); + r_declaration.items.append(std::move(output_decl)); } if (input_decl) { r_declaration.inputs.append(input_decl.get()); r_declaration.items.append(std::move(input_decl)); } - if (output_decl) { - r_declaration.outputs.append(output_decl.get()); - r_declaration.items.append(std::move(output_decl)); - } break; } case NODE_INTERFACE_PANEL: { diff --git a/source/blender/nodes/intern/node_declaration.cc b/source/blender/nodes/intern/node_declaration.cc index 64d6df73943..b54d23a2a7a 100644 --- a/source/blender/nodes/intern/node_declaration.cc +++ b/source/blender/nodes/intern/node_declaration.cc @@ -34,13 +34,6 @@ void build_node_declaration_dynamic(const bNodeTree &node_tree, void NodeDeclarationBuilder::finalize() { - /* Declarations generating both input and output should align these sockets. */ - for (std::unique_ptr &socket_builder : socket_builders_) { - if (socket_builder->input_declaration() && socket_builder->output_declaration()) { - socket_builder->input_declaration()->inline_with_next = true; - } - } - if (is_function_node_) { for (std::unique_ptr &socket_builder : socket_builders_) { if (SocketDeclaration *socket_decl = socket_builder->input_declaration()) { @@ -207,17 +200,14 @@ bool NodeDeclaration::is_valid() const return false; } - /* Check for consistent outputs.., inputs.. blocks. - * Ignored for sockets that are inline pairs. */ - if (!socket_decl->inline_with_next) { - if (state.socket_in_out == SOCK_OUT && socket_decl->in_out == SOCK_IN) { - /* Start of input sockets. */ - state.socket_in_out = SOCK_IN; - } - if (socket_decl->in_out != state.socket_in_out) { - std::cout << "Output socket added after input socket" << std::endl; - return false; - } + /* Check for consistent outputs.., inputs.. blocks. */ + if (state.socket_in_out == SOCK_OUT && socket_decl->in_out == SOCK_IN) { + /* Start of input sockets. */ + state.socket_in_out = SOCK_IN; + } + if (socket_decl->in_out != state.socket_in_out) { + std::cout << "Output socket added after input socket" << std::endl; + return false; } /* Item counting for the panels, but ignore for root items. */ diff --git a/tests/python/bl_node_group_interface.py b/tests/python/bl_node_group_interface.py index 09d72e98a18..97d88422524 100644 --- a/tests/python/bl_node_group_interface.py +++ b/tests/python/bl_node_group_interface.py @@ -132,18 +132,13 @@ class NodeGroupInterfaceTests: out1 = tree.interface.new_socket("Output 1", socket_type=socket_type, in_out='OUTPUT') self.assertIsNotNone(out1, f"Could not create socket of type {socket_type}") - inout0 = tree.interface.new_socket("Input/Output 0", socket_type=socket_type, in_out='BOTH') - self.assertIsNotNone(inout0, f"Could not create socket of type {socket_type}") - self.assertSequenceEqual([(s.name, s.bl_idname) for s in group_node.inputs], [ ("Input 0", socket_type), ("Input 1", socket_type), - ("Input/Output 0", socket_type), ]) self.assertSequenceEqual([(s.name, s.bl_idname) for s in group_node.outputs], [ ("Output 0", socket_type), ("Output 1", socket_type), - ("Input/Output 0", socket_type), ]) def do_test_user_count(self, value, expected_users):