Assets: Expose option to reuse data-block data when appending
ClosedPublic
Actions

Authored by Julian Eisel (Severin) on Sep 27 2021, 12:44 PM.

Details

Reviewers

Bastien Montagne (mont29)
Sybren A. Stüvel (sybren)

Group Reviewers

Asset Browser

Commits

rBef29bf9023f5: Assets: Expose option to reuse data-block data when appending

Summary

With 794c2828af60 & f48a4aa0f915 it's possible to reuse possibly
expensive, nested data of a data-block when appending. E.g. the
texture of a material, or the mesh of an object. Without this it's easy
to bloat memory and the file size. Duplicated textures also cause
unnecessary shader recompilations.

The feature was intended to be the new default behavior for the Asset
Browser, but it wasn't actually added to the UI yet. This patch adds a
new import type option to the Asset Browser. So from the menu in the
header, you can now choose between:

Link
Append
Append (Reuse Data)

The latter is the new default.

Differential Task: https://developer.blender.org/T91741

Diff Detail

Repository: rB Blender

Event Timeline

Julian Eisel (Severin) requested review of this revision.Sep 27 2021, 12:44 PM

Julian Eisel (Severin) created this revision.

Harbormaster completed remote builds in B17331: Diff 42477.Sep 27 2021, 12:44 PM

Julian Eisel (Severin) edited the summary of this revision. (Show Details)Sep 27 2021, 3:13 PM

Julian Eisel (Severin) added a project: Asset Browser.

Julian Eisel (Severin) mentioned this in T91750: Use-after-free with the new ID reuse for appending.Sep 27 2021, 3:20 PM

LGTM.

source/blender/makesrna/intern/rna_space.c
6650–6652	Second sentence is implementation detail, has nothing to do in UI tip imho.

This revision is now accepted and ready to land.Sep 27 2021, 4:44 PM

Sybren A. Stüvel (sybren) requested changes to this revision.Sep 27 2021, 4:49 PM

Sybren A. Stüvel (sybren) added a subscriber: Sybren A. Stüvel (sybren).

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

source/blender/makesdna/DNA_space_types.h
816–823	I feel that, now that the old Link/Append behaviour is expanded upon, each option should get a description of what they mean. At least the new option should describe what is appended and what is reused (or point to another place in the source code where this is explained).
source/blender/makesrna/intern/rna_space.c
6650–6652	A reference to the original asset data-block is kept as a way to recognize it, but this is not a true link I think this is an implementation detail that could be removed from the tooltip. To me it's even confusing, as I would expect dragging an object in with APPEND, and then later dragging it in again with APPEND_REUSE, would still reuse the mesh data from the first append. However, this "reference ... as a way to recognize it" is not mentioned in the APPEND case, which could imply that the mesh will get appended twice. The space in the tooltip could be used for a more elaborate description of what is actually reused; I don't think that just mentioning two data types ("textures, meshes") is clear enough to have an understanding of what gets appended and what gets reused.

This revision now requires changes to proceed.Sep 27 2021, 4:49 PM

Improve comments & tooltip

Harbormaster completed remote builds in B17346: Diff 42499.Sep 27 2021, 5:42 PM

Julian Eisel (Severin) added inline comments.Sep 27 2021, 5:42 PM

source/blender/makesrna/intern/rna_space.c
6650–6652	Changed the tooltip. It's a bit lengthy now, which I personally think is perfectly fine if it gives a good description of the functionality that way. Also noted that the data will be shared then, which is important to understand for users.

Minor notes, no need to re-review after this.

source/blender/makesrna/intern/rna_space.c
6652	There should be a comma after "object asset" (that makes "or the mesh of an object asset" a properly delimited sub-clause).
6653	To clarify the writing, I wouldn't describe what can happen, but rather what happens: "The instances of the asset share the data instead".