From 2bfaf94fa7603dcf522261e202116a7d7664fb38 Mon Sep 17 00:00:00 2001 From: Julian Eisel Date: Fri, 23 Feb 2024 18:36:04 +0100 Subject: [PATCH] Refactor: Lazy create & update asset catalog trees Rebuilding the tree immediately after changes could cause the tree to be rebuilt multiple times. More importantly, it made it harder to reason about thread safety, since we would touch the tree within a whole bunch of API functions. Now tree building is simplified and managed in a single place, so making the tree building thread safe can be made trivially in a follow-up. Note, this means the initial catalog tree building doesn't happen in a background thread together with loading the asset library and catalogs anymore. But we would already do all further rebuilds on the main thread anyway, this shouldn't have any notable impact. --- .../blender/asset_system/AS_asset_catalog.hh | 20 ++++----- .../asset_system/intern/asset_catalog.cc | 44 +++++++++++-------- .../asset_system/intern/asset_library_all.cc | 2 - .../asset_system/tests/asset_catalog_test.cc | 8 ++-- .../tests/asset_catalog_tree_test.cc | 10 ++--- .../editors/asset/intern/asset_filter.cc | 5 +-- .../space_file/asset_catalog_tree_view.cc | 2 +- 7 files changed, 46 insertions(+), 45 deletions(-) diff --git a/source/blender/asset_system/AS_asset_catalog.hh b/source/blender/asset_system/AS_asset_catalog.hh index d66ffe25031..52b0797a057 100644 --- a/source/blender/asset_system/AS_asset_catalog.hh +++ b/source/blender/asset_system/AS_asset_catalog.hh @@ -41,7 +41,11 @@ using OwningAssetCatalogMap = Map>; * directory hierarchy). */ class AssetCatalogService { std::unique_ptr catalog_collection_; - std::unique_ptr catalog_tree_; + /** + * Cached catalog tree storage. Lazy-created by #AssetCatalogService::catalog_tree() (hence + * mutable). + */ + mutable std::unique_ptr catalog_tree_; CatalogFilePath asset_library_root_; Vector> undo_snapshots_; @@ -130,15 +134,6 @@ class AssetCatalogService { */ void reload_catalogs(); - /** - * Make sure the tree is updated to the latest collection of catalogs stored in this service. - * Does not depend on a CDF file being available so this can be called on a service that stores - * catalogs that are not stored in a CDF. - * Most API functions that modify catalog data will trigger this, unless otherwise specified (for - * batch operations). - */ - void rebuild_tree(); - /** Return catalog with the given ID. Return nullptr if not found. */ AssetCatalog *find_catalog(CatalogID catalog_id) const; @@ -183,7 +178,7 @@ class AssetCatalogService { */ void update_catalog_path(CatalogID catalog_id, const AssetCatalogPath &new_catalog_path); - const AssetCatalogTree *get_catalog_tree() const; + const AssetCatalogTree &catalog_tree() const; /** Return true only if there are no catalogs known. */ bool is_empty() const; @@ -256,6 +251,9 @@ class AssetCatalogService { const CatalogFilePath &blend_file_path); std::unique_ptr read_into_tree() const; + /** Ensure a #catalog_tree() will update the tree. Must be called whenever the contained user + * visible catalogs change. */ + void invalidate_catalog_tree(); /** * For every catalog, ensure that its parent path also has a known catalog. diff --git a/source/blender/asset_system/intern/asset_catalog.cc b/source/blender/asset_system/intern/asset_catalog.cc index 78c9d6181f9..3c21eaf4a09 100644 --- a/source/blender/asset_system/intern/asset_catalog.cc +++ b/source/blender/asset_system/intern/asset_catalog.cc @@ -43,8 +43,7 @@ const std::string AssetCatalogDefinitionFile::HEADER = "# Other lines are of the format \"UUID:catalog/path/for/assets:simple catalog name\"\n"; AssetCatalogService::AssetCatalogService() - : catalog_collection_(std::make_unique()), - catalog_tree_(std::make_unique()) + : catalog_collection_(std::make_unique()) { } @@ -237,7 +236,7 @@ void AssetCatalogService::prune_catalogs_by_path(const AssetCatalogPath &path) this->delete_catalog_by_id_soft(cat_id); } - this->rebuild_tree(); + this->invalidate_catalog_tree(); AssetLibraryService::get()->tag_all_library_catalogs_dirty(); } @@ -272,7 +271,8 @@ void AssetCatalogService::update_catalog_path(const CatalogID catalog_id, * blend file, and update the catalog simple name stored there. */ } - this->rebuild_tree(); + this->create_missing_catalogs(); + this->invalidate_catalog_tree(); AssetLibraryService::get()->tag_all_library_catalogs_dirty(); } @@ -297,8 +297,8 @@ AssetCatalog *AssetCatalogService::create_catalog(const AssetCatalogPath &catalo catalog_collection_->catalog_definition_file_->add_new(catalog_ptr); } - BLI_assert_msg(catalog_tree_, "An Asset Catalog tree should always exist."); - catalog_tree_->insert_item(*catalog_ptr); + this->create_missing_catalogs(); + this->invalidate_catalog_tree(); AssetLibraryService::get()->tag_all_library_catalogs_dirty(); return catalog_ptr; @@ -340,7 +340,8 @@ void AssetCatalogService::load_from_disk(const CatalogFilePath &file_or_director /* TODO: Should there be a sanitize step? E.g. to remove catalogs with identical paths? */ - this->rebuild_tree(); + this->create_missing_catalogs(); + this->invalidate_catalog_tree(); } void AssetCatalogService::add_from_existing( @@ -439,7 +440,8 @@ void AssetCatalogService::reload_catalogs() cdf->parse_catalog_file(cdf->file_path, catalog_parsed_callback); this->purge_catalogs_not_listed(cats_in_file); - this->rebuild_tree(); + this->create_missing_catalogs(); + this->invalidate_catalog_tree(); } void AssetCatalogService::purge_catalogs_not_listed(const Set &catalogs_to_keep) @@ -488,7 +490,7 @@ bool AssetCatalogService::write_to_disk(const CatalogFilePath &blend_file_path) } this->untag_has_unsaved_changes(); - this->rebuild_tree(); + this->invalidate_catalog_tree(); return true; } @@ -574,11 +576,6 @@ std::unique_ptr AssetCatalogService::construct_cdf_i return cdf; } -const AssetCatalogTree *AssetCatalogService::get_catalog_tree() const -{ - return catalog_tree_.get(); -} - std::unique_ptr AssetCatalogService::read_into_tree() const { auto tree = std::make_unique(); @@ -591,10 +588,17 @@ std::unique_ptr AssetCatalogService::read_into_tree() const return tree; } -void AssetCatalogService::rebuild_tree() +void AssetCatalogService::invalidate_catalog_tree() { - this->create_missing_catalogs(); - this->catalog_tree_ = this->read_into_tree(); + this->catalog_tree_ = nullptr; +} + +const AssetCatalogTree &AssetCatalogService::catalog_tree() const +{ + if (!catalog_tree_) { + catalog_tree_ = read_into_tree(); + } + return *catalog_tree_; } void AssetCatalogService::create_missing_catalogs() @@ -653,7 +657,8 @@ void AssetCatalogService::undo() redo_snapshots_.append(std::move(catalog_collection_)); catalog_collection_ = undo_snapshots_.pop_last(); - this->rebuild_tree(); + this->create_missing_catalogs(); + this->invalidate_catalog_tree(); AssetLibraryService::get()->tag_all_library_catalogs_dirty(); } @@ -664,7 +669,8 @@ void AssetCatalogService::redo() undo_snapshots_.append(std::move(catalog_collection_)); catalog_collection_ = redo_snapshots_.pop_last(); - this->rebuild_tree(); + this->create_missing_catalogs(); + this->invalidate_catalog_tree(); AssetLibraryService::get()->tag_all_library_catalogs_dirty(); } diff --git a/source/blender/asset_system/intern/asset_library_all.cc b/source/blender/asset_system/intern/asset_library_all.cc index dbdc9d1f219..d36073e5c2c 100644 --- a/source/blender/asset_system/intern/asset_library_all.cc +++ b/source/blender/asset_system/intern/asset_library_all.cc @@ -56,8 +56,6 @@ void AllAssetLibrary::rebuild_catalogs_from_nested(const bool reload_nested_cata }, false); - new_catalog_service->rebuild_tree(); - this->catalog_service = std::move(new_catalog_service); catalogs_dirty_ = false; } diff --git a/source/blender/asset_system/tests/asset_catalog_test.cc b/source/blender/asset_system/tests/asset_catalog_test.cc index dbb1783ec92..76f6773ae16 100644 --- a/source/blender/asset_system/tests/asset_catalog_test.cc +++ b/source/blender/asset_system/tests/asset_catalog_test.cc @@ -566,8 +566,8 @@ TEST_F(AssetCatalogTest, delete_catalog_leaf) "path/without/simplename", }; - const AssetCatalogTree *tree = service.get_catalog_tree(); - AssetCatalogTreeTestFunctions::expect_tree_items(*tree, expected_paths); + const AssetCatalogTree &tree = service.catalog_tree(); + AssetCatalogTreeTestFunctions::expect_tree_items(tree, expected_paths); } TEST_F(AssetCatalogTest, delete_catalog_parent_by_id) @@ -620,8 +620,8 @@ TEST_F(AssetCatalogTest, delete_catalog_parent_by_path) "path/without/simplename", }; - const AssetCatalogTree *tree = service.get_catalog_tree(); - AssetCatalogTreeTestFunctions::expect_tree_items(*tree, expected_paths); + const AssetCatalogTree &tree = service.catalog_tree(); + AssetCatalogTreeTestFunctions::expect_tree_items(tree, expected_paths); } TEST_F(AssetCatalogTest, delete_catalog_write_to_disk) diff --git a/source/blender/asset_system/tests/asset_catalog_tree_test.cc b/source/blender/asset_system/tests/asset_catalog_tree_test.cc index f458af232a7..dcf002062fd 100644 --- a/source/blender/asset_system/tests/asset_catalog_tree_test.cc +++ b/source/blender/asset_system/tests/asset_catalog_tree_test.cc @@ -117,8 +117,8 @@ TEST_F(AssetCatalogTreeTest, load_single_file_into_tree) "path/without/simplename", /* From CDF. */ }; - const AssetCatalogTree *tree = service.get_catalog_tree(); - expect_tree_items(*tree, expected_paths); + const AssetCatalogTree &tree = service.catalog_tree(); + expect_tree_items(tree, expected_paths); } TEST_F(AssetCatalogTreeTest, foreach_in_tree) @@ -141,8 +141,8 @@ TEST_F(AssetCatalogTreeTest, foreach_in_tree) service.load_from_disk(asset_library_root_ + SEP_STR + "blender_assets.cats.txt"); std::vector expected_root_items{{"character", "path"}}; - const AssetCatalogTree *tree = service.get_catalog_tree(); - expect_tree_root_items(*tree, expected_root_items); + const AssetCatalogTree &tree = service.catalog_tree(); + expect_tree_root_items(tree, expected_root_items); /* Test if the direct children of the root item are what's expected. */ std::vector> expected_root_child_items = { @@ -152,7 +152,7 @@ TEST_F(AssetCatalogTreeTest, foreach_in_tree) {"path/without"}, }; int i = 0; - tree->foreach_root_item([&expected_root_child_items, &i](const AssetCatalogTreeItem &item) { + tree.foreach_root_item([&expected_root_child_items, &i](const AssetCatalogTreeItem &item) { expect_tree_item_child_items(item, expected_root_child_items[i]); i++; }); diff --git a/source/blender/editors/asset/intern/asset_filter.cc b/source/blender/editors/asset/intern/asset_filter.cc index 116b876f9ff..1ea97e53319 100644 --- a/source/blender/editors/asset/intern/asset_filter.cc +++ b/source/blender/editors/asset/intern/asset_filter.cc @@ -75,7 +75,7 @@ asset_system::AssetCatalogTree build_filtered_catalog_tree( /* Build catalog tree. */ asset_system::AssetCatalogTree filtered_tree; - const asset_system::AssetCatalogTree &full_tree = *library.catalog_service->get_catalog_tree(); + const asset_system::AssetCatalogTree &full_tree = library.catalog_service->catalog_tree(); full_tree.foreach_item([&](const asset_system::AssetCatalogTreeItem &item) { if (!known_paths.contains(item.catalog_path().str())) { return; @@ -135,8 +135,7 @@ AssetItemTree build_filtered_all_catalog_tree( }); asset_system::AssetCatalogTree catalogs_with_node_assets; - const asset_system::AssetCatalogTree &catalog_tree = - *library->catalog_service->get_catalog_tree(); + const asset_system::AssetCatalogTree &catalog_tree = library->catalog_service->catalog_tree(); catalog_tree.foreach_item([&](const asset_system::AssetCatalogTreeItem &item) { if (assets_per_path.lookup(item.catalog_path()).is_empty()) { return; diff --git a/source/blender/editors/space_file/asset_catalog_tree_view.cc b/source/blender/editors/space_file/asset_catalog_tree_view.cc index 56ac3bacaac..41e0974a8ea 100644 --- a/source/blender/editors/space_file/asset_catalog_tree_view.cc +++ b/source/blender/editors/space_file/asset_catalog_tree_view.cc @@ -184,7 +184,7 @@ AssetCatalogTreeView::AssetCatalogTreeView(asset_system::AssetLibrary *library, : asset_library_(library), params_(params), space_file_(space_file) { if (library && library->catalog_service) { - catalog_tree_ = library->catalog_service->get_catalog_tree(); + catalog_tree_ = &library->catalog_service->catalog_tree(); } else { catalog_tree_ = nullptr;