Fix #106208: data-block socket defaults not used for node group #107179

Jacques Lucke · 2023-04-20T16:35:06+02:00

Jacques Lucke commented

2023-04-20 16:35:06 +02:00

This is an alternative fix to #106816.

The main challenge is to avoid dangling pointers. Currently, the lifetime of socket declarations is somewhat unbounded (at least we didn't restrict it explicitly yet). Therefore, storing non-owning pointers in it tricky. For ID pointers one could potentially use the foreach-id iterator to update pointers in declarations as well, but that's a bit out of scope and might not be the right solution anyway, since it's not obvious that all node declarations are reachable from IDs stored in bmain.

The solution now is to use a callback that retrieves the right ID pointer when it is used. The important thing is that the callback does not capture any potentially dangling pointer either.

This is an alternative fix to #106816. The main challenge is to avoid dangling pointers. Currently, the lifetime of socket declarations is somewhat unbounded (at least we didn't restrict it explicitly yet). Therefore, storing non-owning pointers in it tricky. For ID pointers one could potentially use the foreach-id iterator to update pointers in declarations as well, but that's a bit out of scope and might not be the right solution anyway, since it's not obvious that all node declarations are reachable from IDs stored in `bmain`. The solution now is to use a callback that retrieves the right ID pointer when it is used. The important thing is that the callback does not capture any potentially dangling pointer either.

Jacques Lucke added 1 commit 2023-04-20 16:35:16 +02:00

e96d254e7c fix

Iliya Katushenock reviewed 2023-04-20 16:43:05 +02:00

source/blender/nodes/intern/node_socket_declarations.cc Outdated

						
				@ -438,0 +440,4 @@

				    ID *id = this->default_value_fn(node);

				    /* Assumes that all ID sockets like #bNodeSocketValueObject and #bNodeSocketValueImage have the

				     * ID pointer at the start of the struct. */

				    *(ID **)socket.default_value = id;

Iliya Katushenock commented

2023-04-20 16:43:05 +02:00

C-style cast here

Jacques Lucke requested review from Hans Goudey 2023-04-20 16:53:13 +02:00

Iliya Katushenock referenced this pull request

2023-04-20 16:53:14 +02:00

Fix #106208: Initialize socket value by declaration callback if value is data block #106816

Jacques Lucke added 1 commit 2023-04-20 16:54:38 +02:00

26b61db9fb use static cast

Hans Goudey approved these changes 2023-04-20 21:30:47 +02:00

Hans Goudey left a comment

It's not pretty but I think it makes sense, thanks.

I guess the alternative is including these default ID pointers in node_foreach_id so they will be remapped? I don't have a preference either way really, just checking my understanding.

It's not pretty but I think it makes sense, thanks. I guess the alternative is including these default ID pointers in `node_foreach_id` so they will be remapped? I don't have a preference either way really, just checking my understanding.

source/blender/nodes/NOD_socket_declarations.hh Outdated

						
				@ -158,6 +158,8 @@ class StringBuilder : public SocketDeclarationBuilder<String> {

				class IDSocketDeclaration : public SocketDeclaration {

				 public:

				  const char *idname;

				  /** Get the default ID pointer for this socket. This is a function to avoid dangling pointers. */

Hans Goudey commented

2023-04-20 21:26:24 +02:00

Maybe add , since bNode::idpointers are remapped asID pointers change, but pointers in socket declarations are not managed the same way.

Maybe add `, since `bNode::id` pointers are remapped as `ID` pointers change, but pointers in socket declarations are not managed the same way.`

source/blender/nodes/intern/node_common.cc Outdated

						
				@ -128,0 +140,4 @@

				    const bNodeTree &ntree = *reinterpret_cast<const bNodeTree *>(node.id);

				    const bNodeSocket *io_socket;

				    if (in_out == SOCK_IN) {

				      if (socket_index < ntree.interface_inputs().size()) {

Hans Goudey commented

2023-04-20 21:28:48 +02:00

Hmm, why would the socket index be out of bounds here? Does this function object live past some changes to the group interface? If so, maybe worth noting that in a comment.

Jacques Lucke commented

2023-04-20 22:14:03 +02:00

The problem is, I don't think we know for sure how long this will live currently. At least I don't know.

Jacques Lucke commented

2023-04-20 22:10:22 +02:00

I guess the alternative is including these default ID pointers in node_foreach_id so they will be remapped? I don't have a preference either way really, just checking my understanding.

I mentioned that in the PR description.

> I guess the alternative is including these default ID pointers in node_foreach_id so they will be remapped? I don't have a preference either way really, just checking my understanding. I mentioned that in the PR description.

Jacques Lucke added 2 commits 2023-04-20 22:15:08 +02:00

a102264a1a Merge branch 'main' into node-group-default-id

6a48b05f63 improve comments

Hans Goudey reviewed 2023-04-20 22:16:30 +02:00

source/blender/nodes/intern/node_common.cc Outdated

						
				@ -128,0 +140,4 @@

				    const bNodeTree &ntree = *reinterpret_cast<const bNodeTree *>(node.id);

				    const bNodeSocket *io_socket;

				    if (in_out == SOCK_IN) {

				      /* Better be save than sorry when the underlying node group changed. */