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
This commit is contained in:
Jacques Lucke
2025-03-06 09:47:44 +01:00
parent 093178342d
commit e726357962
3 changed files with 29 additions and 11 deletions

View File

@@ -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<typename CreateValueF>
Value &lookup_or_add_cb(const Key &key, const CreateValueF &create_value)

View File

@@ -763,6 +763,20 @@ TEST(map, Equality)
EXPECT_NE(a, b);
}
TEST(map, AddCbMove)
{
Map<std::string, int> 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.
*/

View File

@@ -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<lf::OutputSocket *> usages,
lf::OutputSocket *or_socket_usages(const Span<lf::OutputSocket *> 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<LazyFunctionForLogicalOr>(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<lf::OutputSocket *> 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<LazyFunctionForLogicalOr>(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)