From debdde985fcf7d8bef9572582ac4b020e1172bcf Mon Sep 17 00:00:00 2001 From: Julian Eisel Date: Tue, 20 Feb 2024 15:48:30 +0100 Subject: [PATCH] Fix #108703: assert when loading "All" asset library Error caused by attempting to register multiple catalogs with the same catalog ID. This would happen when multiple asset libraries would use the same catalog definition file, and the "All" library would attempt to merge them all into one library. Ignore duplicate catalogs, like we already do when reading individual asset catalog definition files. Log an error instead, or a info log if the catalog path matches as well (in which case the conflict can be ignored as it won't be user visible). --- .../blender/asset_system/AS_asset_catalog.hh | 15 +++++++-- .../asset_system/intern/asset_catalog.cc | 33 +++++++++++++++---- .../asset_system/intern/asset_library_all.cc | 27 ++++++++++++++- 3 files changed, 65 insertions(+), 10 deletions(-) diff --git a/source/blender/asset_system/AS_asset_catalog.hh b/source/blender/asset_system/AS_asset_catalog.hh index 025a336cbd4..d9115644c2e 100644 --- a/source/blender/asset_system/AS_asset_catalog.hh +++ b/source/blender/asset_system/AS_asset_catalog.hh @@ -84,8 +84,13 @@ class AssetCatalogService { /** * Duplicate the catalogs from \a other_service into this one. Does not rebuild the tree, this * needs to be done by the caller (call #rebuild_tree()!). + * + * \note If a catalog from \a other already exists in this collection (identified by catalog ID), + * it will be skipped and \a on_duplicate_items will be called. */ - void add_from_existing(const AssetCatalogService &other_service); + void add_from_existing(const AssetCatalogService &other_service, + FunctionRef on_duplicate_items); /** * Write the catalog definitions to disk. @@ -298,11 +303,17 @@ class AssetCatalogCollection { AssetCatalogCollection(AssetCatalogCollection &&other) noexcept = default; std::unique_ptr deep_copy() const; + using OnDuplicateCatalogIdFn = + FunctionRef; /** * Copy the catalogs from \a other and append them to this collection. Copies no other data * otherwise. + * + * \note If a catalog from \a other already exists in this collection (identified by catalog ID), + * it will be skipped and \a on_duplicate_items will be called. */ - void add_catalogs_from_existing(const AssetCatalogCollection &other); + void add_catalogs_from_existing(const AssetCatalogCollection &other, + OnDuplicateCatalogIdFn on_duplicate_items); protected: static OwningAssetCatalogMap copy_catalog_map(const OwningAssetCatalogMap &orig); diff --git a/source/blender/asset_system/intern/asset_catalog.cc b/source/blender/asset_system/intern/asset_catalog.cc index 489ec4c227c..60547c1efca 100644 --- a/source/blender/asset_system/intern/asset_catalog.cc +++ b/source/blender/asset_system/intern/asset_catalog.cc @@ -344,9 +344,12 @@ void AssetCatalogService::load_from_disk(const CatalogFilePath &file_or_director this->rebuild_tree(); } -void AssetCatalogService::add_from_existing(const AssetCatalogService &other_service) +void AssetCatalogService::add_from_existing( + const AssetCatalogService &other_service, + AssetCatalogCollection::OnDuplicateCatalogIdFn on_duplicate_items) { - catalog_collection_->add_catalogs_from_existing(*other_service.catalog_collection_); + catalog_collection_->add_catalogs_from_existing(*other_service.catalog_collection_, + on_duplicate_items); } void AssetCatalogService::load_directory_recursive(const CatalogFilePath &directory_path) @@ -692,24 +695,40 @@ std::unique_ptr AssetCatalogCollection::deep_copy() cons return copy; } -static void copy_catalog_map_into_existing(const OwningAssetCatalogMap &source, - OwningAssetCatalogMap &dest) +static void copy_catalog_map_into_existing( + const OwningAssetCatalogMap &source, + OwningAssetCatalogMap &dest, + AssetCatalogCollection::OnDuplicateCatalogIdFn on_duplicate_items) { for (const auto &orig_catalog_uptr : source.values()) { + if (dest.contains(orig_catalog_uptr->catalog_id)) { + if (on_duplicate_items) { + on_duplicate_items(*dest.lookup(orig_catalog_uptr->catalog_id), *orig_catalog_uptr); + } + continue; + } + auto copy_catalog_uptr = std::make_unique(*orig_catalog_uptr); dest.add_new(copy_catalog_uptr->catalog_id, std::move(copy_catalog_uptr)); } } -void AssetCatalogCollection::add_catalogs_from_existing(const AssetCatalogCollection &other) +void AssetCatalogCollection::add_catalogs_from_existing( + const AssetCatalogCollection &other, + AssetCatalogCollection::OnDuplicateCatalogIdFn on_duplicate_items) { - copy_catalog_map_into_existing(other.catalogs_, catalogs_); + copy_catalog_map_into_existing(other.catalogs_, catalogs_, on_duplicate_items); } OwningAssetCatalogMap AssetCatalogCollection::copy_catalog_map(const OwningAssetCatalogMap &orig) { OwningAssetCatalogMap copy; - copy_catalog_map_into_existing(orig, copy); + copy_catalog_map_into_existing( + orig, copy, /*on_duplicate_items=*/[](const AssetCatalog &, const AssetCatalog &) { + /* `copy` was empty before. If this happens it means there was a duplicate in the `orig` + * catalog map which should've been caught already. */ + BLI_assert_unreachable(); + }); return copy; } diff --git a/source/blender/asset_system/intern/asset_library_all.cc b/source/blender/asset_system/intern/asset_library_all.cc index ab927920dff..6c71417ffd4 100644 --- a/source/blender/asset_system/intern/asset_library_all.cc +++ b/source/blender/asset_system/intern/asset_library_all.cc @@ -12,6 +12,10 @@ #include "asset_library_all.hh" +#include "CLG_log.h" + +static CLG_LogRef LOG = {"asset_system.all_asset_library"}; + namespace blender::asset_system { AllAssetLibrary::AllAssetLibrary() : AssetLibrary(ASSET_LIBRARY_ALL) {} @@ -28,9 +32,30 @@ void AllAssetLibrary::rebuild_catalogs_from_nested(const bool reload_nested_cata if (reload_nested_catalogs) { nested.catalog_service->reload_catalogs(); } - new_catalog_service->add_from_existing(*nested.catalog_service); + + new_catalog_service->add_from_existing( + *nested.catalog_service, + /*on_duplicate_items=*/[](const AssetCatalog &existing, + const AssetCatalog &to_be_ignored) { + if (existing.path == to_be_ignored.path) { + CLOG_INFO(&LOG, + 2, + "multiple definitions of catalog %s (path: %s), ignoring duplicate", + existing.catalog_id.str().c_str(), + existing.path.c_str()); + } + else { + CLOG_ERROR(&LOG, + "multiple definitions of catalog %s with differing paths (%s vs. %s), " + "ignoring second one", + existing.catalog_id.str().c_str(), + existing.path.c_str(), + to_be_ignored.path.c_str()); + } + }); }, false); + new_catalog_service->rebuild_tree(); this->catalog_service = std::move(new_catalog_service); }