Asset Browser: Duplicate/Reuse option for Current File asset library
Needs RevisionPublic
Actions

Authored by Julian Eisel (Severin) on Oct 15 2021, 3:07 PM.

Details

Reviewers

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

Group Reviewers

Asset Browser

Maniphest Tasks

T92077: Add option to re-use or duplicate data for the Current File asset library

Summary

In the Asset Browser while showing the Current File asset library, we now show a menu to choose between Duplicate or Reuse. This is similar to the menu for external asset libraries, that allows selecting between Link, Append and Append (Reuse Data).
{Append (Reuse Data)} and Reuse should behave as consistent as possible with each other.

Unfortunately we can't currently do a generic duplication in the more asset-specific code. Dupliation is ID type specific and in some cases there also needs to be special handling (e.g. local objects are always duplicated, because otherwise a drop would only transform the object). So the duplication has to happen in the operators executed on dropping.
Basic idea of the patch:

Only the Asset Browser knows about the option, so it has to let the dragged item know if it wants it to be duplicated or reused on drop.
On drop, the copy() callback of a drop-box sets a "duplicate" property of the operator, if available, based on what the drag item requests.

Diff Detail

Repository

rB Blender

Branch

temp-asset-browser-instance-method (branched from master)

Build Status

Buildable 17875
Build 17875: arc lint + arc unit

Event Timeline

Julian Eisel (Severin) requested review of this revision.Oct 15 2021, 3:07 PM

Julian Eisel (Severin) created this revision.

Harbormaster completed remote builds in B17875: Diff 43378.Oct 15 2021, 3:07 PM

Julian Eisel (Severin) mentioned this in rB4ba72015465d: Asset Browser: Hide Import Types menu for "Current File" asset library.Oct 15 2021, 6:54 PM

Julian Eisel (Severin) edited the summary of this revision. (Show Details)Oct 15 2021, 6:56 PM

Julian Eisel (Severin) added a project: Asset Browser.Oct 15 2021, 6:58 PM

Julian Eisel (Severin) added a reviewer: Bastien Montagne (mont29).

Julian Eisel (Severin) added inline comments.

source/blender/blenloader/intern/versioning_300.c
1082	I can do this renaming in master already. But goes in hand with this new option.

Jeroen Bakker (jbakker) added a subscriber: Jeroen Bakker (jbakker).Oct 19 2021, 8:29 AM

Jeroen Bakker (jbakker) added inline comments.

source/blender/blenloader/intern/versioning_300.c
1082	Yes, please do small cleanups direct in master. But then again ... can't we use `dna_rename_defs`?
source/blender/editors/object/object_relations.c
2649	(deep) copy is a developer term. `Full Copy` we use when copying scenes for example.
source/blender/windowmanager/WM_types.h
968	Its unclear if this is obsolete code...

This will also need some update once we show current file assets also in asset libraries that are not the "Current File". There's a UI question to this since we'd basically have to display both menus (import method and instance method) then. But I think we can just stick to the import method menu and determine the instance method so that it has matching behavior. So Link and Append (Reuse Data) will use the Reuse behavior, Append uses the Duplicate behavior.

source/blender/blenloader/intern/versioning_300.c
1082	Not worth doing that for .blends saved in beta versions. I'll just make sure the version patch is applied again, by moving it to the other one for `instance_method`.

wmDragIDInstantiateFlags, eFileAssetInstanceMethod, and uiBut.dragflag seem to be tightly coupled. However, there is no mention of one at any of the others. Such coupling should be documented well.

source/blender/editors/interface/interface.c
6215–6224	I don't quite like the design of having an `_ex` function that takes two booleans. At the call level it makes it quite unclear what these booleans are. The two bits of code (the old `UI_but_drag_set_id` and the setting of the new drag flags) also don't share anything that makes it hard to break them up. I would suggest keeping `UI_but_drag_set_id` as-is, and to add `UI_but_drag_set_instanciation_flags()` for the new code.
6231–6236	This means that both "duplicate" and "reuse" flags can be set. I don't think that's a valid combination.
source/blender/editors/interface/interface_intern.h
99–102	Same issue as `wmDragIDInstantiateFlags`: they are named "flags" and have bitflag values, yet conceptually "duplicate" and "reuse" are mutually exclusive. This exclusivity should be documented.
726	If this returns a `uiBut.dragflag`, this should be either the return type or at least be documented.
source/blender/editors/space_view3d/space_view3d.c
641–642	I don't understand this. To me "import" means "from another blend file"; however, "dragged an asset" does not imply "from another file" as the above function is named `...get_local_ID_or...`. How can one be concluded from the other?
source/blender/editors/space_view3d/view3d_edit.c
4937	Either remove `(deep)` or remove the parentheses. It's either important enough to say, so without parentheses, or it's not important enough and then it should just be removed.
source/blender/makesdna/DNA_space_types.h
827–828	These should get some documentation as to what they mean. "Instance" is a word with so many meanings, we shouldn't leave it to the reader to guess which one is meant here. In the same line, maybe `instancing_method` is a better name too, as that at least gramatically limits the possible meanings.
843	Add comment to explain that this is used when using/dragging a current-file asset into the current file.
source/blender/makesrna/intern/rna_space.c
6648–6654	Consistency: both tooltips start with "Create a new instance of the asset", but then they diverge: The first explains what it means to create a new instance ("by doing X") The second does not explain this, and even goes against what the user may think it does ("but Y"). The first is fine, but the second is not explaining things very well. It should explain what it does, not how it goes against what you think it may do. It should also not rely on people having read the first tooltip. Something like this would be an improvement IMO: "Just use the asset from the current file; if necessary a new instance is created while reusing as much data as possible."
6680–6681	What does this comment mean? What is the relation between a GPU operation (redrawing) with "drag info"?
source/blender/windowmanager/WM_types.h
968	I'd go one step further: remove commented-out code, or very clearly & explicitly document why it should be kept, and under which conditions it can be either uncommented or removed.
974	No they're not, because in `wmDragFlags` they're commented out. If certain enum values need to share the same namespace (in terms of having unique values across multiple enums), this should not only be documented, but also the enums should be defined in such a way that this is enforced in code. Further down `r_instantiate_flags = 0;` is used, with `wmDragIDInstantiateFlags r_instantiate_flags`. The zero value is not a valid value for `wmDragIDInstantiateFlags`, as it is `WM_DRAG_NOP` which is defined in another enum. It's clear that this needs more documentation to clarify the design. Most importantly, it should be clear why these flags are shared at all, but still not defined in the same enum.
976	These don't seem to be bitflags, but actually distinct choices. You cannot duplicate and reuse at the same time. This means they can get simple values (1, 2, 3, etc.) instead of being encoded as bitflags. If these are to remain bitflags, they should be documented as mutually exclusive.
1001	Why not just declare as `eFileAssetImportMethod import_method;`? AFAIK this is just runtime data, so it doesn't have to adhere to the limitations of the DNA system.
source/blender/windowmanager/intern/wm_dragdrop.c
411	Document that `import_method` is an `eFileAssetImportMethod` value.