UI: Asset Shelf (Experimental Feature) #104831
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
7 Participants
Notifications
Due Date
No due date set.
Blocks
Depends on
#104546 Pose Library: Update to use the asset shelf (when enabled)
blender/blender-addons
#110589 Nodes: Add asset shelf support to node editor
blender/blender
#109016 UI: Add type-safe C++ button apply function object
blender/blender
Reference: blender/blender#104831
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "asset-shelf"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.AssetShelfHook
toRegionAssetShelf
7b1da46441Did a number of changes, including:
main
which help improving the patchmain
directly.AssetShelfHook
toRegionAssetShelf
. 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.
@blender-bot package
Package build started. Download here when ready.
Accepting with some requests.
Also some suggestions which I don't consider blocking.
Note: committed inclusion of "Asset Shelf" in the region-toggle pie menu (
9572bfe883
).@ -0,0 +1,36 @@
# SPDX-License-Identifier: GPL-2.0-or-later
Use updated copyright convention
SPDX-FileCopyrightText
.@ -1275,2 +1276,4 @@
layout.prop(view, "show_region_ui")
layout.prop(view, "show_region_tool_header")
if prefs.experimental.use_asset_shelf:
layout.prop(view, "show_regions_asset_shelf")
Suggestion: it would be nice if this was greyed out when the asset menu isn't shown, when first testing this patch I found it confusing that toggling the region did nothing until entering sculpt mode.
Submitted as separate PR now since I'd like to get some feedback without delaying the initial asset shelf merge: #110756.
@ -416,0 +422,4 @@
typedef enum AssetShelfTypeFlag {
/** Do not trigger asset dragging on drag events. Drag events can be overridden with custom
* keymap items then. */
ASSET_SHELF_TYPE_NO_ASSET_DRAG = (1 << 0),
Suggestion: calling
ASSET_SHELF_TYPE_*
prefix reads like a type which are typically unique values, not flags.ASSET_SHELF_TYPE_FLAG_NO_ASSET_DRAG
- some extra clarity at the expense of verbosity.@ -52,6 +52,10 @@
#include "BLO_read_write.h"
/* TODO(Julian): For asset shelf region reading/writing. Region read/write should be done via a
picky use
TODO(@username)
format.@ -0,0 +1,697 @@
/* SPDX-License-Identifier: GPL-2.0-or-later */
Use updated copyright format
@ -0,0 +99,4 @@
static void activate_shelf(RegionAssetShelf &shelf_regiondata, AssetShelf &shelf)
{
shelf_regiondata.active_shelf = &shelf;
BLI_remlink_safe(&shelf_regiondata.shelves, &shelf);
Suggestion: I find it reads baddy when it's the existence of data in a list is undefined.
In this case the caller can ensure it's in the list in the case it's just been created.
@ -427,0 +430,4 @@
* they will be upscaled to the button size. Should probably be done by the widget code. */
const int is_preview_flag = (BKE_icon_is_preview(preview_icon_id) ||
BKE_icon_is_image(preview_icon_id)) ?
UI_BUT_ICON_PREVIEW :
-Wextra
warns:grid_view.cc:432:68: warning: enumerated and non-enumerated type in conditional expression
.int(UI_BUT_ICON_PREVIEW)
.@ -859,6 +859,22 @@ static void rna_Space_show_region_hud_update(bContext *C, PointerRNA *ptr)
rna_Space_bool_from_region_flag_update_by_type(C, ptr, RGN_TYPE_HUD, RGN_FLAG_HIDDEN_BY_USER);
}
/* Asset Shelf Regions */
This should probably be called
show_region_asset_shelf
? (regions
->region
). Even though internally disabling the asset shelf also disables the asset-shelf header, this is more of a detail. The HEADER & TOOL_HEADER have the same logic without naming the property "regions".Tested it a bit. I'm getting crashes when disabling asset names and when changing the size slider.
Smaller nitpicks (Not necessaryily related to this PR):
Sybren A. Stüvel referenced this pull request from blender/blender-addons2023-08-03 11:30:32 +02:00
Thanks for the feedback @JulienKaspar!
Which commit are you on? Sounds like a crash that I fixed earlier.
Hmm, I don't really see that delay here. Something we can look at. And I'm also not sure why there's no sliding animation, from the code I think it should do it already.
For the header I find it fine, but indeed with the tool settings at the bottom it's awkward indeed. Do people have both at the bottom even? (Took me a moment to find the Flip to Bottom for it. You have to click close to a button or on one, then go into the Header [?!] submenu, ...)
Yeah I used to have a workaround for that in code, but not too nice to merge that into
main
(trivial to bring back though). If you recreate the 3D View from scratch, that is create an entirely new area that never contained a 3D view before, it should be fine :)BTW I also did some brief performance tests, to make sure the asset shelf doesn't slow down animation playback especially. Seems all fine, couldn't spot any issues.
Ah I think it's because of the aggressive region size snapping the asset shelf does. The animation changes the size, but because of the snapping you don't really see the intermediate states. That would also make it seem laggy (although it doesn't feel like that to me, interestingly..). I guess the region snapping should be skipped during the animation.
Latest commit. I built it on my machine.
There's a preference for it. Even some at the Studio are using it to set all headers to the bottom:
Interface -> Editors -> "Header Position"
But they also tend to not use the tool settings, so maybe it's ok.
I guess when the header is at the bottom, the asset shelf should be at the top?
@blender-bot build
Committed as:
Thanks all!
Pull request closed