Page MenuHome

Outliner: Collection - Duplicate Hierarchy, and Duplicate Linked Hierarchy
ClosedPublic

Authored by Dalai Felinto (dfelinto) on Feb 22 2019, 9:47 PM.

Details

Summary

As per the suggestion on T57064, this introduces two new options to duplicate collections.
We then have:

  • Duplicate > Collection (New collection with linked content).
  • Duplicate > Hierachy (Duplicate entire hierarchy and make all contents single user).
  • Duplicate > Linked Hierarchy (Duplicate entire hierarchy keeping content linked with original).

Diff Detail

Repository
rB Blender

Event Timeline

Patch LGTM mostly, besides the LIB_TAG_DOIT flag not being reset before usage, comments below are mostly regarding naming and code organization. ;)

source/blender/blenkernel/BKE_collection.h
56–57

Not related to this patch, but those tree should really be put below 'generic ID API' that are _copy_data() and _make_local(), with an empty line in between...

56–57

In fact, think I would rather see those renamed to BKE_collection_duplicate()/_duplicate_ex().

Am becoming a bit tired of seeing copy used for all kind of different things, would like to keep it for 'basic' regular ID copying, and any more involved code doing some deep-copy and such would then use duplicate instead.

57

Naming conventions for bools, something like do_hierarchy and keep_linked e.g. ?

57–58

Not implemented, good oportunity to nuke it and replace its only call (in full scene copying) by one to BKE_collection_copy_ex I guess?

source/blender/blenkernel/BKE_key.h
63–64 ↗(On Diff #13808)

That looks more like unrelated cleanup? ;)

source/blender/blenkernel/BKE_object.h
124

Same remark as for collections regarding naming, in fact this is even more critical here since your new copy_ex func is not even used by the copy one. So would rather like to see it named BKE_object_duplicate() imho.

source/blender/blenkernel/intern/collection.c
212

name as well, rather duplicate than copy… And maybe also replace hierarchy with recursive, so that this behavior becomes obvious?

217

LIB_TAG_DOIT is a RESET_BEFORE_USEflag currently (turning it into a RESET_AFTER_USE is still a boring TODO for some rainy day). So please clean it up *before* any usage, and also after in new code.

245–247

Not sure why you cannot move (most of) BKE_collection_copy_ex() logic here, just keeping security checks and final collection sync there, and e.g. pass NULL GHash when you do not want to do recursion in hierarchy? Then you just need to add the parent parameter to this function, and you are done?

Would find it cleaner to have all logic in a same place…

281–303

Think that whole block could go into collection_copy_hierarchy()

source/blender/blenkernel/BKE_collection.h
57

In fact for the linked guy, I’d rather see it logically reversed, and named something like do_deep_copy or something, because 'linked' behavior should remain default one in general, doing deep-copy being some 'less usual' behavior.

Dalai Felinto (dfelinto) marked 2 inline comments as done.
  • From review: use more boolean-like names
  • From review: Refactor - shuffle code around
  • From review: Rename BKE_collection_copy functions
  • From review: collection_copy_hierarchy > collection_duplication_recursive
  • From review: BKE_object_copy > BKE_object_duplicate
  • Small refactor: BKE_collection_duplicate > +_shallow, _ex > -_ex
  • From review: flip logic for linked/do_deep_copy

Note (not addressed here): single_object_users can be refactored to use the new logic.

Updated patch with the considerations from review. I'm now just waiting for a final design open-question on T57064 and I can finish + commit.
Meanwhile @Bastien Montagne (mont29) feel free to see if it is all to your liking now. Note that I flipped BKE_collection_duplicate and BKE_collection_duplicate_shallow around.

source/blender/blenkernel/BKE_collection.h
57–58

I would rather we do it afterwards. Scene copying is working now, so just remove this and call BKE_collection_duplicate will likely introduce issues.

source/blender/blenkernel/BKE_key.h
63–64 ↗(On Diff #13808)

It is related to making ob const in BKE_copy_ex, since this function was called from a non-const ob function before (object_add_duplicate_internal).
That said I will separate this "cleanup" on its own commit.

source/blender/blenkernel/intern/collection.c
217

Calling BKE_main_id_tag_idcode(bmain, ID_GR, LIB_TAG_DOIT, false); before this function. Maybe you missed that?

Besides the wrong rename of collection_copy to collection_duplicate_shadow, think this is ready for master.

source/blender/blenkernel/BKE_collection.h
63

That one should not have been renamed, we have BKE_xxx_copy for nearly all our ID types I think, let’s keep it consistent: copy implies "shallow", duplicate implies "[some] deep copy".

source/blender/blenkernel/intern/collection.c
281–303

Not done? :P Can also do it myself later...

This revision is now accepted and ready to land.Wed, Feb 27, 10:18 AM
This revision was automatically updated to reflect the committed changes.

And committed followed by 7d4e1ac727f3cd697d137935a9dbd30d5cfe5192 that get rid of the original useless collection duplicate operator.

source/blender/blenkernel/intern/collection.c
281–303

Not sure how much more you wanted this integrated in the collection_duplicate_recursive, so I leave to you to do this final cleanup? :)

Ended up with a rather extensive refactor in rB52f318b9142ba (found some optimizations/simplifications on the way, and a broken handling of children collections being linked more than once in the hierarchy). Checked new code with some rather convoluted test cases, but would not mind if you could double-check it on your end as well?

a broken handling of children collections being linked more than once in the hierarchy).

How can I reproduce this?

Open that file and duplicate the top Collection 1 collection. You will notice that in duplicated Collection 1.001, you will have two Collection 1 1 2.001 and Collection 1 1 2.002, instead of expected single one linked twice. Now this is working as expected (at least I hope ;) ).