From f9113c4be836691ba599aab9b2f43e26333f8133 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Mon, 18 Oct 2021 12:33:53 +0200 Subject: [PATCH] Assets: add global `bke::AssetLibraryService` class Add `blender::bke::AssetLibraryService` class that acts like a blendfile-scoped singleton. It's allocated upon the first call to `BKE_asset_library_load` and destroyed in the LOAD-PRE handler. The `AssetLibraryService` ensures that edits to asset catalogs are not lost when the asset browser editor closes (or even reloads). Instead, the `AssetLibrary` pointers it owns are kept around as long as the blend file is open. Reviewed By: Severin Maniphest Tasks: T92151 Differential Revision: https://developer.blender.org/D12885 --- source/blender/blenkernel/BKE_asset_library.h | 9 +- .../blender/blenkernel/BKE_asset_library.hh | 8 +- source/blender/blenkernel/CMakeLists.txt | 3 + .../blenkernel/intern/asset_library.cc | 29 ++-- .../intern/asset_library_service.cc | 136 ++++++++++++++++++ .../intern/asset_library_service.hh | 90 ++++++++++++ .../intern/asset_library_service_test.cc | 101 +++++++++++++ .../blenkernel/intern/asset_library_test.cc | 29 +++- source/blender/editors/space_file/filelist.c | 6 +- 9 files changed, 388 insertions(+), 23 deletions(-) create mode 100644 source/blender/blenkernel/intern/asset_library_service.cc create mode 100644 source/blender/blenkernel/intern/asset_library_service.hh create mode 100644 source/blender/blenkernel/intern/asset_library_service_test.cc diff --git a/source/blender/blenkernel/BKE_asset_library.h b/source/blender/blenkernel/BKE_asset_library.h index 12ca55c0ef4..b4674a8d932 100644 --- a/source/blender/blenkernel/BKE_asset_library.h +++ b/source/blender/blenkernel/BKE_asset_library.h @@ -29,9 +29,14 @@ extern "C" { /** Forward declaration, defined in intern/asset_library.hh */ typedef struct AssetLibrary AssetLibrary; -/** TODO(@sybren): properly have a think/discussion about the API for this. */ +/** + * Return the #AssetLibrary rooted at the given directory path. + * + * Will return the same pointer for repeated calls, until another blend file is loaded. + * + * To get the in-memory-only "current file" asset library, pass an empty path. + */ struct AssetLibrary *BKE_asset_library_load(const char *library_path); -void BKE_asset_library_free(struct AssetLibrary *asset_library); /** * Try to find an appropriate location for an asset library root from a file or directory path. diff --git a/source/blender/blenkernel/BKE_asset_library.hh b/source/blender/blenkernel/BKE_asset_library.hh index 419df2a1061..b8b4b0f8447 100644 --- a/source/blender/blenkernel/BKE_asset_library.hh +++ b/source/blender/blenkernel/BKE_asset_library.hh @@ -33,9 +33,15 @@ namespace blender::bke { +/** + * AssetLibrary provides access to an asset library's data. + * For now this is only for catalogs, later this can be expanded to indexes/caches/more. + */ struct AssetLibrary { std::unique_ptr catalog_service; + ~AssetLibrary(); + void load(StringRefNull library_root_directory); /** @@ -52,7 +58,7 @@ struct AssetLibrary { void on_save_post(struct Main *, struct PointerRNA **pointers, const int num_pointers); private: - bCallbackFuncStore on_save_callback_store_; + bCallbackFuncStore on_save_callback_store_{}; }; } // namespace blender::bke diff --git a/source/blender/blenkernel/CMakeLists.txt b/source/blender/blenkernel/CMakeLists.txt index 47c1d698360..a86c6fbd3dd 100644 --- a/source/blender/blenkernel/CMakeLists.txt +++ b/source/blender/blenkernel/CMakeLists.txt @@ -90,6 +90,7 @@ set(SRC intern/asset_catalog.cc intern/asset_catalog_path.cc intern/asset_library.cc + intern/asset_library_service.cc intern/attribute.c intern/attribute_access.cc intern/attribute_math.cc @@ -472,6 +473,7 @@ set(SRC intern/CCGSubSurf_inline.h intern/CCGSubSurf_intern.h intern/attribute_access_intern.hh + intern/asset_library_service.hh intern/data_transfer_intern.h intern/lib_intern.h intern/multires_inline.h @@ -801,6 +803,7 @@ if(WITH_GTESTS) intern/armature_test.cc intern/asset_catalog_test.cc intern/asset_catalog_path_test.cc + intern/asset_library_service_test.cc intern/asset_library_test.cc intern/asset_test.cc intern/cryptomatte_test.cc diff --git a/source/blender/blenkernel/intern/asset_library.cc b/source/blender/blenkernel/intern/asset_library.cc index 5956a4af0cb..48ace8efea1 100644 --- a/source/blender/blenkernel/intern/asset_library.cc +++ b/source/blender/blenkernel/intern/asset_library.cc @@ -31,6 +31,8 @@ #include "MEM_guardedalloc.h" +#include "asset_library_service.hh" + #include /** @@ -39,19 +41,17 @@ */ struct AssetLibrary *BKE_asset_library_load(const char *library_path) { - blender::bke::AssetLibrary *lib = new blender::bke::AssetLibrary(); - lib->on_save_handler_register(); - lib->load(library_path); + blender::bke::AssetLibraryService *service = blender::bke::AssetLibraryService::get(); + blender::bke::AssetLibrary *lib; + if (library_path == nullptr || library_path[0] == '\0') { + lib = service->get_asset_library_current_file(); + } + else { + lib = service->get_asset_library_on_disk(library_path); + } return reinterpret_cast(lib); } -void BKE_asset_library_free(struct AssetLibrary *asset_library) -{ - blender::bke::AssetLibrary *lib = reinterpret_cast(asset_library); - lib->on_save_handler_unregister(); - delete lib; -} - bool BKE_asset_library_find_suitable_root_path_from_path(const char *input_path, char *r_library_path) { @@ -102,6 +102,13 @@ void BKE_asset_library_refresh_catalog_simplename(struct AssetLibrary *asset_lib namespace blender::bke { +AssetLibrary::~AssetLibrary() +{ + if (on_save_callback_store_.func) { + on_save_handler_unregister(); + } +} + void AssetLibrary::load(StringRefNull library_root_directory) { auto catalog_service = std::make_unique(library_root_directory); @@ -134,6 +141,8 @@ void AssetLibrary::on_save_handler_register() void AssetLibrary::on_save_handler_unregister() { BKE_callback_remove(&on_save_callback_store_, BKE_CB_EVT_SAVE_POST); + on_save_callback_store_.func = nullptr; + on_save_callback_store_.arg = nullptr; } void AssetLibrary::on_save_post(struct Main *main, diff --git a/source/blender/blenkernel/intern/asset_library_service.cc b/source/blender/blenkernel/intern/asset_library_service.cc new file mode 100644 index 00000000000..14604def0d3 --- /dev/null +++ b/source/blender/blenkernel/intern/asset_library_service.cc @@ -0,0 +1,136 @@ +/* + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +/** \file + * \ingroup bke + */ + +#include "asset_library_service.hh" + +#include "BKE_asset_library.hh" +#include "BKE_blender.h" +#include "BKE_callbacks.h" + +#include "BLI_string_ref.hh" + +#include "MEM_guardedalloc.h" + +#include "CLG_log.h" + +static CLG_LogRef LOG = {"bke.asset_service"}; + +namespace blender::bke { + +std::unique_ptr AssetLibraryService::instance_; +bool AssetLibraryService::atexit_handler_registered_ = false; + +AssetLibraryService *AssetLibraryService::get() +{ + if (!instance_) { + allocate_service_instance(); + } + return instance_.get(); +} + +void AssetLibraryService::destroy() +{ + if (!instance_) { + return; + } + instance_->app_handler_unregister(); + instance_.reset(); +} + +AssetLibrary *AssetLibraryService::get_asset_library_on_disk(StringRefNull top_level_directory) +{ + BLI_assert_msg(!top_level_directory.is_empty(), + "top level directory must be given for on-disk asset library"); + + AssetLibraryPtr *lib_uptr_ptr = on_disk_libraries_.lookup_ptr(top_level_directory); + if (lib_uptr_ptr != nullptr) { + CLOG_INFO(&LOG, 2, "get \"%s\" (cached)", top_level_directory.c_str()); + return lib_uptr_ptr->get(); + } + + AssetLibraryPtr lib_uptr = std::make_unique(); + AssetLibrary *lib = lib_uptr.get(); + + lib->on_save_handler_register(); + lib->load(top_level_directory); + + on_disk_libraries_.add_new(top_level_directory, std::move(lib_uptr)); + CLOG_INFO(&LOG, 2, "get \"%s\" (loaded)", top_level_directory.c_str()); + return lib; +} + +AssetLibrary *AssetLibraryService::get_asset_library_current_file() +{ + if (current_file_library_) { + CLOG_INFO(&LOG, 2, "get current file lib (cached)"); + } + else { + CLOG_INFO(&LOG, 2, "get current file lib (loaded)"); + current_file_library_ = std::make_unique(); + current_file_library_->on_save_handler_register(); + } + + AssetLibrary *lib = current_file_library_.get(); + return lib; +} + +void AssetLibraryService::allocate_service_instance() +{ + instance_ = std::make_unique(); + instance_->app_handler_register(); + + if (!atexit_handler_registered_) { + /* Ensure the instance gets freed before Blender's memory leak detector runs. */ + BKE_blender_atexit_register([](void * /*user_data*/) { AssetLibraryService::destroy(); }, + nullptr); + atexit_handler_registered_ = true; + } +} + +static void on_blendfile_load(struct Main * /*bMain*/, + struct PointerRNA ** /*pointers*/, + const int /*num_pointers*/, + void * /*arg*/) +{ + AssetLibraryService::destroy(); +} + +/** + * Ensure the AssetLibraryService instance is destroyed before a new blend file is loaded. + * This makes memory management simple, and ensures a fresh start for every blend file. */ +void AssetLibraryService::app_handler_register() +{ + /* The callback system doesn't own `on_load_callback_store_`. */ + on_load_callback_store_.alloc = false; + + on_load_callback_store_.func = &on_blendfile_load; + on_load_callback_store_.arg = this; + + BKE_callback_add(&on_load_callback_store_, BKE_CB_EVT_LOAD_PRE); +} + +void AssetLibraryService::app_handler_unregister() +{ + BKE_callback_remove(&on_load_callback_store_, BKE_CB_EVT_LOAD_PRE); + on_load_callback_store_.func = nullptr; + on_load_callback_store_.arg = nullptr; +} + +} // namespace blender::bke diff --git a/source/blender/blenkernel/intern/asset_library_service.hh b/source/blender/blenkernel/intern/asset_library_service.hh new file mode 100644 index 00000000000..63ffe56ab74 --- /dev/null +++ b/source/blender/blenkernel/intern/asset_library_service.hh @@ -0,0 +1,90 @@ +/* + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +/** \file + * \ingroup bke + */ + +#pragma once + +#ifndef __cplusplus +# error This is a C++-only header file. +#endif + +#include "BKE_asset_library.hh" + +#include "BLI_map.hh" + +#include + +namespace blender::bke { + +/** + * Global singleton-ish that provides access to individual #AssetLibrary instances. + * + * Whenever a blend file is loaded, the existing instance of AssetLibraryService is destructed, and + * a new one is created -- hence the "singleton-ish". This ensures only information about relevant + * asset libraries is loaded. + * + * \note How Asset libraries are identified may change in the future. + * For now they are assumed to be: + * - on disk (identified by the absolute directory), or + * - the "current file" library (which is in memory but could have catalogs + * loaded from a file on disk). + */ +class AssetLibraryService { + public: + using AssetLibraryPtr = std::unique_ptr; + + AssetLibraryService() = default; + ~AssetLibraryService() = default; + + /** Return the AssetLibraryService singleton, allocating it if necessary. */ + static AssetLibraryService *get(); + + /** Destroy the AssetLibraryService singleton. It will be reallocated by #get() if necessary. */ + static void destroy(); + + /** + * Get the given asset library. Opens it (i.e. creates a new AssetLibrary instance) if necessary. + */ + AssetLibrary *get_asset_library_on_disk(StringRefNull top_level_directory); + + /** Get the "Current File" asset library. */ + AssetLibrary *get_asset_library_current_file(); + + protected: + static std::unique_ptr instance_; + + /* Mapping absolute path of the library's top-level directory to the AssetLibrary instance. */ + Map on_disk_libraries_; + AssetLibraryPtr current_file_library_; + + /* Handlers for managing the life cycle of the AssetLibraryService instance. */ + bCallbackFuncStore on_load_callback_store_; + static bool atexit_handler_registered_; + + /** Allocate a new instance of the service and assign it to `instance_`. */ + static void allocate_service_instance(); + + /** + * Ensure the AssetLibraryService instance is destroyed before a new blend file is loaded. + * This makes memory management simple, and ensures a fresh start for every blend file. */ + void app_handler_register(); + void app_handler_unregister(); +}; + +} // namespace blender::bke diff --git a/source/blender/blenkernel/intern/asset_library_service_test.cc b/source/blender/blenkernel/intern/asset_library_service_test.cc new file mode 100644 index 00000000000..6acbe193eb7 --- /dev/null +++ b/source/blender/blenkernel/intern/asset_library_service_test.cc @@ -0,0 +1,101 @@ +/* + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + * + * The Original Code is Copyright (C) 2020 Blender Foundation + * All rights reserved. + */ + +#include "asset_library_service.hh" + +#include "CLG_log.h" + +#include "testing/testing.h" + +namespace blender::bke::tests { + +class AssetLibraryServiceTest : public testing::Test { + public: + CatalogFilePath asset_library_root_; + + static void SetUpTestSuite() + { + CLG_init(); + } + static void TearDownTestSuite() + { + CLG_exit(); + } + + void SetUp() override + { + const std::string test_files_dir = blender::tests::flags_test_asset_dir(); + if (test_files_dir.empty()) { + FAIL(); + } + asset_library_root_ = test_files_dir + "/" + "asset_library"; + } + + void TearDown() override + { + AssetLibraryService::destroy(); + } +}; + +TEST_F(AssetLibraryServiceTest, get_destroy) +{ + AssetLibraryService *const service = AssetLibraryService::get(); + EXPECT_EQ(service, AssetLibraryService::get()) + << "Calling twice without destroying in between should return the same instance."; + + AssetLibraryService::destroy(); + EXPECT_NE(service, AssetLibraryService::get()) + << "Calling twice with destroying in between should return a new instance."; + + /* This should not crash. */ + AssetLibraryService::destroy(); + AssetLibraryService::destroy(); +} + +TEST_F(AssetLibraryServiceTest, library_pointers) +{ + AssetLibraryService *service = AssetLibraryService::get(); + AssetLibrary *const lib = service->get_asset_library_on_disk(asset_library_root_); + AssetLibrary *const curfile_lib = service->get_asset_library_current_file(); + + EXPECT_EQ(lib, service->get_asset_library_on_disk(asset_library_root_)) + << "Calling twice without destroying in between should return the same instance."; + EXPECT_EQ(curfile_lib, service->get_asset_library_current_file()) + << "Calling twice without destroying in between should return the same instance."; + + AssetLibraryService::destroy(); + service = AssetLibraryService::get(); + EXPECT_NE(lib, service->get_asset_library_on_disk(asset_library_root_)) + << "Calling twice with destroying in between should return a new instance."; + EXPECT_NE(curfile_lib, service->get_asset_library_current_file()) + << "Calling twice with destroying in between should return a new instance."; +} + +TEST_F(AssetLibraryServiceTest, catalogs_loaded) +{ + AssetLibraryService *const service = AssetLibraryService::get(); + AssetLibrary *const lib = service->get_asset_library_on_disk(asset_library_root_); + AssetCatalogService *const cat_service = lib->catalog_service.get(); + + const bUUID UUID_POSES_ELLIE("df60e1f6-2259-475b-93d9-69a1b4a8db78"); + EXPECT_NE(nullptr, cat_service->find_catalog(UUID_POSES_ELLIE)) + << "Catalogs should be loaded after getting an asset library from disk."; +} + +} // namespace blender::bke::tests diff --git a/source/blender/blenkernel/intern/asset_library_test.cc b/source/blender/blenkernel/intern/asset_library_test.cc index 30ac4dc6ad8..7c3587c6dfa 100644 --- a/source/blender/blenkernel/intern/asset_library_test.cc +++ b/source/blender/blenkernel/intern/asset_library_test.cc @@ -21,11 +21,32 @@ #include "BKE_asset_catalog.hh" #include "BKE_asset_library.hh" +#include "asset_library_service.hh" + +#include "CLG_log.h" + #include "testing/testing.h" namespace blender::bke::tests { -TEST(AssetLibraryTest, load_and_free_c_functions) +class AssetLibraryServiceTest : public testing::Test { + public: + static void SetUpTestSuite() + { + CLG_init(); + } + static void TearDownTestSuite() + { + CLG_exit(); + } + + void TearDown() override + { + AssetLibraryService::destroy(); + } +}; + +TEST_F(AssetLibraryServiceTest, bke_asset_library_load) { const std::string test_files_dir = blender::tests::flags_test_asset_dir(); if (test_files_dir.empty()) { @@ -50,11 +71,9 @@ TEST(AssetLibraryTest, load_and_free_c_functions) AssetCatalog *poses_ellie = service->find_catalog(uuid_poses_ellie); ASSERT_NE(nullptr, poses_ellie) << "unable to find POSES_ELLIE catalog"; EXPECT_EQ("character/Ellie/poselib", poses_ellie->path.str()); - - BKE_asset_library_free(library_c_ptr); } -TEST(AssetLibraryTest, load_nonexistent_directory) +TEST_F(AssetLibraryServiceTest, load_nonexistent_directory) { const std::string test_files_dir = blender::tests::flags_test_asset_dir(); if (test_files_dir.empty()) { @@ -75,8 +94,6 @@ TEST(AssetLibraryTest, load_nonexistent_directory) /* Check that the catalog service doesn't have any catalogs. */ EXPECT_TRUE(service->is_empty()); - - BKE_asset_library_free(library_c_ptr); } } // namespace blender::bke::tests diff --git a/source/blender/editors/space_file/filelist.c b/source/blender/editors/space_file/filelist.c index 773a321da5c..fc502b065f3 100644 --- a/source/blender/editors/space_file/filelist.c +++ b/source/blender/editors/space_file/filelist.c @@ -391,7 +391,7 @@ typedef struct FileList { eFileSelectType type; /* The library this list was created for. Stored here so we know when to re-read. */ AssetLibraryReference *asset_library_ref; - struct AssetLibrary *asset_library; + struct AssetLibrary *asset_library; /* Non-owning pointer. */ short flags; @@ -1847,9 +1847,7 @@ void filelist_clear_ex(struct FileList *filelist, } if (do_asset_library && (filelist->asset_library != NULL)) { - /* There is no way to refresh the catalogs stored by the AssetLibrary struct, so instead of - * "clearing" it, the entire struct is freed. It will be reallocated when needed. */ - BKE_asset_library_free(filelist->asset_library); + /* The AssetLibraryService owns the AssetLibrary pointer, so no need for us to free it. */ filelist->asset_library = NULL; } }