Initial Implementation of local ID re-use when appending.
ClosedPublic
Actions

Authored by Bastien Montagne (mont29) on Sep 17 2021, 4:48 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Campbell Barton (campbellbarton)

Maniphest Tasks

T90545: ID Deduplication on Append - Technical Implementation

Commits

rB794c2828af60: Initial implementation of local ID re-use when appending.

Summary

This commit adds to ID struct a new optional 'weak reference' to a
linked ID (in the form of a blend file library path and full ID name).

This can then be used on next append to try to find a matching local ID
instead of re-making the linked data local again.

Ref. T90545

NOTE: in final commit in master ID re-use will be disabled for regular append for the time being (3.0 release), and only used for assets.

Diff Detail

Repository: rB Blender

Event Timeline

Bastien Montagne (mont29) requested review of this revision.Sep 17 2021, 4:48 PM

Bastien Montagne (mont29) created this revision.

Harbormaster completed remote builds in B17106: Diff 41996.Sep 17 2021, 4:48 PM

Bastien Montagne (mont29) added inline comments.Sep 20 2021, 9:58 AM

source/blender/windowmanager/intern/wm_files_link.c
645	This will be false in initial commit, and API to use it for asset browser will be added afterward.

Bastien Montagne (mont29) added a task: T90545: ID Deduplication on Append - Technical Implementation.Sep 20 2021, 9:58 AM

@Brecht Van Lommel (brecht) @Campbell Barton (campbellbarton) Would be ideal to get this in master this week, I know this is really last minute, but would love to have at least agreement on the general API and changes to ID struct (code itself is fairly straight forward I think).

Generally seems fine, some notes:

There doesn't seem to be a way to clear the weak-references (perhaps we would want this? - not urgent though).
Users may expect this to work between systems (different operating-systems for example), this may be possible in a limited way - using relative locations for example. OTOH, it's not high priority.

source/blender/blenkernel/intern/main.c
368–392	Since this is `main.c` prefer common prefix on utility functions.
405–418	It'd be more efficient to loop over list-base types, skipping all lists that fail the `BKE_idtype_idcode_append_is_reusable` test.
406–411	picky the NULL check could be before the function call.
source/blender/makesrna/intern/rna_ID.c
1945	Any reason not to make this editable? It's the kind of thing Python scripts could manipulate to account for relocating projects - for e.g.

This revision is now accepted and ready to land.Sep 21 2021, 5:30 PM

I think this filepath needs to be iterated over in bpath.c, for cases like path remapping?

What happens when you append a .blend file from a.blend to b.blend, and then append it from b.blend to c.blend. Does it reference a.blend or b.blend? I guess it should be a.blend, to be able to detect duplicates from both a.blend and c.blend.

This revision now requires changes to proceed.Sep 21 2021, 6:06 PM

In D12545#328498, @Brecht Van Lommel (brecht) wrote:

I think this filepath needs to be iterated over in bpath.c, for cases like path remapping?

Not sure? We could, but many things can break those weak ref anyway, this is part of the design?

What happens when you append a .blend file from a.blend to b.blend, and then append it from b.blend to c.blend. Does it reference a.blend or b.blend? I guess it should be a.blend, to be able to detect duplicates from both a.blend and c.blend.

Item from c.blend refers to item from b.blend, by definition. Weak ref is not recursive, it is fully cleared when linking data. This is a helper to work on some specific cases of append, not a second implementation of linking with chains of dependencies etc. If people append the 'same' ID from both a.blend and b.blend it's their choice, from PoV of this system those are two fully different IDs.

source/blender/makesrna/intern/rna_ID.c
1945	Main reason is that we'd need a way to ensure uniqueness of edited references then... possible but not high priority currently.

In D12545#328501, @Bastien Montagne (mont29) wrote:

Not sure? We could, but many things can break those weak ref anyway, this is part of the design?

But this case is easy to fix by adding 3 lines of code. Why not do it?

Item from c.blend refers to item from b.blend, by definition. Weak ref is not recursive, it is fully cleared when linking data. This is a helper to work on some specific cases of append, not a second implementation of linking with chains of dependencies etc. If people append the 'same' ID from both a.blend and b.blend it's their choice, from PoV of this system those are two fully different IDs.

Ok, seems like a reasonable limitation.

Updates from reviews.

In D12545#328588, @Brecht Van Lommel (brecht) wrote:

In D12545#328501, @Bastien Montagne (mont29) wrote:

Not sure? We could, but many things can break those weak ref anyway, this is part of the design?

But this case is easy to fix by adding 3 lines of code. Why not do it?

Fair enough, done.

Harbormaster completed remote builds in B17221: Diff 42224.Sep 22 2021, 11:15 AM

Campbell Barton (campbellbarton) added inline comments.Sep 22 2021, 2:59 PM

source/blender/blenkernel/BKE_idtype.h
52–53	A reference to `#LibraryWeakReference` would be good, it's handy to be able to quickly jump to declaration without having to dig around in the code.
source/blender/blenkernel/intern/main.c
445	picky use colon after argument name.
source/blender/makesdna/DNA_ID.h
443	picky use `NOTE:`