[Bf-blender-cvs] [b998a7b384c] master: Fix T64247: Crash on playback with special shader node tree
Sergey Sharybin
noreply at git.blender.org
Tue Jun 4 09:39:59 CEST 2019
Commit: b998a7b384c6a598ea851fbc06a2df8829c34329
Author: Sergey Sharybin
Date: Mon Jun 3 17:08:25 2019 +0200
Branches: master
https://developer.blender.org/rBb998a7b384c6a598ea851fbc06a2df8829c34329
Fix T64247: Crash on playback with special shader node tree
The root of the problem goes to the fact that node tree copying
uses source tree and nodes for a temporary storage.
This makes it so multiple dependency graphs can not be reliably
evaluated from different threads if they are using same original
node tree.
Solved by doing the following:
- Commonly used tree copying function (which is used by library
manager) keeps source tree, nodes and sockets untouched.
- All the related areas (like node tree's callback) now have
const qualifier on the input.
- Areas which needs to have those temporary pointers assigned are
now using explicit function.
Would be really cool to get rid of those temporary pointers
completely, but this is a bit tricky due to hairy nature of the
code. Can happen any time now though: is easy enough to generalize
the new pointers mapping.
Note that this change is only intended to solve the crash.
The fact that icons shouldn't be updated on playback will be fixed
as a separate change.
Reviewers: brecht, fclem
Reviewed By: brecht, fclem
Subscribers: brecht, fclem
Differential Revision: https://developer.blender.org/D5002
===================================================================
M source/blender/blenkernel/BKE_node.h
M source/blender/blenkernel/intern/node.c
M source/blender/editors/space_node/node_edit.c
M source/blender/editors/space_node/node_group.c
M source/blender/makesrna/intern/rna_nodetree.c
M source/blender/nodes/composite/node_composite_tree.c
M source/blender/nodes/composite/nodes/node_composite_cryptomatte.c
M source/blender/nodes/composite/nodes/node_composite_image.c
M source/blender/nodes/composite/nodes/node_composite_moviedistortion.c
M source/blender/nodes/composite/nodes/node_composite_outputFile.c
M source/blender/nodes/intern/node_util.c
M source/blender/nodes/intern/node_util.h
M source/blender/nodes/shader/nodes/node_shader_script.c
M source/blender/nodes/shader/nodes/node_shader_tex_pointdensity.c
M source/blender/nodes/texture/nodes/node_texture_output.c
===================================================================
diff --git a/source/blender/blenkernel/BKE_node.h b/source/blender/blenkernel/BKE_node.h
index b760be29dae..fb6096cd82f 100644
--- a/source/blender/blenkernel/BKE_node.h
+++ b/source/blender/blenkernel/BKE_node.h
@@ -225,12 +225,14 @@ typedef struct bNodeType {
/// Free the node instance.
void (*freefunc)(struct bNode *node);
/// Make a copy of the node instance.
- void (*copyfunc)(struct bNodeTree *dest_ntree, struct bNode *dest_node, struct bNode *src_node);
+ void (*copyfunc)(struct bNodeTree *dest_ntree,
+ struct bNode *dest_node,
+ const struct bNode *src_node);
/* Registerable API callback versions, called in addition to C callbacks */
void (*initfunc_api)(const struct bContext *C, struct PointerRNA *ptr);
void (*freefunc_api)(struct PointerRNA *ptr);
- void (*copyfunc_api)(struct PointerRNA *ptr, struct bNode *src_node);
+ void (*copyfunc_api)(struct PointerRNA *ptr, const struct bNode *src_node);
/* can this node type be added to a node tree */
bool (*poll)(struct bNodeType *ntype, struct bNodeTree *nodetree);
@@ -538,7 +540,20 @@ void nodeRemoveNode(struct Main *bmain,
struct bNode *node,
bool do_id_user);
-struct bNode *BKE_node_copy_ex(struct bNodeTree *ntree, struct bNode *node_src, const int flag);
+struct bNode *BKE_node_copy_ex(struct bNodeTree *ntree,
+ const struct bNode *node_src,
+ const int flag);
+
+/* Same as BKE_node_copy_ex() but stores pointers to a new node and its sockets in the source
+ * node.
+ *
+ * NOTE: DANGER ZONE!
+ *
+ * TODO(sergey): Maybe it's better to make BKE_node_copy_ex() return a mapping from old node and
+ * sockets to new one. */
+struct bNode *BKE_node_copy_store_new_pointers(struct bNodeTree *ntree,
+ struct bNode *node_src,
+ const int flag);
struct bNodeLink *nodeAddLink(struct bNodeTree *ntree,
struct bNode *fromnode,
@@ -730,7 +745,7 @@ void node_type_storage(struct bNodeType *ntype,
void (*freefunc)(struct bNode *node),
void (*copyfunc)(struct bNodeTree *dest_ntree,
struct bNode *dest_node,
- struct bNode *src_node));
+ const struct bNode *src_node));
void node_type_label(
struct bNodeType *ntype,
void (*labelfunc)(struct bNodeTree *ntree, struct bNode *, char *label, int maxlen));
diff --git a/source/blender/blenkernel/intern/node.c b/source/blender/blenkernel/intern/node.c
index b7db434e234..d8b63dac99d 100644
--- a/source/blender/blenkernel/intern/node.c
+++ b/source/blender/blenkernel/intern/node.c
@@ -41,6 +41,7 @@
#include "DNA_linestyle_types.h"
#include "BLI_listbase.h"
+#include "BLI_ghash.h"
#include "BLI_math.h"
#include "BLI_path_util.h"
#include "BLI_string.h"
@@ -1008,10 +1009,8 @@ bNode *nodeAddStaticNode(const struct bContext *C, bNodeTree *ntree, int type)
return nodeAddNode(C, ntree, idname);
}
-static void node_socket_copy(bNodeSocket *sock_dst, bNodeSocket *sock_src, const int flag)
+static void node_socket_copy(bNodeSocket *sock_dst, const bNodeSocket *sock_src, const int flag)
{
- sock_src->new_sock = sock_dst;
-
if (sock_src->prop) {
sock_dst->prop = IDP_CopyProperty_ex(sock_src->prop, flag);
}
@@ -1029,7 +1028,7 @@ static void node_socket_copy(bNodeSocket *sock_dst, bNodeSocket *sock_src, const
/* keep socket listorder identical, for copying links */
/* ntree is the target tree */
-bNode *BKE_node_copy_ex(bNodeTree *ntree, bNode *node_src, const int flag)
+bNode *BKE_node_copy_ex(bNodeTree *ntree, const bNode *node_src, const int flag)
{
bNode *node_dst = MEM_callocN(sizeof(bNode), "dupli node");
bNodeSocket *sock_dst, *sock_src;
@@ -1063,10 +1062,17 @@ bNode *BKE_node_copy_ex(bNodeTree *ntree, bNode *node_src, const int flag)
for (link_dst = node_dst->internal_links.first, link_src = node_src->internal_links.first;
link_dst != NULL;
link_dst = link_dst->next, link_src = link_src->next) {
+ /* This is a bit annoying to do index lookups in a list, but is likely to be faster than
+ * trying to create a hash-map. At least for usual nodes, which only have so much sockets
+ * and internal links. */
+ const int from_sock_index = BLI_findindex(&node_src->inputs, link_src->fromsock);
+ const int to_sock_index = BLI_findindex(&node_src->outputs, link_src->tosock);
+ BLI_assert(from_sock_index != -1);
+ BLI_assert(to_sock_index != -1);
link_dst->fromnode = node_dst;
link_dst->tonode = node_dst;
- link_dst->fromsock = link_dst->fromsock->new_sock;
- link_dst->tosock = link_dst->tosock->new_sock;
+ link_dst->fromsock = BLI_findlink(&node_dst->inputs, from_sock_index);
+ link_dst->tosock = BLI_findlink(&node_dst->outputs, to_sock_index);
}
if ((flag & LIB_ID_CREATE_NO_USER_REFCOUNT) == 0) {
@@ -1077,7 +1083,6 @@ bNode *BKE_node_copy_ex(bNodeTree *ntree, bNode *node_src, const int flag)
node_src->typeinfo->copyfunc(ntree, node_dst, node_src);
}
- node_src->new_node = node_dst;
node_dst->new_node = NULL;
bool do_copy_api = !((flag & LIB_ID_CREATE_NO_MAIN) || (flag & LIB_ID_COPY_LOCALIZE));
@@ -1095,6 +1100,30 @@ bNode *BKE_node_copy_ex(bNodeTree *ntree, bNode *node_src, const int flag)
return node_dst;
}
+bNode *BKE_node_copy_store_new_pointers(bNodeTree *ntree, bNode *node_src, const int flag)
+{
+ bNode *new_node = BKE_node_copy_ex(ntree, node_src, flag);
+ /* Store mapping to the node itself. */
+ node_src->new_node = new_node;
+ /* Store mapping to inputs. */
+ bNodeSocket *new_input_sock = new_node->inputs.first;
+ bNodeSocket *input_sock_src = node_src->inputs.first;
+ while (new_input_sock != NULL) {
+ input_sock_src->new_sock = new_input_sock;
+ new_input_sock = new_input_sock->next;
+ input_sock_src = input_sock_src->next;
+ }
+ /* Store mapping to outputs. */
+ bNodeSocket *new_output_sock = new_node->outputs.first;
+ bNodeSocket *output_sock_src = node_src->outputs.first;
+ while (new_output_sock != NULL) {
+ output_sock_src->new_sock = new_output_sock;
+ new_output_sock = new_output_sock->next;
+ output_sock_src = output_sock_src->next;
+ }
+ return new_node;
+}
+
/* also used via rna api, so we check for proper input output direction */
bNodeLink *nodeAddLink(
bNodeTree *ntree, bNode *fromnode, bNodeSocket *fromsock, bNode *tonode, bNodeSocket *tosock)
@@ -1402,23 +1431,45 @@ void BKE_node_tree_copy_data(Main *UNUSED(bmain),
BLI_listbase_clear(&ntree_dst->nodes);
BLI_listbase_clear(&ntree_dst->links);
- for (bNode *node_src = ntree_src->nodes.first; node_src; node_src = node_src->next) {
- BKE_node_copy_ex(ntree_dst, node_src, flag_subdata);
+ /* Since source nodes and sockets are unique pointers we can put everything in a single map. */
+ GHash *new_pointers = BLI_ghash_ptr_new("BKE_node_tree_copy_data");
+
+ for (const bNode *node_src = ntree_src->nodes.first; node_src; node_src = node_src->next) {
+ bNode *new_node = BKE_node_copy_ex(ntree_dst, node_src, flag_subdata);
+ BLI_ghash_insert(new_pointers, (void *)node_src, new_node);
+ /* Store mapping to inputs. */
+ bNodeSocket *new_input_sock = new_node->inputs.first;
+ const bNodeSocket *input_sock_src = node_src->inputs.first;
+ while (new_input_sock != NULL) {
+ BLI_ghash_insert(new_pointers, (void *)input_sock_src, new_input_sock);
+ new_input_sock = new_input_sock->next;
+ input_sock_src = input_sock_src->next;
+ }
+ /* Store mapping to outputs. */
+ bNodeSocket *new_output_sock = new_node->outputs.first;
+ const bNodeSocket *output_sock_src = node_src->outputs.first;
+ while (new_output_sock != NULL) {
+ BLI_ghash_insert(new_pointers, (void *)output_sock_src, new_output_sock);
+ new_output_sock = new_output_sock->next;
+ output_sock_src = output_sock_src->next;
+ }
}
/* copy links */
BLI_duplicatelist(&ntree_dst->links, &ntree_src->links);
for (link_dst = ntree_dst->links.first; link_dst; link_dst = link_dst->next) {
- link_dst->fromnode = (link_dst->fromnode ? link_dst->fromnode->new_node : NULL);
- link_dst->fromsock = (link_dst->fromsock ? link_dst->fromsock->new_sock : NULL);
- link_dst->tonode = (link_dst->tonode ? link_dst->tonode->new_node : NULL);
- link_dst->tosock = (link_dst->tosock ? link_dst->tosock->new_sock : NULL);
+ link_dst->fromnode = BLI_ghash_lookup_default(new_pointers, link_dst->fromnode, NULL);
+ link_dst->fromsock = BLI_ghash_lookup_default(new_pointers, link_dst->fromsock, NULL);
+ link_dst->tonode = BLI_ghash_lookup_default(new_pointers, link_dst->tonode, NULL);
+ link_dst->tosock = BLI_ghash_lookup_default(new_pointers, link_dst->tosock, NULL);
/* update the link socket's pointer */
if (link_dst->tosock) {
link_dst->tosock->link = link_dst;
}
}
+ BLI_ghash_free(new_pointers, NULL, NULL);
+
/* copy interface sockets */
BLI_duplicatelist(&ntree_dst->inputs, &ntree_src->inputs);
for (sock_dst = ntree_dst->inputs.first, sock_src = ntree_src->inputs.first; sock_dst != NULL;
@@ -2201,9 +2252,12 @@ bNodeTree *ntreeLocalize(bNodeTree *ntree)
/* ensures only a single output node is enabled */
ntreeSetOutput(ntree);
- for (node = ntree->nodes.first; node; node = node->next) {
- /* store new_node pointer to original */
- node->new_node->original = node;
+ bNode *node_src = ntree->nodes.first;
+ bNode *node_local = ltree->nodes.first;
+ while (node_src != NULL) {
+ node_local->original = node_src;
+ node_src = node_src->next;
+ node_local = node_local->next;
}
if (ntree->typeinfo->localize) {
@@ -3595,7 +3649,7 @@ void node_type_storage(bNodeType *ntype,
void (*freefunc)(struct bNode *node),
void (*copyfunc)(struct bNodeTree *dest_ntree,
struct bNode *dest_node
@@ Diff output truncated at 10240 characters. @@
More information about the Bf-blender-cvs
mailing list