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

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

Details

Reviewers

Bastien Montagne (mont29)

Commits

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

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

Alexander Gavrilov (angavrilov) created this revision.Aug 22 2019, 7:44 PM

Harbormaster completed remote builds in B4553: Diff 17419.Aug 22 2019, 7:44 PM

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

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.

Jacques Lucke (JacquesLucke) added a subscriber: Jacques Lucke (JacquesLucke).Aug 23 2019, 2:41 PM

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

Harbormaster completed remote builds in B4563: Diff 17437.Aug 23 2019, 2:47 PM

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 handling of embedded data-blocks (root nodetrees and master collections).

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

Bastien Montagne (mont29) mentioned this in rB16c1b10ed240: Fix T69146: Segment Fault using Undo for file with several scenes and script..Aug 26 2019, 2:56 PM

Alexander Gavrilov (angavrilov) added inline comments.Aug 28 2019, 10:50 AM

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.