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:blender-v4.3-release
blender:npr-prototype
blender:cmbasnett/use_pass_z_name
blender:cmbasnett/bdk-v4.3
blender:blender-v4.2-release
blender:color-node-drag-and-drop
blender:blender-v3.6-release
blender:temp-sculpt-dyntopo
blender:icons-cleanup
blender:blender-v3.3-release
blender:brush-assets-project
blender:pr-extensions-tidy-space
blender:blender-v4.0-release
blender:universal-scene-description
blender:blender-v4.1-release
blender:blender-v3.6-temp_wmoss_animrig_public
blender:gpencil-next
blender:blender-projects-basics
blender:bridge-curves
blender:sculpt-blender
blender:asset-browser-frontend-split
blender:tmp-usd-python-mtl
blender:tmp-usd-3.6
blender:blender-v3.5-release
blender:blender-v2.93-release
blender:sculpt-dev
blender:bevelv2
blender:xr-dev
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-shelf
add-ons branch to be checked out,make update
does 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
nullopt
return value instead of{}
2a7571fa41Julian Eisel referenced this pull request2023-03-30 13:01:58 +02:00
make update
7ecb23962ebpy.types.AssetShelf
subclass 73a2c3453c@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_cast
to avoid bloating the code with dynamic casting.Also, class methods should generally be accessed with
this->
Not a fan of using
static_cast
for 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_cast
gives 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
using
statements shouldn't be necessary, looks like the whole file is in that namespace.using namespace
s, correct comment c13ab04280evil_C
) storage 9ca39993baI 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')
@classmethod
def 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
AssetShelfCollection
orAssetShelfStore
... 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
AssetShelfHook
struct 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_SHELF
instead, 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_SIZE
changed 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
AssetShelfHook
toAssetShelfStore
, but it's moved to the region data now.