archive/blender-archive
Archived
1
1
Fork 0
Code Pull Requests Packages Activity

Fix T64247: Crash on playback with special shader node tree

Browse Source 
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
This commit is contained in:
Sergey Sharybin
2019-06-03 17:08:25 +02:00
parent 351e68e0a4
commit b998a7b384
15 changed files with 154 additions and 61 deletions
Download Patch File Download Diff File Expand all files Collapse all files

23
source/blender/blenkernel/BKE_node.h
View File

@@ -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));

88
source/blender/blenkernel/intern/node.c
View File

@@ -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,
struct bNode *src_node))
const struct bNode *src_node))
{
if (storagename) {
BLI_strncpy(ntype->storagename, storagename, sizeof(ntype->storagename));

7
source/blender/editors/space_node/node_edit.c
View File

@@ -1182,7 +1182,7 @@ static int node_duplicate_exec(bContext *C, wmOperator *op)
lastnode = ntree->nodes.last;
for (node = ntree->nodes.first; node; node = node->next) {
if (node->flag & SELECT) {
newnode = BKE_node_copy_ex(ntree, node, LIB_ID_COPY_DEFAULT);
newnode = BKE_node_copy_store_new_pointers(ntree, node, LIB_ID_COPY_DEFAULT);
/* to ensure redraws or rerenders happen */
ED_node_tag_update_id(snode->id);
@@ -2037,7 +2037,8 @@ static int node_clipboard_copy_exec(bContext *C, wmOperator *UNUSED(op))
if (node->flag & SELECT) {
/* No ID refcounting, this node is virtual,
* detached from any actual Blender data currently. */
bNode *new_node = BKE_node_copy_ex(NULL, node, LIB_ID_CREATE_NO_USER_REFCOUNT);
bNode *new_node = BKE_node_copy_store_new_pointers(
NULL, node, LIB_ID_CREATE_NO_USER_REFCOUNT);
BKE_node_clipboard_add_node(new_node);
}
}
@@ -2163,7 +2164,7 @@ static int node_clipboard_paste_exec(bContext *C, wmOperator *op)
/* copy nodes from clipboard */
for (node = clipboard_nodes_lb->first; node; node = node->next) {
bNode *new_node = BKE_node_copy_ex(ntree, node, LIB_ID_COPY_DEFAULT);
bNode *new_node = BKE_node_copy_store_new_pointers(ntree, node, LIB_ID_COPY_DEFAULT);
/* pasted nodes are selected */
nodeSetSelected(new_node, true);

2
source/blender/editors/space_node/node_group.c
View File

@@ -439,7 +439,7 @@ static int node_group_separate_selected(
if (make_copy) {
/* make a copy */
newnode = BKE_node_copy_ex(ngroup, node, LIB_ID_COPY_DEFAULT);
newnode = BKE_node_copy_store_new_pointers(ngroup, node, LIB_ID_COPY_DEFAULT);
}
else {
/* use the existing node */

2
source/blender/makesrna/intern/rna_nodetree.c
View File

@@ -1336,7 +1336,7 @@ static void rna_Node_init(const bContext *C, PointerRNA *ptr)
RNA_parameter_list_free(&list);
}
static void rna_Node_copy(PointerRNA *ptr, struct bNode *copynode)
static void rna_Node_copy(PointerRNA *ptr, const struct bNode *copynode)
{
extern FunctionRNA rna_Node_copy_func;

34
source/blender/nodes/composite/node_composite_tree.c
View File

@@ -95,15 +95,17 @@ static void free_cache(bNodeTree *ntree)
}
/* local tree then owns all compbufs */
static void localize(bNodeTree *UNUSED(localtree), bNodeTree *ntree)
static void localize(bNodeTree *localtree, bNodeTree *ntree)
{
bNode *node;
bNodeSocket *sock;
for (node = ntree->nodes.first; node; node = node->next) {
bNode *node = ntree->nodes.first;
bNode *local_node = localtree->nodes.first;
while (node != NULL) {
local_node->original = node;
/* ensure new user input gets handled ok */
node->need_exec = 0;
node->new_node->original = node;
local_node->original = node;
/* move over the compbufs */
/* right after ntreeCopyTree() oldsock pointers are valid */
@@ -111,19 +113,29 @@ static void localize(bNodeTree *UNUSED(localtree), bNodeTree *ntree)
if (ELEM(node->type, CMP_NODE_VIEWER, CMP_NODE_SPLITVIEWER)) {
if (node->id) {
if (node->flag & NODE_DO_OUTPUT) {
node->new_node->id = (ID *)node->id;
local_node->id = (ID *)node->id;
}
else {
node->new_node->id = NULL;
local_node->id = NULL;
}
}
}
for (sock = node->outputs.first; sock; sock = sock->next) {
sock->new_sock->cache = sock->cache;
sock->cache = NULL;
sock->new_sock->new_sock = sock;
bNodeSocket *output_sock = node->outputs.first;
bNodeSocket *local_output_sock = local_node->outputs.first;
while (output_sock != NULL) {
local_output_sock->cache = output_sock->cache;
output_sock->cache = NULL;
/* This is actually link to original: someone was just lazy enough and tried to save few
* bytes in the cost of readability. */
local_output_sock->new_sock = output_sock;
output_sock = output_sock->next;
local_output_sock = local_output_sock->next;
}
node = node->next;
local_node = local_node->next;
}
}

4
source/blender/nodes/composite/nodes/node_composite_cryptomatte.c
View File

@@ -273,7 +273,9 @@ static void node_free_cryptomatte(bNode *node)
}
}
static void node_copy_cryptomatte(bNodeTree *UNUSED(dest_ntree), bNode *dest_node, bNode *src_node)
static void node_copy_cryptomatte(bNodeTree *UNUSED(dest_ntree),
bNode *dest_node,
const bNode *src_node)
{
NodeCryptomatte *src_nc = src_node->storage;
NodeCryptomatte *dest_nc = MEM_dupallocN(src_nc);

31
source/blender/nodes/composite/nodes/node_composite_image.c
View File

@@ -432,15 +432,17 @@ static void node_composit_free_image(bNode *node)
static void node_composit_copy_image(bNodeTree *UNUSED(dest_ntree),
bNode *dest_node,
bNode *src_node)
const bNode *src_node)
{
bNodeSocket *sock;
dest_node->storage = MEM_dupallocN(src_node->storage);
/* copy extra socket info */
for (sock = src_node->outputs.first; sock; sock = sock->next) {
sock->new_sock->storage = MEM_dupallocN(sock->storage);
const bNodeSocket *src_output_sock = src_node->outputs.first;
bNodeSocket *dest_output_sock = dest_node->outputs.first;
while (dest_output_sock != NULL) {
dest_output_sock->storage = MEM_dupallocN(src_output_sock->storage);
src_output_sock = src_output_sock->next;
dest_output_sock = dest_output_sock->next;
}
}
@@ -556,16 +558,17 @@ static void node_composit_free_rlayers(bNode *node)
}
static void node_composit_copy_rlayers(bNodeTree *UNUSED(dest_ntree),
bNode *UNUSED(dest_node),
bNode *src_node)
bNode *dest_node,
const bNode *src_node)
{
bNodeSocket *sock;
/* copy extra socket info */
for (sock = src_node->outputs.first; sock; sock = sock->next) {
if (sock->storage) {
sock->new_sock->storage = MEM_dupallocN(sock->storage);
}
const bNodeSocket *src_output_sock = src_node->outputs.first;
bNodeSocket *dest_output_sock = dest_node->outputs.first;
while (dest_output_sock != NULL) {
dest_output_sock->storage = MEM_dupallocN(src_output_sock->storage);
src_output_sock = src_output_sock->next;
dest_output_sock = dest_output_sock->next;
}
}

2
source/blender/nodes/composite/nodes/node_composite_moviedistortion.c
View File

@@ -64,7 +64,7 @@ static void storage_free(bNode *node)
node->storage = NULL;
}
static void storage_copy(bNodeTree *UNUSED(dest_ntree), bNode *dest_node, bNode *src_node)
static void storage_copy(bNodeTree *UNUSED(dest_ntree), bNode *dest_node, const bNode *src_node)
{
if (src_node->storage) {
dest_node->storage = BKE_tracking_distortion_copy(src_node->storage);

4
source/blender/nodes/composite/nodes/node_composite_outputFile.c
View File

@@ -228,7 +228,9 @@ static void free_output_file(bNode *node)
MEM_freeN(node->storage);
}
static void copy_output_file(bNodeTree *UNUSED(dest_ntree), bNode *dest_node, bNode *src_node)
static void copy_output_file(bNodeTree *UNUSED(dest_ntree),
bNode *dest_node,
const bNode *src_node)
{
bNodeSocket *src_sock, *dest_sock;

6
source/blender/nodes/intern/node_util.c
View File

@@ -57,12 +57,14 @@ void node_free_standard_storage(bNode *node)
}
}
void node_copy_curves(bNodeTree *UNUSED(dest_ntree), bNode *dest_node, bNode *src_node)
void node_copy_curves(bNodeTree *UNUSED(dest_ntree), bNode *dest_node, const bNode *src_node)
{
dest_node->storage = curvemapping_copy(src_node->storage);
}
void node_copy_standard_storage(bNodeTree *UNUSED(dest_ntree), bNode *dest_node, bNode *src_node)
void node_copy_standard_storage(bNodeTree *UNUSED(dest_ntree),
bNode *dest_node,
const bNode *src_node)
{
dest_node->storage = MEM_dupallocN(src_node->storage);
}

4
source/blender/nodes/intern/node_util.h
View File

@@ -58,10 +58,10 @@ extern void node_free_standard_storage(struct bNode *node);
extern void node_copy_curves(struct bNodeTree *dest_ntree,
struct bNode *dest_node,
struct bNode *src_node);
const struct bNode *src_node);
extern void node_copy_standard_storage(struct bNodeTree *dest_ntree,
struct bNode *dest_node,
struct bNode *src_node);
const struct bNode *src_node);
extern void *node_initexec_curves(struct bNodeExecContext *context,
struct bNode *node,
bNodeInstanceKey key);

4
source/blender/nodes/shader/nodes/node_shader_script.c
View File

@@ -44,7 +44,9 @@ static void node_free_script(bNode *node)
}
}
static void node_copy_script(bNodeTree *UNUSED(dest_ntree), bNode *dest_node, bNode *src_node)
static void node_copy_script(bNodeTree *UNUSED(dest_ntree),
bNode *dest_node,
const bNode *src_node)
{
NodeShaderScript *src_nss = src_node->storage;
NodeShaderScript *dest_nss = MEM_dupallocN(src_nss);

2
source/blender/nodes/shader/nodes/node_shader_tex_pointdensity.c
View File

@@ -57,7 +57,7 @@ static void node_shader_free_tex_pointdensity(bNode *node)
static void node_shader_copy_tex_pointdensity(bNodeTree *UNUSED(dest_ntree),
bNode *dest_node,
bNode *src_node)
const bNode *src_node)
{
dest_node->storage = MEM_dupallocN(src_node->storage);
NodeShaderTexPointDensity *point_density = dest_node->storage;

2
source/blender/nodes/texture/nodes/node_texture_output.c
View File

@@ -154,7 +154,7 @@ static void init(bNodeTree *UNUSED(ntree), bNode *node)
assign_index(node);
}
static void copy(bNodeTree *dest_ntree, bNode *dest_node, bNode *src_node)
static void copy(bNodeTree *dest_ntree, bNode *dest_node, const bNode *src_node)
{
node_copy_standard_storage(dest_ntree, dest_node, src_node);
unique_name(dest_node);