UI: Asset Shelf (Experimental Feature) #104831
Closed
Julian Eisel
wants to merge 399 commits from
When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
asset-shelf into main
pull from: asset-shelf
merge into: blender:main
blender:main
blender:asset-browser-frontend-split
blender:blender-v3.3-release
blender:blender-v3.6-release
blender:universal-scene-description
blender:temp-sculpt-dyntopo
blender:brush-assets-project
blender:anim/armature-drawing-refactor-3
blender:temp-sculpt-dyntopo-hive-alloc
blender:tmp-usd-python-mtl
blender:tmp-usd-3.6
blender:blender-v3.5-release
blender:blender-projects-basics
blender:blender-v2.93-release
blender:temp-sculpt-attr-api
blender:realtime-clock
blender:sculpt-dev
blender:gpencil-next
blender:bevelv2
blender:microfacet_hair
blender:xr-dev
blender:principled-v2
When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
This introduces asset shelves as a new standard UI element for accessing assets. Based on the current context (like the active mode and/or tool), they can provide assets for specific workflows/tasks. As such they are more limited in functionality than the asset browser, but a lot more efficient for certain tasks.
The asset shelf is developed as part of the brush assets project (#101895: Brush Asset Project), but is also meant to serve as the next version of the pose library UI.
Support for asset shelves can quite easily be added to different editor types, this PR does so for the 3D view. If an editor type supports asset shelves, add-ons can chose to register an asset shelf type for an editor with just a few lines of Python.
It should be possible to entirely remove
UILayout.asset_view_template()once this is in use.Some changes are to be expected still.
Task: #102879
Brush asset workflow blog post: https://code.blender.org/2022/12/brush-assets-workflow/
Initial technical documentation: https://developer.blender.org/docs/asset_system/user_interface/asset_shelf/
How to test
There are 3 easy ways to test the asset shelf - the experimental Asset Shelf option needs to be enabled:
Pose Library
asset-shelfadd-ons branch to be checked out,make updatedoes this.Python Template
Brush Assets (Extended Asset Browser)
Note that the work is still ongoing, but this should be ready to be merged into main soon, at least as experimental feature. See #107498 for the remaining pre-merge TODOs.
Tasks:
#107784: Name filtering for Asset Shelf assetsUpdates to Pose Library Add-on: #104546: WIP: Update Pose Library add-on to use the asset shelf
@blender-bot build
For example, the pose library add-on can now register an asset shelf like this: ``` class VIEW3D_AST_pose_library(bpy.types.AssetShelf): bl_space_type = "VIEW_3D" @classmethod def poll(cls, context: Context) -> bool: return PoseLibraryPanel.poll(context) ``` Filtering by ID type is not supported yet. This replaces the hack of registering a header type and the asset shelf template to draw into that.Julian Eisel referenced this pull request2023-03-30 13:01:58 +02:00
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
WIP: Asset Shelfto UI: Asset Shelf (Experimental Feature)@blender-bot build
@ -0,0 +193,4 @@void AssetViewItem::build_context_menu(bContext &C, uiLayout &column) const{const AssetView &asset_view = dynamic_cast<AssetView &>(get_view());If the dynamic_cast isn't meant to fail in some cases, it's probably better to use
static_castto avoid bloating the code with dynamic casting.Also, class methods should generally be accessed with
this->Not a fan of using
static_castfor down casting. It removes type safety for virtually (pun intended) no benefit. Sure it's unlikely to cause issues here, but if it does it's good to get an exception thrown. There's a language feature designed for this, so I rather use it.https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-dynamic_cast
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c147-use-dynamic_cast-to-a-reference-type-when-failure-to-find-the-required-class-is-considered-an-error
Hard to argue with the code guidelines I guess.. Still though, I'd find it clearer to use
static_cast, sincedynamic_castgives the impression that the author thinks the cast might fail. But using a reference for the variable negates that impression, making the whole thing confusing. Not a big deal though@ -0,0 +28,4 @@#include "asset_shelf.hh"using namespace blender;using namespace blender::ed::asset;These
usingstatements shouldn't be necessary, looks like the whole file is in that namespace.I left some notes on the UI in #107881, and on the asset shelf types design in #102879.
The asset shelf implementation itself looks generally ok from a cursory look, but I have not reviewed in the detail.
The purpose of some of the changes to other UI code are unclear to me, and as mentioned before would be good to have as separate PRs so we can understand better what each one does.
Just as a heads up: I've already merged a bunch of smaller general changes into
main, created some patches and some more will follow. A bit of a slow process but I'm on it. (Patch went from 77 changed files to 58 already.)@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
I read through all the code, don't really have a lot to say about the implementation specifics. Mainly wondering if we really need the hooks and dedicated region type for settings.
@ -8248,0 +8264,4 @@return bool(context.object and context.object.mode == 'SCULPT')@classmethoddef asset_poll__(cls, asset):What is the meaning of this double underscore at the end? I know single underscore at the start indicates a private method, but this I have not seen anywhere else in Blender.
Checking the name elsewhere this is temporary, so perhaps call it
asset_poll_temp_api- however in this case it seems like it should be resolved before considering the PR final.While this was initially meant as temporary design, I think it's fine to keep this callback in the API. It adds more flexibility in deciding which assets should be shown in the shelf, so you don't have to rely purely on traits.
Plus it's unclear if we can get the necessary Python API syntax to work before the release (the
bl_asset_traits = {{"Object", "Mesh"}, {"Material"}}kind of syntax), so I prefer not relying on that. Should we get it to work, the API can still be changed before the release.Changed the code now so this is a regular (non-temporary) API callback.
@ -0,0 +439,4 @@{static const char *context_dir[] = {"asset_shelf","asset_library_ref",Don't abbreviate things in the public API like this, ref -> reference.
We already use
"asset_library_ref"in a number of other places in the API. Noted in #102877 as potential 4.0 change.Ok, I would not break API compatibility over something minor like this.
@ -2220,0 +2286,4 @@/* regions: asset shelf settings */art = MEM_cnew<ARegionType>("spacetype view3d asset shelf settings region");art->regionid = RGN_TYPE_ASSET_SHELF_SETTINGS;It's unclear to me why this exists instead of using
RGN_TYPE_TEMPORARY, or really just a regular popover like we have for e.g. shading and overlay settings in the 3D viewport.Is it because you want list scrolling to be persistent or something like that?
It's a bit of a strange precedent to have a region specifically for a popover.
This isn't for the popover, it's for the part of the shelf with the tabs, catalog selector, search button etc. This needs a different kind of root layout, and needs to be independently scrollable. That's why it's a separate region.
Can you call it asset shelf header then? Since we don't normally call the things in this region "settings" in Blender.
I used to call it footer when it was still at the bottom, but thought it makes sense to make it position independent. But fair enough if we can avoid confusion this way.
@ -361,6 +361,8 @@ typedef struct View3D {/** Path to the viewer node that is currently previewed. This is retrieved from the workspace. */ViewerPath viewer_path;struct AssetShelfHook *asset_shelf_hook;Why does this exist rather than storing everything in
ARegion.regiondata? If it's in the regiondata it doesn't need to get added to every space type, and is more self contained and generic.I'm also not sure how to interpret the term "hook" in this context.
Also find the term hook awkward, hooks are often used in the context of callbacks, it might make sense if the data stored in hooks was a way to hook into data used elsewhere. Where as this looks like a collection. So it could be
AssetShelfCollectionorAssetShelfStore... or similar (personally prefer "Store" as it's not confused with Object Collections).Am fine with another name. Just kept it consistent with
WorkSpaceHook(not that this is a great name either).Using regiondata for storage didn't cross my mind, especially since there are two asset shelf regions. But we can still let the main asset shelf region keep the storage. Might simplify things a bit. I will look into it.
Just to note since this isn't obvious: One main reason there's a dedicated
AssetShelfHookstruct is so that can be reused easily in different editors, e.g. to support asset shelves in the Node Editor, Image Editor (e.g. for brushes), etc.@ -496,2 +496,4 @@#define ND_SPACE_FILE_PREVIEW (21 << 16)#define ND_SPACE_SPREADSHEET (22 << 16)/* Not a space itself, but a part of another space. */#define ND_SPACE_ASSET_SHELF (23 << 16)Then call it
ND_REGION_ASSET_SHELFinstead, if it's not a space?No strong opinion, see above regarding the asset shelf not being a single region. Maybe
ND_REGIONS_ASSET_SHELF(plural).That sounds fine.
@ -908,0 +900,4 @@/* The region size may be set dynamically through #ARegionType.layout(). So if the region is* only hidden because it's too small, still run the layout in case that leads to a different* region size. */const bool hidden_because_too_small = (region->flag & RGN_FLAG_TOO_SMALL) &&Why remove
RGN_FLAG_DYNAMIC_SIZE? Is there a region without this flag that has a dynamic size anyway?The meaning of
RGN_FLAG_DYNAMIC_SIZEchanged a bit. I think it should be renamed toRGN_FLAG_SIZE_LAYOUT_BASED, since it indicates that the size is entirely determined by the layout. The asset shelf region does use a self-controlled size (so it's dynamically sized in a sense), but it's not exactly determined by the layout. It can't use theRGN_FLAG_DYNAMIC_SIZE, which disables region resizing for example.I think for the logic here it makes sense to not consider the flag. If the region is too small but it has a layout function, execute that so it can change the size. It must still be tagged for redraw anyway.
Ok, that's reasonable.
Made some changes in
main, so this wasn't needed anymore:4319c211dc.@ -0,0 +209,4 @@{const AssetView &asset_view = dynamic_cast<const AssetView &>(get_view());const AssetShelfType &shelf_type = *asset_view.shelf_.type;shelf_type.draw_context_menu(&C, &shelf_type, &asset_, &column);This is crashing on a simple test (default sculpt brush made into an asset).
@ -0,0 +40,4 @@next = prev = nullptr;active_catalog_path = BLI_strdup(other.active_catalog_path);This can be null (noted in doc-string), crashes when duplicating an empty 3D view.
@ -5007,3 +5008,3 @@#define PREVIEW_TILE_PAD (0.15f * UI_UNIT_X)int UI_preview_tile_size_x()int UI_preview_tile_size_x(const int unscaled_size)If this is the size in pixels, it could be called
size_px(not that fussed though).@ -1759,1 +1794,4 @@}if (flag & ED_KEYMAP_ASSET_SHELF) {/* standard keymap for asset shelf regions */wmKeyMap *keymap = WM_keymap_ensure(wm->defaultconf, "Asset Shelf", 0, 0);There is currently a warning that this is an empty key-map, could this have at least one item added? (otherwise hard-code disable the warning): see
source/blender/windowmanager/intern/wm_keymap.c:457 WM_keymap_poll: empty keymap 'Asset Shelf'.@ -339,0 +356,4 @@static void view3d_init(wmWindowManager * /*wm*/, ScrArea *area){BLI_assert(area->spacetype == SPACE_VIEW3D);View3D *v3d = static_cast<View3D *>(area->spacedata.first);This caused a crash however it exposed an existing issue: committed fix
486086d349@ -1081,0 +1205,4 @@}SpaceType *space_type = BKE_spacetype_from_id(dummy_shelf_type.space_type);if (!space_type) {Shouldn't this report an error?
EDIT or be impossible (that block could use
BLI_assert_unreachable()).Still planning to rename
AssetShelfHooktoAssetShelfStore, but it's moved to the region data now.Did a number of changes, including:
mainwhich help improving the patchmaindirectly.AssetShelfHooktoRegionAssetShelf. We wanted a better name and this is now region-data, so that name made sense (consistent withRegionView3D).Quick test to add asset shelf support to the node editor: #110589
Looks fine overall to me now, certainly on the level of an experimental feature with the data design in good shape.