Page MenuHome

Fix T68971: Copy As New Driver from Material node creates a bad reference.
ClosedPublic

Authored by Alexander Gavrilov (angavrilov) on Aug 22 2019, 7:44 PM.

Details

Summary

NodeTree structures of materials and some other data blocks are
effectively node group datablock objects that are contained inside
the parent block. Thus, direct references to them are only valid
while blender is running, and are lost on save.

Fix Copy As New Driver to create a reference that goes through
the owner datablock, by adding a new ID flag to mark private
pseudo-datablocks.

Diff Detail

Repository
rB Blender

Event Timeline

Bastien Montagne (mont29) requested changes to this revision.Aug 23 2019, 11:19 AM

Main point to change here for me is the fact that you generate the proper RNA path only for drivers. this should be done in regular RNA path generation code instead.

Otherwise, that looks like a proper solution to me, and a much less potentially annoying one that the backpointer thingy. ;)

source/blender/blenkernel/intern/library.c
1415–1418

Probably as a separate commit, but could be interesting to use that new LIB_PRIVATE_DATA flag instead of the BLI_findindex() call here? And rename it to use_private_data_alloc e.g.

1454–1455

On one hand am a bit wary of putting that in the generic ID management code, on the other hand it makes total sense... Maybe hide it behind the use_nodetree_alloc_exception condition, to be 100% clear that this is a specific case?

source/blender/blenkernel/intern/node.c
2204–2218

I would not hard-code which kind of IDs can have ntrees in too many places, this is always an issue when you change the code...

Think a function to check if an ID code can have nodes would be nice (ntreeIDTypeCanHaveNodes() ? defined next to ntreeFromID()), then you can loop over the whole Main DB (FOREACH_MAIN_LISTBASE_BEGIN), check whether first ID of each type can actually have nodes, and go forward if they do (FOREACH_MAIN_LISTBASE_ID_BEGIN), with virtually no extra processing time, and with fully future-proof code.

Same in versioning file of course.

source/blender/editors/include/ED_keyframing.h
406–414 ↗(On Diff #17419)

I don't think that this belong to anim code. This is RNA business to provide a proper RNA path, and it's important in other contexts than animation.

RNA_path_from_ID_to_struct() should rather be changed to detect that new flag when it's dealing with IDs, and handle it properly (probably by defining a path callback for the ntree RNA struct, to handle those cases...). And add a way to return the PointerRNA of the actual ID owner too.

This revision now requires changes to proceed.Aug 23 2019, 11:19 AM
Brecht Van Lommel (brecht) requested changes to this revision.EditedAug 23 2019, 2:17 PM

I don't think this is a good solution. We have to stop doing these ID operations with bad time complexity. Creating an RNA path should not require looping over the main database. This will also not work for datablocks outside the main database. And thinking further into the future, for things like lazy loading of datablocks or hierarchical organization of scenes.

ID management as a whole should get a more object-oriented design, where the implementation of a datablock does not spread out all over and leads to spaghetti code.

I think the original commits got it right. Handling of the backpointer can be encapsulated in the code for datablocks containing the nodetree, and the only time it needs to be updated is when the datablock is initialized or copied.

Moved path code to rna_access.c and used it in existing 'full path' functions.

While I don't think the current patch is ideal, @Bastien Montagne (mont29) and I agreed to go with the approach used here.

In the future we can improve the design here to avoid the need for such exceptions: T69169: Improve embedded nodetree/master collection storage & handling.

I'll leave further review of this patch to @Bastien Montagne (mont29).

source/blender/blenkernel/intern/library.c
1454–1455

I think it's not "specific" - it's just the first case when a flag has to be copied too.

source/blender/blenkernel/intern/node.c
2204–2218

I think it's not a problem here because it's right next to ntreeFromID. This is no different from another function with a switch.

I changed the doversion code to simply scan all IDs since it's only supposed to run once.

think that's OK, we can always tweak it further later in code. Also, scene->master collection need the same hack, but will have to check first how it handles copying of that one etc.

source/blender/makesrna/intern/rna_access.c
5762–5765

Please use same syntax as everywhere in blender : \param id blah...

And \brief is useless and redundant, just type in description, doxy will take first block as brief one.

This revision is now accepted and ready to land.Wed, Aug 28, 5:33 PM