From ea618c20273fb2d3824d57033064e012ca59c608 Mon Sep 17 00:00:00 2001 From: Jacques Lucke Date: Mon, 23 Jun 2025 10:08:43 +0200 Subject: [PATCH] Refactor: Nodes: improve socket value variant construction Previously, there was the issue that `SocketValueVariant` had a constructor that took a forwarding reference as parameter. This was problematic, because this could potentially hide copy/move constructors which is not intentional. This patch makes sure that these special constructors are not overridden and adds two static utility functions to make sure it's still straight forward to construct the `SocketValueVariant` on a single line. Clangd also warned about this case. Pull Request: https://projects.blender.org/blender/blender/pulls/140842 --- .../blenkernel/BKE_node_socket_value.hh | 38 ++++++++++++++++--- .../blenkernel/intern/bake_items_socket.cc | 6 +-- source/blender/nodes/NOD_geometry_exec.hh | 3 +- .../geometry/nodes/node_geo_index_switch.cc | 2 +- .../geometry/nodes/node_geo_menu_switch.cc | 2 +- .../nodes/geometry/nodes/node_geo_switch.cc | 2 +- .../intern/geometry_nodes_closure_zone.cc | 5 ++- .../nodes/intern/geometry_nodes_execute.cc | 10 ++--- ...try_nodes_foreach_geometry_element_zone.cc | 6 +-- .../intern/geometry_nodes_lazy_function.cc | 2 +- .../blender/nodes/intern/node_declaration.cc | 24 +++++++----- source/blender/nodes/intern/node_socket.cc | 8 ++-- 12 files changed, 69 insertions(+), 39 deletions(-) diff --git a/source/blender/blenkernel/BKE_node_socket_value.hh b/source/blender/blenkernel/BKE_node_socket_value.hh index ac939af4f26..9181db425fa 100644 --- a/source/blender/blenkernel/BKE_node_socket_value.hh +++ b/source/blender/blenkernel/BKE_node_socket_value.hh @@ -81,12 +81,30 @@ class SocketValueVariant { * Create an empty variant. This is not valid for any socket type yet. */ SocketValueVariant() = default; + SocketValueVariant(const SocketValueVariant &other) = default; + SocketValueVariant(SocketValueVariant &&other) = default; + SocketValueVariant &operator=(const SocketValueVariant &other) = default; + SocketValueVariant &operator=(SocketValueVariant &&other) = default; + ~SocketValueVariant() = default; /** - * Create a variant based on the given value. This works for primitive types, #GField and - * #Field. + * Create a variant based on the given value. This works for primitive types. For more complex + * types use #set explicity. Alternatively, one can use the #From or #ConstructIn utilities. */ - template explicit SocketValueVariant(T &&value); + template> || + is_same_any_v, std::string>))> + explicit SocketValueVariant(T &&value) + { + this->set(std::forward(value)); + } + + /** Construct a #SocketValueVariant at the given pointer from the given value. */ + template static SocketValueVariant &ConstructIn(void *ptr, T &&value); + + /** Create a new #SocketValueVariant from the given value. */ + template static SocketValueVariant From(T &&value); /** * \return True if the stored value is valid for a specific socket type. This is mainly meant to @@ -174,9 +192,19 @@ class SocketValueVariant { template void store_impl(T value); }; -template inline SocketValueVariant::SocketValueVariant(T &&value) +template +inline SocketValueVariant &SocketValueVariant::ConstructIn(void *ptr, T &&value) { - this->set(std::forward(value)); + SocketValueVariant *value_variant = new (ptr) SocketValueVariant(); + value_variant->set(std::forward(value)); + return *value_variant; +} + +template inline SocketValueVariant SocketValueVariant::From(T &&value) +{ + SocketValueVariant value_variant; + value_variant.set(std::forward(value)); + return value_variant; } template inline void SocketValueVariant::set(T &&value) diff --git a/source/blender/blenkernel/intern/bake_items_socket.cc b/source/blender/blenkernel/intern/bake_items_socket.cc index daa9226dbe3..499b206decf 100644 --- a/source/blender/blenkernel/intern/bake_items_socket.cc +++ b/source/blender/blenkernel/intern/bake_items_socket.cc @@ -238,7 +238,7 @@ Array> move_socket_values_to_bake_items(const Span attribute_field = make_attribute_field(base_type); r_attribute_map.add(item->name(), attribute_field->attribute_name()); fn::GField field{attribute_field}; - new (r_value) SocketValueVariant(std::move(field)); + SocketValueVariant::ConstructIn(r_value, std::move(field)); return true; } #ifdef WITH_OPENVDB @@ -251,7 +251,7 @@ Array> move_socket_values_to_bake_items(const Spangrid); + bke::SocketValueVariant::ConstructIn(r_value, std::move(*item->grid)); return true; } return false; @@ -287,7 +287,7 @@ Array> move_socket_values_to_bake_items(const Spangeometry_nodes_cpp_type->destruct(buffer); } - new (r_value) SocketValueVariant(std::move(bundle_ptr)); + bke::SocketValueVariant::ConstructIn(r_value, std::move(bundle_ptr)); return true; } return false; diff --git a/source/blender/nodes/NOD_geometry_exec.hh b/source/blender/nodes/NOD_geometry_exec.hh index 3703cd324cb..5de15634434 100644 --- a/source/blender/nodes/NOD_geometry_exec.hh +++ b/source/blender/nodes/NOD_geometry_exec.hh @@ -186,8 +186,7 @@ class GeoNodeExecParams { { using StoredT = std::decay_t; if constexpr (stored_as_SocketValueVariant_v) { - SocketValueVariant value_variant(std::forward(value)); - this->set_output(identifier, std::move(value_variant)); + this->set_output(identifier, SocketValueVariant::From(std::forward(value))); } else { #ifndef NDEBUG diff --git a/source/blender/nodes/geometry/nodes/node_geo_index_switch.cc b/source/blender/nodes/geometry/nodes/node_geo_index_switch.cc index 332b26a0fc1..1cb3bc6bc10 100644 --- a/source/blender/nodes/geometry/nodes/node_geo_index_switch.cc +++ b/source/blender/nodes/geometry/nodes/node_geo_index_switch.cc @@ -309,7 +309,7 @@ class LazyFunctionForIndexSwitchNode : public LazyFunction { GField output_field(FieldOperation::Create(std::move(switch_fn), std::move(input_fields))); void *output_ptr = params.get_output_data_ptr(0); - new (output_ptr) SocketValueVariant(std::move(output_field)); + SocketValueVariant::ConstructIn(output_ptr, std::move(output_field)); params.output_set(0); } }; diff --git a/source/blender/nodes/geometry/nodes/node_geo_menu_switch.cc b/source/blender/nodes/geometry/nodes/node_geo_menu_switch.cc index 0cd665cd47a..d6d066f081b 100644 --- a/source/blender/nodes/geometry/nodes/node_geo_menu_switch.cc +++ b/source/blender/nodes/geometry/nodes/node_geo_menu_switch.cc @@ -319,7 +319,7 @@ class LazyFunctionForMenuSwitchNode : public LazyFunction { GField output_field{FieldOperation::Create(std::move(multi_function), std::move(item_fields))}; void *output_ptr = params.get_output_data_ptr(0); - new (output_ptr) SocketValueVariant(std::move(output_field)); + SocketValueVariant::ConstructIn(output_ptr, std::move(output_field)); params.output_set(0); } }; diff --git a/source/blender/nodes/geometry/nodes/node_geo_switch.cc b/source/blender/nodes/geometry/nodes/node_geo_switch.cc index 395dc113474..ee254f12fb8 100644 --- a/source/blender/nodes/geometry/nodes/node_geo_switch.cc +++ b/source/blender/nodes/geometry/nodes/node_geo_switch.cc @@ -179,7 +179,7 @@ class LazyFunctionForSwitchNode : public LazyFunction { {std::move(condition), std::move(false_field), std::move(true_field)})}; void *output_ptr = params.get_output_data_ptr(0); - new (output_ptr) SocketValueVariant(std::move(output_field)); + SocketValueVariant::ConstructIn(output_ptr, std::move(output_field)); params.output_set(0); } diff --git a/source/blender/nodes/intern/geometry_nodes_closure_zone.cc b/source/blender/nodes/intern/geometry_nodes_closure_zone.cc index 90b521a4b18..ef305a6eb3d 100644 --- a/source/blender/nodes/intern/geometry_nodes_closure_zone.cc +++ b/source/blender/nodes/intern/geometry_nodes_closure_zone.cc @@ -139,7 +139,8 @@ class LazyFunctionForClosureZone : public LazyFunction { params.set_output(zone_info_.indices.outputs.border_link_usages[i], true); } if (!U.experimental.use_bundle_and_closure_nodes) { - params.set_output(zone_info_.indices.outputs.main[0], bke::SocketValueVariant(ClosurePtr())); + params.set_output(zone_info_.indices.outputs.main[0], + bke::SocketValueVariant::From(ClosurePtr())); return; } @@ -256,7 +257,7 @@ class LazyFunctionForClosureZone : public LazyFunction { std::make_shared())}; params.set_output(zone_info_.indices.outputs.main[0], - bke::SocketValueVariant(std::move(closure))); + bke::SocketValueVariant::From(std::move(closure))); } }; diff --git a/source/blender/nodes/intern/geometry_nodes_execute.cc b/source/blender/nodes/intern/geometry_nodes_execute.cc index 71a5da61343..7cab42ce86c 100644 --- a/source/blender/nodes/intern/geometry_nodes_execute.cc +++ b/source/blender/nodes/intern/geometry_nodes_execute.cc @@ -565,7 +565,7 @@ static void init_socket_cpp_value_from_property(const IDProperty &property, } case SOCK_STRING: { std::string value = IDP_String(&property); - new (r_value) bke::SocketValueVariant(std::move(value)); + bke::SocketValueVariant::ConstructIn(r_value, std::move(value)); break; } case SOCK_MENU: { @@ -662,14 +662,14 @@ static void initialize_group_input(const bNodeTree &tree, if (attribute_name && bke::allow_procedural_attribute_access(*attribute_name)) { fn::GField attribute_field = bke::AttributeFieldInput::Create(*attribute_name, *typeinfo->base_cpp_type); - new (r_value) bke::SocketValueVariant(std::move(attribute_field)); + bke::SocketValueVariant::ConstructIn(r_value, std::move(attribute_field)); } else if (is_layer_selection_field(io_input)) { const IDProperty *property_layer_name = properties.lookup_key_as(io_input.identifier); StringRef layer_name = IDP_String(property_layer_name); - const fn::GField selection_field( - std::make_shared(layer_name), 0); - new (r_value) bke::SocketValueVariant(std::move(selection_field)); + fn::GField selection_field(std::make_shared(layer_name), + 0); + bke::SocketValueVariant::ConstructIn(r_value, std::move(selection_field)); } else { init_socket_cpp_value_from_property(*property, socket_data_type, r_value); diff --git a/source/blender/nodes/intern/geometry_nodes_foreach_geometry_element_zone.cc b/source/blender/nodes/intern/geometry_nodes_foreach_geometry_element_zone.cc index 536066b92d1..c702b50cc73 100644 --- a/source/blender/nodes/intern/geometry_nodes_foreach_geometry_element_zone.cc +++ b/source/blender/nodes/intern/geometry_nodes_foreach_geometry_element_zone.cc @@ -916,8 +916,7 @@ void LazyFunctionForReduceForeachGeometryElement::handle_main_items_and_geometry *base_cpp_type, make_anonymous_attribute_socket_inspection_string( parent_.output_bnode_.output_socket(parent_.indices_.main.bsocket_outer[item_i]))); - SocketValueVariant attribute_value_variant{GField(std::move(attribute_field))}; - params.set_output(1 + item_i, std::move(attribute_value_variant)); + params.set_output(1 + item_i, SocketValueVariant::From(GField(std::move(attribute_field)))); } /* Output the original geometry with potentially additional attributes. */ @@ -1183,9 +1182,8 @@ void LazyFunctionForReduceForeachGeometryElement::handle_generation_items_group( base_cpp_type, make_anonymous_attribute_socket_inspection_string( parent_.output_bnode_.output_socket(2 + node_storage.main_items.items_num + item_i))); - SocketValueVariant attribute_value_variant{GField(std::move(attribute_field))}; params.set_output(parent_.indices_.generation.lf_outer[item_i], - std::move(attribute_value_variant)); + bke::SocketValueVariant::From(GField(std::move(attribute_field)))); } } diff --git a/source/blender/nodes/intern/geometry_nodes_lazy_function.cc b/source/blender/nodes/intern/geometry_nodes_lazy_function.cc index 89a068ac5d9..052d6a8920e 100644 --- a/source/blender/nodes/intern/geometry_nodes_lazy_function.cc +++ b/source/blender/nodes/intern/geometry_nodes_lazy_function.cc @@ -303,7 +303,7 @@ class LazyFunctionForGeometryNode : public LazyFunction { std::move(socket_inspection_name)); void *r_value = params.get_output_data_ptr(lf_index); - new (r_value) SocketValueVariant(GField(std::move(attribute_field))); + SocketValueVariant::ConstructIn(r_value, GField(std::move(attribute_field))); params.output_set(lf_index); } diff --git a/source/blender/nodes/intern/node_declaration.cc b/source/blender/nodes/intern/node_declaration.cc index 54f66da0a63..f9794546010 100644 --- a/source/blender/nodes/intern/node_declaration.cc +++ b/source/blender/nodes/intern/node_declaration.cc @@ -916,40 +916,44 @@ namespace implicit_field_inputs { static void position(const bNode & /*node*/, void *r_value) { - new (r_value) bke::SocketValueVariant(bke::AttributeFieldInput::Create("position")); + bke::SocketValueVariant::ConstructIn(r_value, + bke::AttributeFieldInput::Create("position")); } static void normal(const bNode & /*node*/, void *r_value) { - new (r_value) - bke::SocketValueVariant(fn::Field(std::make_shared())); + bke::SocketValueVariant::ConstructIn( + r_value, fn::Field(std::make_shared())); } static void index(const bNode & /*node*/, void *r_value) { - new (r_value) bke::SocketValueVariant(fn::Field(std::make_shared())); + bke::SocketValueVariant::ConstructIn(r_value, + fn::Field(std::make_shared())); } static void id_or_index(const bNode & /*node*/, void *r_value) { - new (r_value) - bke::SocketValueVariant(fn::Field(std::make_shared())); + bke::SocketValueVariant::ConstructIn( + r_value, fn::Field(std::make_shared())); } static void instance_transform(const bNode & /*node*/, void *r_value) { - new (r_value) - bke::SocketValueVariant(bke::AttributeFieldInput::Create("instance_transform")); + bke::SocketValueVariant::ConstructIn( + r_value, bke::AttributeFieldInput::Create("instance_transform")); } static void handle_left(const bNode & /*node*/, void *r_value) { - new (r_value) bke::SocketValueVariant(bke::AttributeFieldInput::Create("handle_left")); + bke::SocketValueVariant::ConstructIn(r_value, + bke::AttributeFieldInput::Create("handle_left")); } static void handle_right(const bNode & /*node*/, void *r_value) { - new (r_value) bke::SocketValueVariant(bke::AttributeFieldInput::Create("handle_right")); + bke::SocketValueVariant::ConstructIn(r_value, + bke::AttributeFieldInput::Create("handle_right")); } } // namespace implicit_field_inputs diff --git a/source/blender/nodes/intern/node_socket.cc b/source/blender/nodes/intern/node_socket.cc index 24d8b2dadd0..cc2db3980b2 100644 --- a/source/blender/nodes/intern/node_socket.cc +++ b/source/blender/nodes/intern/node_socket.cc @@ -995,9 +995,9 @@ static bke::bNodeSocketType *make_socket_type_bundle() }; socktype->geometry_nodes_cpp_type = &blender::CPPType::get(); socktype->get_geometry_nodes_cpp_value = [](const void * /*socket_value*/, void *r_value) { - new (r_value) SocketValueVariant(nodes::BundlePtr()); + SocketValueVariant::ConstructIn(r_value, nodes::BundlePtr()); }; - static SocketValueVariant default_value{nodes::BundlePtr()}; + static SocketValueVariant default_value = SocketValueVariant::From(nodes::BundlePtr()); socktype->geometry_nodes_default_cpp_value = &default_value; return socktype; } @@ -1011,9 +1011,9 @@ static bke::bNodeSocketType *make_socket_type_closure() }; socktype->geometry_nodes_cpp_type = &blender::CPPType::get(); socktype->get_geometry_nodes_cpp_value = [](const void * /*socket_value*/, void *r_value) { - new (r_value) SocketValueVariant(nodes::ClosurePtr()); + SocketValueVariant::ConstructIn(r_value, nodes::ClosurePtr()); }; - static SocketValueVariant default_value{nodes::ClosurePtr()}; + static SocketValueVariant default_value = SocketValueVariant::From(nodes::ClosurePtr()); socktype->geometry_nodes_default_cpp_value = &default_value; return socktype; }