Details

Reviewers

Sybren A. Stüvel (sybren)

Group Reviewers

Pipeline, Assets & I/O

Summary

This patch supports instantiating object data on append/link, reported as a bug T58304.

This is an option, available when linking/appending, similar to the existing "Instance Collections" option.

Notes:

Currently to add object-data to the scene you need to append it, then add an object of the same data type and manually swap the ID in the ID selector.

In many cases users *really* mean to append objects, not object data, so we could leave the current behavior as-is and resolve in the linking UI (support filtering by object type, or rely on the asset manager to better handle this usability issue).

TODO:

This patch sets and clears LIB_TAG_DOIT for all ID types, this should be changed only to clear/set id types that are needed, it's not complicated but would add changes in many other places, making this patch much noisier, this needs to be done before the patch can be considered finished but shouldn't be needed for the purpose of reviewing the basic functionality.

add_collections_to_scene / add_loose_objects_to_scene / add_loose_object_data_to_scene could de-duplicate add-object logic, I'd prefer this be left separate from this patch.

Diff Detail

Repository

rB Blender

Branch

TEMP-T58304-FIX (branched from master)

Build Status

Buildable 9956
Build 9956: arc lint + arc unit

Event Timeline

Campbell Barton (campbellbarton) requested review of this revision.Sep 3 2020, 8:46 AM

Campbell Barton (campbellbarton) created this revision.

Harbormaster completed remote builds in B9956: Diff 28485.Sep 3 2020, 8:46 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Sep 3 2020, 8:56 AM

Campbell Barton (campbellbarton) added a reviewer: Pipeline, Assets & I/O.

Campbell Barton (campbellbarton) added a project: Pipeline, Assets & I/O.

Campbell Barton (campbellbarton) mentioned this in T58304: Metaballs appended as orphan.Sep 3 2020, 8:59 AM

Campbell Barton (campbellbarton) retitled this revision from Library: support instantiating object data on append/link to Library Linking: support instantiating object data on append/link.Sep 3 2020, 9:47 AM

I'm missing a few "bigger picture" and user interface things in the patch description. How would users interact with this feature? What happens when they select a datablock that cannot be instanced (like a material)? Is this still intended to work this way when the asset engine lands?

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Sep 3 2020, 12:22 PM

In D8792#214929, @Sybren A. Stüvel (sybren) wrote:

I'm missing a few "bigger picture" and user interface things in the patch description. How would users interact with this feature? What happens when they select a datablock that cannot be instanced (like a material)? Is this still intended to work this way when the asset engine lands?

Updated the description.

This is an option matching link/append "Instance Collections" fairly closely, both have the down side that they don't make much sense when when appending some data-types such as materials. Although hiding them isn't quite right either since a material could always link in an object/collection.

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Sep 3 2020, 12:25 PM

This patch sets and clears LIB_TAG_DOIT for all ID types, this should be changed only to clear/set id types that are needed, it's not complicated but would add changes in many other places.

Is this something you'll do in an update to this patch?

source/blender/blenloader/intern/readfile.c
10335	Why would a brand spanking new function need an unused parameter? It's not like it's used as a callback that requires a specific signature.
10349	Single-letter variables should be avoided.
10362–10382	This can be extracted into its own function, f.e. `add_object_data_to_scene()`.
10382	Apparently the objects are all instanced at the cursor position. This isn't mentioned anywhere in the patch description, the property tooltip, etc.
source/blender/windowmanager/intern/wm_files_link.c
570	This means that the feature is enabled by default when linking. I'd write this in the patch description as well, as it's an important aspect for users.
572	This sentence isn't finished.

This revision now requires changes to proceed.Sep 3 2020, 4:14 PM

Enable 'insance_object_data' by default as is_link doesn't make sense as it does for collections.
Remove redundant lib argument.
Rename iterator.

Harbormaster completed remote builds in B10014: Diff 28588.Sep 7 2020, 7:05 AM

Missed minor change last update.

Harbormaster completed remote builds in B10015: Diff 28589.Sep 7 2020, 7:06 AM

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Sep 7 2020, 7:09 AM

