Support for simulation zones in copy operators #106812

Lukas Tönne · 2023-04-11T15:48:47+02:00

Lukas Tönne commented

2023-04-11 15:48:47 +02:00

Copying a simulation zone should keep the 1:1 pairing intact (see remap_pairing functions)

When copying a simulation input node on its own, unpair it to avoid ambiguity. Only the old simulation input node is paired to the output.
When copying a simulation output node on its own, no special action is needed - the node gets a new ID and nothing is paired with it.
When copying both input and output, remap the output_node_id property of the simulation input node, so that it is paired with the output copy.

There are a couple of places where copies happen:

Node tree copy
Duplicate nodes
Group Separate (copies nodes from the group tree into another tree)
Clipboard (both copy to clipboard and paste into node tree)
Shader node tree branch copy for execution

These copy operators do mostly the same thing, but in slightly different ways, which makes the code incompatible (e.g. using a Map<const bNode *, bNode *> node_map vs. Map<bNode *, bNode *> node_map). That's why there are 3 remap_pairing implementations.

Dynamic node declarations are problematic:
Copying nodes invokes nodeDeclarationEnsure to generate declarations for new nodes. It does not, however, change the socket lists. If a dynamic declaration for a node copy alters the sockets (in this case: remove all because the node is unpaired), the subsequent update_socket_declarations will crash because it expects sockets to match the declaration.
At the end of operators there is usually a BKE_ntree_update_main or similar, which invokes update_node_declaration_and_sockets. This method does update the socket lists as well (see #106732), but only if a node is tagged for a respective update.

The solution here is to use update_node_declaration_and_sockets for dynamic declarations instead of just build_node_declaration_dynamic.

Fixes #106732

Copying a simulation zone should keep the 1:1 pairing intact (see `remap_pairing` functions) - When copying a simulation input node on its own, unpair it to avoid ambiguity. Only the old simulation input node is paired to the output. - When copying a simulation output node on its own, no special action is needed - the node gets a new ID and nothing is paired with it. - When copying both input and output, remap the `output_node_id` property of the simulation input node, so that it is paired with the output copy. There are a couple of places where copies happen: * Node tree copy * Duplicate nodes * Group Separate (copies nodes from the group tree into another tree) * Clipboard (both copy to clipboard and paste into node tree) * Shader node tree branch copy for execution These copy operators do mostly the same thing, but in slightly different ways, which makes the code incompatible (e.g. using a `Map<const bNode *, bNode *> node_map` vs. `Map<bNode *, bNode *> node_map`). That's why there are 3 `remap_pairing` implementations. Dynamic node declarations are problematic: Copying nodes invokes `nodeDeclarationEnsure` to generate declarations for new nodes. It does not, however, change the socket lists. If a dynamic declaration for a node copy alters the sockets (in this case: remove all because the node is unpaired), the subsequent `update_socket_declarations` will crash because it expects sockets to match the declaration. At the end of operators there is usually a `BKE_ntree_update_main` or similar, which invokes `update_node_declaration_and_sockets`. This method _does_ update the socket lists as well (see #106732), but only if a node is tagged for a respective update. The solution here is to use `update_node_declaration_and_sockets` for dynamic declarations instead of just `build_node_declaration_dynamic`. Fixes #106732

Lukas Tönne added the

Module

Nodes & Physics

label 2023-04-11 15:48:47 +02:00

Lukas Tönne added 4 commits 2023-04-11 15:49:02 +02:00

08d171734c Support for basic copying inside the same node tree.

1c60fddf4e Merge branch 'geometry-nodes-simulation' into geometry-nodes-simulation-copy-support

b43a85ce5f Move calls to `nodeDeclarationEnsure` past link remapping.

This is necessary because link remapping uses the `socket_map`. If the
declaration removes sockets this `socket_map` becomes invalid. So
updating the declaration may only happen _after_ any references to
previous sockets are no longer needed.

e6c71cb989 Perform a full socket update when updating dynamic declarations.

This is to ensure that dynamic declarations can actually change the
socket layout outside of operators that explicitly call
`update_node_declaration_and_sockets`.

Lukas Tönne added this to the Nodes & Physics project 2023-04-11 15:49:13 +02:00

Lukas Tönne reviewed 2023-04-11 15:57:59 +02:00

source/blender/blenkernel/intern/node.cc Outdated

						
				@ -3819,3 +3819,2 @@

				    BLI_assert(node != nullptr);

				    node->runtime->declaration = new blender::nodes::NodeDeclaration();

				    blender::nodes::build_node_declaration_dynamic(*ntree, *node, *node->runtime->declaration);

				    blender::nodes::update_node_declaration_and_sockets(*ntree, *node);

Lukas Tönne commented

2023-04-11 15:50:07 +02:00

This is to ensure sockets are updated to match dynamic declarations.

source/blender/editors/space_node/clipboard.cc Outdated

						
				@ -237,10 +266,6 @@ static int node_clipboard_paste_exec(bContext *C, wmOperator *op)

				    }

				  }

				  for (bNode *new_node : node_map.values()) {

Lukas Tönne commented

2023-04-11 15:56:02 +02:00

Changing socket layout too early causes the socket_map to become invalid. So this section has been moved down beyond the link remapping section.

This section was originally in node_copy_with_mapping (see f36dd0660983). It was moved out of the primary node duplication loop so that the simulation input could build its declaration based on the copy of the output node.

Same applies in "Duplicate" and "Group Separate" operators below.

Changing socket layout too early causes the `socket_map` to become invalid. So this section has been moved down beyond the link remapping section. This section was originally in `node_copy_with_mapping` (see [f36dd0660983](https://projects.blender.org/blender/blender/commit/f36dd066098311c6ec77ca144e5bb1b2bca252c1)). It was moved out of the primary node duplication loop so that the simulation input could build its declaration based on the copy of the output node. Same applies in "Duplicate" and "Group Separate" operators below.

Lukas Tönne requested review from Jacques Lucke 2023-04-11 15:58:48 +02:00

Lukas Tönne requested review from Hans Goudey 2023-04-11 15:58:48 +02:00

Lukas Tönne commented

2023-04-11 16:05:54 +02:00

"Group Separate" operator is technically not correct because it copies nodes from the group tree, then potentially updates their ID values, then uses the old ID values to reconstruct links in the new tree. In practice this is unlikely to cause problems because IDs are random uint32_t numbers and almost never collide, so the copied nodes will almost always have the same IDs as the original nodes.

"Make Group" and "Ungroup" operators will currently unpair simulation nodes because they simply move the nodes to a different tree. Then the nodes get new random IDs, which means the output_node_id will almost certainly be incorrect, effectively un-pairing the simulation nodes.

"Group Separate" operator is technically not correct because it copies nodes from the group tree, then potentially updates their ID values, then uses the _old_ ID values to reconstruct links in the _new_ tree. In practice this is unlikely to cause problems because IDs are random `uint32_t` numbers and almost never collide, so the copied nodes will almost always have the same IDs as the original nodes. "Make Group" and "Ungroup" operators will currently unpair simulation nodes because they simply move the nodes to a different tree. Then the nodes get new random IDs, which means the `output_node_id` will almost certainly be incorrect, effectively un-pairing the simulation nodes.

Jacques Lucke requested changes 2023-04-11 17:23:00 +02:00

Jacques Lucke left a comment

Worked perfectly in all my tests. Feels much better now than before :)

source/blender/editors/space_node/clipboard.cc Outdated

						
				@ -185,0 +188,4 @@

				{

				  /* We don't have the old tree for looking up output nodes by ID,

				   * so have to build a map first to find copied output nodes in the new tree. */

				  Map<uint32_t, bNode *> dst_output_node_map;

Jacques Lucke commented

2023-04-11 17:20:49 +02:00

Use int32_t instead of uint32_t for node identifiers.

Use `int32_t` instead of `uint32_t` for node identifiers.

Lukas Tönne commented

2023-04-12 11:51:25 +02:00

Oh, somehow thought the identifiers were uint32_t, my bad.

Oh, somehow thought the identifiers were `uint32_t`, my bad.

LukasTonne marked this conversation as resolved

source/blender/editors/space_node/node_group.cc

						
				@ -439,0 +444,4 @@

				          dst_node->storage);

				      /* XXX Technically this is not correct because the output_node_id is only valid

				       * in the original node group tree and we'd have map old IDs to new nodes first.

				       * The ungroup operator does not build a node map, it just expects node IDs to

Jacques Lucke commented

2023-04-11 17:22:34 +02:00

Based on the comment it sounds like the proper solution would be to build the node map. Is there a reason for not just doing that instead of using a solution you already know might not work?

Lukas Tönne commented

2023-04-12 12:23:03 +02:00

I'd consider that a separate task, to fix the group separate operator. Ideally would also unify all the other copy operators. It is extremely unlikely this would cause problems in practice because of the randomness of identifiers. Relying on randomness and expecting that the identifier method remains unchanged is bad of course.

I've added a new task for this #106852

I'd consider that a separate task, to fix the group separate operator. Ideally would also unify all the other copy operators. It is extremely unlikely this would cause problems in practice because of the randomness of identifiers. Relying on randomness and expecting that the identifier method remains unchanged is bad of course. I've added a new task for this #106852

Lukas Tönne added 1 commit 2023-04-12 12:23:51 +02:00

ca69bd2878 Use int32_t in the clipboard remap_pairing function.

Jacques Lucke approved these changes 2023-04-13 20:04:57 +02:00

Jacques Lucke left a comment

The patch still uses uint32_t in two places (one of those is in a comment), that should still be fixed.

The patch still uses `uint32_t` in two places (one of those is in a comment), that should still be fixed.

Hans Goudey requested changes 2023-04-14 04:00:41 +02:00

Hans Goudey left a comment

I'm not sure how I feel about adding three implementations of remap_pairing. It seems like they could be unified by creating a Map<int32_t, int32_t> when copying node groups instead of the various set/map types we currently use. If you plan to work on #106852 in the near future, we could still commit this now, but otherwise I think it would make more sense for that unification to happen first, better not to leave it like this longer term.

I'm not sure how I feel about adding three implementations of `remap_pairing`. It seems like they could be unified by creating a `Map<int32_t, int32_t>` when copying node groups instead of the various set/map types we currently use. If you plan to work on #106852 in the near future, we could still commit this now, but otherwise I think it would make more sense for that unification to happen first, better not to leave it like this longer term.

👍 1

Lukas Tönne commented

2023-04-14 08:09:43 +02:00

otherwise I think it would make more sense for that unification to happen first, better not to leave it like this longer term.

I'll give the copy unification #106852 a go and see if i run into any major obstacles.

> otherwise I think it would make more sense for that unification to happen first, better not to leave it like this longer term. I'll give the copy unification #106852 a go and see if i run into any major obstacles.

❤️ 1

Lukas Tönne added 1 commit 2023-04-17 14:46:12 +02:00

f6884664a3 Merge branch 'geometry-nodes-simulation' into geometry-nodes-simulation-copy-support

Hans Goudey approved these changes 2023-04-17 15:35:08 +02:00

Lukas Tönne merged commit 315cc66bd8 into geometry-nodes-simulation

2023-04-17 16:11:09 +02:00

Lukas Tönne deleted branch geometry-nodes-simulation-copy-support

2023-04-17 16:11:10 +02:00

Lukas Tönne referenced this issue from a commit

2023-04-17 16:11:10 +02:00

Fix #106732: Support for simulation zones in copy operators

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Support for simulation zones in copy operators #106812