Page MenuHome

Asset Browser: "Mark Asset" & "Clear Asset" operators and UI integration
ClosedPublic

Authored by Julian Eisel (Severin) on Dec 2 2020, 5:25 PM.

Details

Summary

Exposes "Mark Asset" and "Clear Asset" in Outliner context menus and button
context menus.

The Outliner can tell operators which IDs are selected via a new "selected_ids"
context member. We can use that for more operators and editors in future.
Other UI code can set a new "id" context member (automatically done for ID
pointer property buttons). That way, data-block search menus or other buttons
representing a data-block (e.g. materal slots) can hand an ID to operators via
context.

Diff Detail

Repository
rB Blender
Branch
temp-review-asset-create-ui (branched from master)
Build Status
Buildable 11598
Build 11598: arc lint + arc unit

Event Timeline

Julian Eisel (Severin) requested review of this revision.Dec 2 2020, 5:26 PM
source/blender/blenkernel/intern/icons.c
333

This is only to prevent a crash when using "Make Asset" for objects. This is fixed with other changes in the asset-browser branch to add proper object preview support.

source/blender/editors/asset/asset_edit.c
41

I'm unsure about this. Maybe use id_us_ensure_real() instead?

I don't think "focused" has to be in the name. For example for modifiers there is also a modifier context member for the focused modifier. I would use just id.

From a UI point of view, is there a clear asset operator coming along with this? Will there be a visual indication in the outliner for which datablocks are assets (an icon like linked/overrides have for example)?

I guess these are more design questions that do not necessarily need to be handled in code review, but they'd help make this operator feel more complete.

source/blender/editors/asset/asset_ops.c
89

Is it possible to make assets of all datablocks?

I'd guess that only linkable/appendable datablocks would support it, and probably no window manager, screen or key datablock for example?

132

This description needs to be expanded. A hint that the asset can be viewed and its metadata edited in the asset browser would also be good.

I'm also not sure that "asset management" is a good term to use.

From a UI point of view, is there a clear asset operator coming along with this?

A clear asset operator should definitely come, T82654. Should be easy to add so I should probably just do it.

Will there be a visual indication in the outliner for which datablocks are assets (an icon like linked/overrides have for example)?

Think that makes a lot of sense. I first wanted to gray out the "Make Asset" when a data-block already is an asset (with a disabled hint!), but that's difficult due to the way outliner operations are implemented, and the fact that poll functions shouldn't check over the selected_ids list for performance reasons.

source/blender/blenloader/intern/writefile.c
96 ↗(On Diff #31538)

Unnecessary of course.

source/blender/editors/asset/asset_ops.c
89

Indeed, this should be more selective.

  • Use "id" as context member, not "focused_id"
  • Do not allow making non-linkable data-blocks assets
  • General cleanup of the "Make Asset" oeprator
  • Improve "Make Asset" tooltip
  • Add "Remove Asset-Data" operator

Correct base branch

  • Fix "Make Asset" failing for collections in the Outliner
  • Fix leftover "focused_id", causing "Make Asset" on material slots to fail

Correct base branch... again...

Code looks fine to me, but I find the naming still weak.

The operator idname should match the operator name whenever possible. But I'm not sure I like either name, "unmake" sounds odd to me, and "asset-data" is not a term we use elsewhere and it's unclear what exactly that means nor is it obviously the opposite of "make asset".

Personally I still prefer the terminology "mark asset" / "clear asset", since make implies some new entity is created, while really what we are doing is marking datablocks. Not that they are ideal either, but it's the best I can think of.

Agreed, I was never quite happy with the name either. Mark Asset and Clear Asset seem fine for now. We can tweak it later if we find something better.

  • Rename "Make Asset" & "Remove Asset Data" to "Mark Asset" and "Clear Asset"
This revision is now accepted and ready to land.Dec 7 2020, 6:11 PM
Julian Eisel (Severin) retitled this revision from Asset Browser: "Make Asset" operator and UI integration to Asset Browser: "Mark Asset" & "Clear Asset" operators and UI integration.Dec 7 2020, 8:27 PM
Julian Eisel (Severin) edited the summary of this revision. (Show Details)

@Bastien Montagne (mont29) one question for you, how should user counting for asset data-blocks be handled? Currently Mark Asset adds a fake user, but I think it could just be a real one? Otherwise Clear Asset should probably remove the fake user I suppose, which would be an issue if it was the user who set the fake user first.
I think the ensure-real thing wouldn't be what's wanted. So I think it should either be a real user or we don't clear the fake user (like done here).

@Bastien Montagne (mont29) one question for you, how should user counting for asset data-blocks be handled? Currently Mark Asset adds a fake user, but I think it could just be a real one? Otherwise Clear Asset should probably remove the fake user I suppose, which would be an issue if it was the user who set the fake user first.
I think the ensure-real thing wouldn't be what's wanted. So I think it should either be a real user or we don't clear the fake user (like done here).

Until T61209 is implemented you can only use fake user. Do not, ever, mess with ID user count if you do not actually have a real ID user (that would be listed in our 'foreach_id' system with user refcounting tag).

Oh, and you can only set fake user, clear asset should not clear fake user, since something else might also have set it.

Alright, thanks! That means I can leave the code as is and only remove the TODO comment :)