Campbell Barton (campbellbarton) marked 4 inline comments as done.Sep 7 2020, 7:16 AM

Campbell Barton (campbellbarton) added inline comments.

source/blender/blenloader/intern/readfile.c
10362–10382	add_collections_to_scene / add_loose_objects_to_scene / add_loose_object_data_to_scene could de-duplicate add-object logic, I'd prefer this be left separate from this patch.
10382	This matches collection instancing, I don't have a strong opinion on this but prefer this is added to both collection/obdata instancing in that case.
source/blender/windowmanager/intern/wm_files_link.c
570	The rationale for using linking to change defaults for collections doesn't apply to object data. It's enabled by default now, since I think it's unlikely you don't want object data that's linked to be accessible in the current scene.

Marking as planned since limited ID types that set LIB_TAG_DOIT would need to be done before committing.

Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)Sep 7 2020, 9:50 AM

Sybren A. Stüvel (sybren) requested changes to this revision.Sep 7 2020, 10:37 AM

Sybren A. Stüvel (sybren) added inline comments.

source/blender/blenloader/intern/readfile.c
10349	Right now `lbarray` and `lba_idx` convey no meaning as to their contents. Yes, a variable `ListBase somename[12]` is an array of `ListBase` pointers. But that's already clear from the type and doesn't need repeating in the name. This is especially important with `ListBase`, as that effectively is just a `void ` without any type info. What is in here? What is being iterated over? I would find a name like `bmain_contents` a lot clearer.
10355	`id_first` cannot be `NULL` here.
10362–10382	My suggestion to separate into a different function wasn't only meant to aid reuse, but also to adhere to the "Functions Should Do One Thing" heuristic described in Clean Code (R.C. Martin, 2009). Granted, Martin often takes it quite far, but still I feel a separation of "Iterate over all of bmain and find loose object data" and "create an object for this data" would help here.
10382	Sounds good to me.
source/blender/windowmanager/intern/wm_files_link.c
570	My point was that this is currently in the patch description: This is an option, available when linking/appending, similar to the existing "Instance Collections" option. and that I would add something like "This option is enabled by default when linking, just like the Instance Collections option."

Campbell Barton (campbellbarton) added inline comments.Sep 7 2020, 11:00 AM

source/blender/blenloader/intern/readfile.c

10349

If we want new conventions here, we could deal with this in the current code-base first, currently this is called a or i, almost everywhere, except where it's called lba_idx in wm_files_link.c.

