Question about (broken) copying nodes and `Node.copy` method

Archive Blender Development
1

The method Node.copy is supposed to be called when a node is copied. In particular, this should allow developers of ShaderNodeCustomGroup to recreate internal node_tree if it depends on node options/properties.
However, this does not happen when node is copied into another material with operator ops.material.new. This results in that all node trees are shared between materials and adjusting trees according to options/properties affect all materials.

The condition when the method shold be called is determined by this line at source/blender/rendkernel/intern/node.c, in function BKE_node_copy_ex:

bool do_copy_api = !((flag & LIB_ID_CREATE_NO_MAIN) || (flag & LIB_ID_COPY_LOCALIZE));

The flags are described as /** Create datablock outside of any main database - similar to 'localize' functions of materials etc. */ and /** Generate a local copy, outside of bmain, to work on (used by COW e.g.). */

When copying material node_tree, the flags to copy nodes are set
at source/blender/rendkernel/intern/node.c, in function BKE_node_tree_copy_data
:

/* We never handle usercount here for own data. */
  const int flag_subdata = flag | LIB_ID_CREATE_NO_USER_REFCOUNT;

The flag LIB_ID_CREATE_NO_USER_REFCOUNT is /** Do not affect user refcount of datablocks used by new one (which also gets zero usercount then). */ and it’s included into LIB_ID_COPY_LOCALIZE.
Altough, refcounts of node_trees of copied ShaderNodeCustomGroups are effectively affected by such copying.

Could someone or @mont29 please explain the logic behind this, and hint if the behaviour of not calling copy can be ever fixed?

2

From quick overview the do_copy_api check is obviously wrong, you cannot use a simple boolean value to check a multiple-bits ‘flag’ like LIB_ID_COPY_LOCALIZE, you need a proper full comparison of the result of the AND operation here to ensure all expected bits are actually set…

Will fix that now, am kind of wary though that some ID may create other random IDs in their duplication code… we do that in a few specific cases (like actions from animdata, or shapekeys for meshes and co), but those are fairly well contained scope, and were already rather hairy to get working properly… :confused:

3

Isn’t it the line in BKE_node_tree_copy_data that unconditionally sets LIB_ID_CREATE_NO_USER_REFCOUNT that affects do_call_api?

In particular, in this call trace:

BKE_node_copy_ex node.c:1094 // flag == 0x5000002, do_copy_api == false
BKE_node_tree_copy_data node.c:1518 // flag == 0x5000000
BKE_id_copy_ex library.c:724
BKE_material_copy_data material.c:193 // BKE_id_copy_ex(bmain, (ID *)ma_src->nodetree, (ID **)&ma_dst->nodetree, flag & ~LIB_ID_CREATE_NO_ALLOCATE);
BKE_id_copy_ex library.c:682
new_material_exec render_shading.c:638 // BKE_id_copy_ex(bmain, &ma->id, (ID **)&new_ma, LIB_ID_COPY_DEFAULT | LIB_ID_COPY_ACTIONS);
5

@Secrop, @brecht, could you please comment that line about do_copy_api from your patch https://developer.blender.org/rB5797a5fc65c87b69460d910a82d219b5e3ea12ad ?

6

Thanks for investigating, I think it should be all fixed by:
https://developer.blender.org/rB416e66c7fe211e7e21d0d4fad987d4219c191f71

3 Likes
7

Not quite.
New bug discovered and new investigation posted:
https://developer.blender.org/T72317

9

Well.
Setting property node_tree updates user counts for old and new values.

The reason why it fails is that it is called from various BKE_*_copy_data while a node is inconsistent state in the middle of copying/linking/parenting.

I suppose that the call should be moved from the BKE_node_copy_ex to upper contexts such as operator implementations.