blender
/
blender
137
416
Fork 599
Code Issues 6.3k Pull Requests 774 Projects 19 Wiki Activity

UI: Asset Shelf (Experimental Feature) #104831

Closed
Julian Eisel wants to merge 399 commits from asset-shelf into main
pull from: asset-shelf
merge into: blender:main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 30 Commits 399 Files Changed - +13 -5
1 changed files with 13 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 5fc17af505 - Show all commits

18
source/blender/editors/asset/intern/asset_shelf_asset_view.cc
View File

@ -32,6 +32,7 @@ namespace blender::ed::asset::shelf {
class AssetView : public ui::AbstractGridView {
const AssetLibraryReference library_ref_;
const AssetShelfSettings &shelf_settings_;
std::optional<asset_system::AssetCatalogFilter> catalog_filter_ = std::nullopt;
/* XXX Temporary: Only for #asset_poll__() callback. Should use traits instead. */
bContext &evil_C_;
@ -39,7 +40,9 @@ class AssetView : public ui::AbstractGridView {
friend class AssetDragController;
public:
AssetView(const AssetLibraryReference &library_ref, bContext &evil_C);
AssetView(const AssetLibraryReference &library_ref,
const AssetShelfSettings &shelf_settings,
bContext &evil_C);
void build_items() override;
@ -67,8 +70,10 @@ class AssetDragController : public ui::AbstractViewItemDragController {
void *create_drag_data() const override;
};
AssetView::AssetView(const AssetLibraryReference &library_ref, bContext &evil_C)
: library_ref_(library_ref), evil_C_(evil_C)
AssetView::AssetView(const AssetLibraryReference &library_ref,
const AssetShelfSettings &shelf_settings,
bContext &evil_C)
: library_ref_(library_ref), shelf_settings_(shelf_settings), evil_C_(evil_C)
{
}
@ -117,9 +122,11 @@ void AssetView::build_items()
return true;
}
const bool show_names = (shelf_settings_.display_flag & ASSETSHELF_SHOW_NAMES);
/* Use the path within the library as identifier, this should be unique. */
const StringRef identifier = ED_asset_handle_get_relative_path(asset);
const StringRef name = ED_asset_handle_get_name(&asset);
const StringRef name = show_names ? ED_asset_handle_get_name(&asset) : "";
const int preview_id = ED_asset_handle_get_preview_icon_id(&asset);
add_item<AssetViewItem>(asset, identifier, name, preview_id);
@ -188,7 +195,8 @@ void build_asset_view(uiLayout &layout,
const float tile_width = ED_asset_shelf_default_tile_width();
const float tile_height = ED_asset_shelf_default_tile_height();
JulianEisel marked this conversation as resolved Outdated
Hans Goudey commented 2023-06-06 19:28:01 +02:00
Outdated
Review
Copy Link

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->

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->`
Julian Eisel commented 2023-06-12 16:27:15 +02:00
Outdated
Review
Copy Link

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

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
Hans Goudey commented 2023-06-12 16:38:02 +02:00
Outdated
Review
Copy Link

Hard to argue with the code guidelines I guess.. Still though, I'd find it clearer to use static_cast, since dynamic_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

Hard to argue with the code guidelines I guess.. Still though, I'd find it clearer to use `static_cast`, since `dynamic_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
std::unique_ptr asset_view = std::make_unique<AssetView>(library_ref, const_cast<bContext &>(C));
std::unique_ptr asset_view = std::make_unique<AssetView>(
library_ref, *shelf_settings, const_cast<bContext &>(C));
asset_view->set_catalog_filter(catalog_filter_from_shelf_settings(shelf_settings, *library));
asset_view->set_tile_size(tile_width, tile_height);