cr "[a-z_]+ = set_listbasepointers"
./source/blender/blenkernel/BKE_main.h:195:8:     int _i = set_listbasepointers((_bmain), _lbarray); \
./source/blender/blenkernel/intern/blendfile.c:864:2:   a = set_listbasepointers(bmain_dst, lbarray_dst);
./source/blender/blenkernel/intern/blendfile.c:907:2:   a = set_listbasepointers(bmain_dst, lbarray_src);
./source/blender/blenkernel/intern/bpath.c:787:6:   int a = set_listbasepointers(bmain, lbarray);
./source/blender/blenkernel/intern/lib_id.c:902:2:   a = set_listbasepointers(mainvar, lbarray);
./source/blender/blenkernel/intern/lib_id.c:934:2:   a = set_listbasepointers(bmain, lbarray);
./source/blender/blenkernel/intern/lib_id.c:1856:11:   for (int a = set_listbasepointers(bmain, lbarray); a--;) {
./source/blender/blenkernel/intern/lib_id_delete.c:248:2:   base_count = set_listbasepointers(bmain, lbarray);
./source/blender/blenkernel/intern/lib_query.c:515:6:   int i = set_listbasepointers(bmain, lb_array);
./source/blender/blenkernel/intern/lib_query.c:568:6:   int i = set_listbasepointers(bmain, lb_array);
./source/blender/blenkernel/intern/lib_query.c:675:8:     int i = set_listbasepointers(bmain, lb_array);
./source/blender/blenkernel/intern/main.c:66:2:   a = set_listbasepointers(mainvar, lbarray);
./source/blender/blenloader/intern/blend_validate.c:64:6:   int i = set_listbasepointers(bmain, lbarray);
./source/blender/blenloader/intern/blend_validate.c:97:4:     i = set_listbasepointers(curmain, lbarray);
./source/blender/blenloader/intern/readfile.c:523:2:   a = set_listbasepointers(from, fromarray);
./source/blender/blenloader/intern/readfile.c:588:2:   i = set_listbasepointers(main, lbarray);
./source/blender/blenloader/intern/readfile.c:1955:8:     int i = set_listbasepointers(ptr, lbarray);
./source/blender/blenloader/intern/readfile.c:10119:4:     a = set_listbasepointers(mainvar, lbarray);
./source/blender/blenloader/intern/readfile.c:10623:6:   int i = set_listbasepointers(mainptr, lbarray);
./source/blender/blenloader/intern/readfile.c:10787:6:   int a = set_listbasepointers(mainvar, lbarray);
./source/blender/blenloader/intern/readfile.c:10854:6:   int a = set_listbasepointers(mainvar, lbarray);
./source/blender/blenloader/intern/readfile.c:10903:6:   int a = set_listbasepointers(mainvar, lbarray);
./source/blender/blenloader/intern/versioning_270.c:1144:4:     a = set_listbasepointers(bmain, lbarray);
./source/blender/blenloader/intern/writefile.c:3380:8:     a = tot = set_listbasepointers(main, lbarray);
./source/blender/blenloader/intern/writefile.c:3563:8:     int a = set_listbasepointers(bmain, lbarray);
./source/blender/editors/space_outliner/outliner_tree.c:1433:4:     tot = set_listbasepointers(mainvar, lbarray);
./source/blender/editors/space_outliner/outliner_tree.c:1501:4:     tot = set_listbasepointers(mainvar, lbarray);
./source/blender/windowmanager/intern/wm_files_link.c:799:2:   lba_idx = set_listbasepointers(bmain, lbarray);
./source/blender/windowmanager/intern/wm_files_link.c:908:2:   lba_idx = set_listbasepointers(bmain, lbarray);
./source/blender/windowmanager/intern/wm_files_link.c:922:2:   lba_idx = set_listbasepointers(bmain, lbarray);

Remove redundant check

Harbormaster completed remote builds in B10016: Diff 28592.Sep 7 2020, 11:03 AM

Campbell Barton (campbellbarton) marked an inline comment as done.Sep 7 2020, 11:03 AM

The functionality is fine with me. I can't think of many practical uses, but might as well support it.

+1
I recently got confused when trying to link in an armature and nothing happened. Took me a minute to realize that I'm just dumb. It's good to avoid these kinds of confusions.

Simplify id-code check using BKE_idtype_idcode_from_index

Harbormaster completed remote builds in B10043: Diff 28638.Sep 8 2020, 2:51 AM

Campbell Barton (campbellbarton) mentioned this in rB748deced1c7b: Link/Append: support instancing object data.Sep 8 2020, 8:17 AM

Committed refactor rBe467c54d58c88316d959f2481dc7484037a4c0be & this patch with some minor changes rB748deced1c7b85009142d1ebbffba4c1f2405a3f

Campbell Barton (campbellbarton) mentioned this in D8843: Cleanup: de-duplicate code for instancing objects when linking.Sep 9 2020, 7:00 AM

Campbell Barton (campbellbarton) added inline comments.Sep 9 2020, 7:12 AM

source/blender/blenloader/intern/readfile.c
10382	Submitted D8843, although this only de-duplicates shared base operations.

Library Linking: support instantiating object data on append/link
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 28485

source/blender/blenloader/intern/readfile.c

source/blender/blenloader/intern/versioning_280.c

source/blender/makesdna/DNA_space_types.h

source/blender/windowmanager/intern/wm_files_link.c

Library Linking: support instantiating object data on append/linkAbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 28485

source/blender/blenloader/intern/readfile.c

source/blender/blenloader/intern/versioning_280.c

source/blender/makesdna/DNA_space_types.h

source/blender/windowmanager/intern/wm_files_link.c

Library Linking: support instantiating object data on append/link
AbandonedPublic
Actions

Revision Contents
Changeset List