From e72635796288ab56f143aa61d95354d4e4db9123 Mon Sep 17 00:00:00 2001 From: Jacques Lucke Date: Thu, 6 Mar 2025 09:47:44 +0100 Subject: [PATCH] Fix: Geometry Nodes: bad attribute propagation with multiple group outputs and warning node The issue was that sometimes the group inputs of a node group were shuffled around unexpectedly and thus inputs were passed to the wrong sockets. The `or_socket_usages` function sorts the given span so that the key is more likely to be reused, reducing the number of nodes inserted in the graph. The issue was that `build_warning_node` passes `group_output_used_sockets_` into the function the order of which is important. It thus should not be reordered. The fix is to just never reorder the span passed to `or_socket_usages` but to make a local copy instead which can be sorted without problems. Often this copy is done already anyway when the span is inserted into `graph_params.socket_usages_combination_cache` as `Vector`. This fix also makes an assumption about `Map.lookup_or_add_cb` which was not documented before. Namely it assumes that the key is moved into the map only after the callback has been called. This behavior is now documented and there is a unit test for it. Pull Request: https://projects.blender.org/blender/blender/pulls/135528 --- source/blender/blenlib/BLI_map.hh | 3 ++- source/blender/blenlib/tests/BLI_map_test.cc | 14 +++++++++++ .../intern/geometry_nodes_lazy_function.cc | 23 +++++++++++-------- 3 files changed, 29 insertions(+), 11 deletions(-) diff --git a/source/blender/blenlib/BLI_map.hh b/source/blender/blenlib/BLI_map.hh index 8c6dbe788bc..f5588c37710 100644 --- a/source/blender/blenlib/BLI_map.hh +++ b/source/blender/blenlib/BLI_map.hh @@ -613,7 +613,8 @@ class Map { * the map, it will be newly added. * * The create_value callback is only called when the key did not exist yet. It is expected to - * take no parameters and return the value to be inserted. + * take no parameters and return the value to be inserted. The callback is called before the key + * is copied/moved into the map. */ template Value &lookup_or_add_cb(const Key &key, const CreateValueF &create_value) diff --git a/source/blender/blenlib/tests/BLI_map_test.cc b/source/blender/blenlib/tests/BLI_map_test.cc index 85381c81eee..e38347af598 100644 --- a/source/blender/blenlib/tests/BLI_map_test.cc +++ b/source/blender/blenlib/tests/BLI_map_test.cc @@ -763,6 +763,20 @@ TEST(map, Equality) EXPECT_NE(a, b); } +TEST(map, AddCbMove) +{ + Map map; + std::string value = "a"; + bool value_checked = false; + map.lookup_or_add_cb(std::move(value), [&]() { + EXPECT_EQ(value, "a"); + value_checked = true; + return 10; + }); + EXPECT_TRUE(value_checked); + EXPECT_EQ(value, ""); +} + /** * Set this to 1 to activate the benchmark. It is disabled by default, because it prints a lot. */ diff --git a/source/blender/nodes/intern/geometry_nodes_lazy_function.cc b/source/blender/nodes/intern/geometry_nodes_lazy_function.cc index 9392c44883b..482aa2c8733 100644 --- a/source/blender/nodes/intern/geometry_nodes_lazy_function.cc +++ b/source/blender/nodes/intern/geometry_nodes_lazy_function.cc @@ -3786,7 +3786,7 @@ struct GeometryNodesLazyFunctionBuilder { * Combine multiple socket usages with a logical or. Inserts a new node for that purpose if * necessary. */ - lf::OutputSocket *or_socket_usages(MutableSpan usages, + lf::OutputSocket *or_socket_usages(const Span usages, BuildGraphParams &graph_params) { if (usages.is_empty()) { @@ -3796,16 +3796,19 @@ struct GeometryNodesLazyFunctionBuilder { return usages[0]; } - std::sort(usages.begin(), usages.end()); - return graph_params.socket_usages_combination_cache.lookup_or_add_cb_as(usages, [&]() { - auto &logical_or_fn = scope_.construct(usages.size()); - lf::Node &logical_or_node = graph_params.lf_graph.add_function(logical_or_fn); + /* Sort usages to produce a deterministic key for the same set of sockets. */ + Vector usages_sorted(usages); + std::sort(usages_sorted.begin(), usages_sorted.end()); + return graph_params.socket_usages_combination_cache.lookup_or_add_cb( + std::move(usages_sorted), [&]() { + auto &logical_or_fn = scope_.construct(usages.size()); + lf::Node &logical_or_node = graph_params.lf_graph.add_function(logical_or_fn); - for (const int i : usages.index_range()) { - graph_params.lf_graph.add_link(*usages[i], logical_or_node.input(i)); - } - return &logical_or_node.output(0); - }); + for (const int i : usages_sorted.index_range()) { + graph_params.lf_graph.add_link(*usages_sorted[i], logical_or_node.input(i)); + } + return &logical_or_node.output(0); + }); } void build_output_socket_usages(const bNode &bnode, BuildGraphParams &graph_params)