diff --git a/source/blender/asset_system/AS_asset_library.hh b/source/blender/asset_system/AS_asset_library.hh index 72ebca53251..e04f3b57f94 100644 --- a/source/blender/asset_system/AS_asset_library.hh +++ b/source/blender/asset_system/AS_asset_library.hh @@ -127,13 +127,16 @@ class AssetLibrary { * \param relative_asset_path: The path of the asset relative to the asset library root. With * this the asset must be uniquely identifiable within the asset * library. + * \return A weak pointer to the new asset representation. The caller needs to keep some + * reference stored to be able to call #remove_asset(). This would be dangling once the + * asset library is destructed, so a weak pointer should be used to reference it. */ - AssetRepresentation &add_external_asset(StringRef relative_asset_path, - StringRef name, - int id_type, - std::unique_ptr metadata); + std::weak_ptr add_external_asset(StringRef relative_asset_path, + StringRef name, + int id_type, + std::unique_ptr metadata); /** See #AssetLibrary::add_external_asset(). */ - AssetRepresentation &add_local_id_asset(StringRef relative_asset_path, ID &id); + std::weak_ptr add_local_id_asset(StringRef relative_asset_path, ID &id); /** * Remove an asset from the library that was added using #add_external_asset() or * #add_local_id_asset(). Can usually be expected to be constant time complexity (worst case may diff --git a/source/blender/asset_system/intern/asset_library.cc b/source/blender/asset_system/intern/asset_library.cc index e3ffccdeefa..f5f618b8e88 100644 --- a/source/blender/asset_system/intern/asset_library.cc +++ b/source/blender/asset_system/intern/asset_library.cc @@ -208,17 +208,19 @@ AssetCatalogService &AssetLibrary::catalog_service() const void AssetLibrary::refresh_catalogs() {} -AssetRepresentation &AssetLibrary::add_external_asset(StringRef relative_asset_path, - StringRef name, - const int id_type, - std::unique_ptr metadata) +std::weak_ptr AssetLibrary::add_external_asset( + StringRef relative_asset_path, + StringRef name, + const int id_type, + std::unique_ptr metadata) { AssetIdentifier identifier = this->asset_identifier_from_library(relative_asset_path); return asset_storage_->add_external_asset( std::move(identifier), name, id_type, std::move(metadata), *this); } -AssetRepresentation &AssetLibrary::add_local_id_asset(StringRef relative_asset_path, ID &id) +std::weak_ptr AssetLibrary::add_local_id_asset(StringRef relative_asset_path, + ID &id) { AssetIdentifier identifier = this->asset_identifier_from_library(relative_asset_path); return asset_storage_->add_local_id_asset(std::move(identifier), id, *this); diff --git a/source/blender/asset_system/intern/asset_storage.cc b/source/blender/asset_system/intern/asset_storage.cc index 9738212d7d8..423a14978e5 100644 --- a/source/blender/asset_system/intern/asset_storage.cc +++ b/source/blender/asset_system/intern/asset_storage.cc @@ -17,21 +17,21 @@ namespace blender::asset_system { -AssetRepresentation &AssetStorage::add_local_id_asset(AssetIdentifier &&identifier, - ID &id, - const AssetLibrary &owner_asset_library) +std::weak_ptr AssetStorage::add_local_id_asset( + AssetIdentifier &&identifier, ID &id, const AssetLibrary &owner_asset_library) { - return *local_id_assets_.lookup_key_or_add( - std::make_unique(std::move(identifier), id, owner_asset_library)); + return local_id_assets_.lookup_key_or_add( + std::make_shared(std::move(identifier), id, owner_asset_library)); } -AssetRepresentation &AssetStorage::add_external_asset(AssetIdentifier &&identifier, - StringRef name, - const int id_type, - std::unique_ptr metadata, - const AssetLibrary &owner_asset_library) +std::weak_ptr AssetStorage::add_external_asset( + AssetIdentifier &&identifier, + StringRef name, + const int id_type, + std::unique_ptr metadata, + const AssetLibrary &owner_asset_library) { - return *external_assets_.lookup_key_or_add(std::make_unique( + return external_assets_.lookup_key_or_add(std::make_shared( std::move(identifier), name, id_type, std::move(metadata), owner_asset_library)); } diff --git a/source/blender/asset_system/intern/asset_storage.hh b/source/blender/asset_system/intern/asset_storage.hh index 21553e762bb..e7d99ed0cd9 100644 --- a/source/blender/asset_system/intern/asset_storage.hh +++ b/source/blender/asset_system/intern/asset_storage.hh @@ -25,10 +25,13 @@ class IDRemapper; namespace blender::asset_system { class AssetIdentifier; +class AssetLibrary; class AssetRepresentation; class AssetStorage { - using StorageT = Set>; + /* Uses shared pointers so the UI can aquire weak pointers. It can then ensure pointers are not + * dangling before accessing. */ + using StorageT = Set>; StorageT external_assets_; /* Store local ID assets separately for efficient lookups. @@ -38,15 +41,15 @@ class AssetStorage { public: /** See #AssetLibrary::add_external_asset(). */ - AssetRepresentation &add_external_asset(AssetIdentifier &&identifier, - StringRef name, - int id_type, - std::unique_ptr metadata, - const AssetLibrary &owner_asset_library); + std::weak_ptr add_external_asset(AssetIdentifier &&identifier, + StringRef name, + int id_type, + std::unique_ptr metadata, + const AssetLibrary &owner_asset_library); /** See #AssetLibrary::add_external_asset(). */ - AssetRepresentation &add_local_id_asset(AssetIdentifier &&identifier, - ID &id, - const AssetLibrary &owner_asset_library); + std::weak_ptr add_local_id_asset(AssetIdentifier &&identifier, + ID &id, + const AssetLibrary &owner_asset_library); /** See #AssetLibrary::remove_asset(). */ bool remove_asset(AssetRepresentation &asset); diff --git a/source/blender/asset_system/tests/asset_representation_test.cc b/source/blender/asset_system/tests/asset_representation_test.cc index d2ed2c2a5f1..3b7e48608e0 100644 --- a/source/blender/asset_system/tests/asset_representation_test.cc +++ b/source/blender/asset_system/tests/asset_representation_test.cc @@ -37,8 +37,9 @@ class AssetRepresentationTest : public AssetLibraryTestBase { AssetRepresentation &add_dummy_asset(AssetLibrary &library, StringRef relative_path) { std::unique_ptr dummy_metadata = std::make_unique(); - return library.add_external_asset( - relative_path, "Some asset name", 0, std::move(dummy_metadata)); + return *library + .add_external_asset(relative_path, "Some asset name", 0, std::move(dummy_metadata)) + .lock(); } }; diff --git a/source/blender/editors/space_file/filelist.cc b/source/blender/editors/space_file/filelist.cc index 2752e7132b9..4412b97530d 100644 --- a/source/blender/editors/space_file/filelist.cc +++ b/source/blender/editors/space_file/filelist.cc @@ -12,6 +12,7 @@ #include #include #include +#include #include #include @@ -86,20 +87,20 @@ using namespace blender; /* ------------------FILELIST------------------------ */ struct FileListInternEntry { - FileListInternEntry *next, *prev; + FileListInternEntry *next = nullptr, *prev = nullptr; - FileUID uid; + FileUID uid = 0; - eFileSel_File_Types typeflag; + eFileSel_File_Types typeflag = eFileSel_File_Types(0); /** ID type, in case typeflag has FILE_TYPE_BLENDERLIB set. */ - int blentype; + int blentype = 0; - char *relpath; + char *relpath = nullptr; /** Optional argument for shortcuts, aliases etc. */ - char *redirection_path; + char *redirection_path = nullptr; /** not strictly needed, but used during sorting, avoids to have to recompute it there... */ - const char *name; - bool free_name; + const char *name = nullptr; + bool free_name = false; /** * This is data from the current main, represented by this file. It's crucial that this is @@ -108,22 +109,45 @@ struct FileListInternEntry { */ struct { /** When showing local IDs (FILE_MAIN, FILE_MAIN_ASSET), the ID this file entry represents. */ - ID *id; + ID *id = nullptr; /* For the few file types that have the preview already in memory. For others, there's delayed * preview reading from disk. Non-owning pointer. */ - PreviewImage *preview_image; + PreviewImage *preview_image = nullptr; } local_data; - /* References an asset in the asset library storage. */ - asset_system::AssetRepresentation *asset; /* Non-owning. */ + /** + * References an asset in the asset library storage. + * The file list inserts this asset representation into the library, and removes it again when + * the file list is destructed. In that sense it manages the asset but doesn't own it. + * + * Weak pointer so access is protected in case the asset library gets destructed externally. + */ + std::weak_ptr asset; /* See #FILE_ENTRY_BLENDERLIB_NO_PREVIEW. */ - bool blenderlib_has_no_preview; + bool blenderlib_has_no_preview = false; /** Defined in BLI_fileops.h */ - eFileAttributes attributes; - BLI_stat_t st; + eFileAttributes attributes = eFileAttributes(0); + BLI_stat_t st = {0}; + + /** + * Be careful not to use the returned asset pointer in a context where it might be dangling, e.g. + * because the file list or the asset library were destroyed. + */ + asset_system::AssetRepresentation *get_asset() const + { + if (std::shared_ptr asset_ptr = asset.lock()) { + /* Returning a raw pointer from a shared pointer and destructing the shared pointer + * immediately afterwards isn't entirely clean. But it's just a way to get the raw pointer + * from the weak pointer. Nothing should free the asset in the asset library meanwhile, so + * this should be fine really. */ + BLI_assert(asset_ptr.use_count() > 1); + return asset_ptr.get(); + } + return nullptr; + } }; struct FileListIntern { @@ -572,15 +596,17 @@ static int compare_asset_catalog(void *user_data, const void *a1, const void *a2 const FileListInternEntry *entry1 = static_cast(a1); const FileListInternEntry *entry2 = static_cast(a2); const FileSortData *sort_data = static_cast(user_data); + const asset_system::AssetRepresentation *asset1 = entry1->get_asset(); + const asset_system::AssetRepresentation *asset2 = entry2->get_asset(); /* Order non-assets. */ - if (entry1->asset && !entry2->asset) { + if (asset1 && !asset2) { return 1; } - else if (!entry1->asset && entry2->asset) { + else if (!asset1 && asset2) { return -1; } - else if (!entry1->asset && !entry2->asset) { + else if (!asset1 && !asset2) { if (int order = compare_direntry_generic(entry1, entry2); order) { return compare_apply_inverted(order, sort_data); } @@ -588,13 +614,13 @@ static int compare_asset_catalog(void *user_data, const void *a1, const void *a2 return compare_apply_inverted(compare_tiebreaker(entry1, entry2), sort_data); } - const asset_system::AssetLibrary &asset_library1 = entry1->asset->owner_asset_library(); - const asset_system::AssetLibrary &asset_library2 = entry2->asset->owner_asset_library(); + const asset_system::AssetLibrary &asset_library1 = asset1->owner_asset_library(); + const asset_system::AssetLibrary &asset_library2 = asset2->owner_asset_library(); const asset_system::AssetCatalog *catalog1 = asset_library1.catalog_service().find_catalog( - entry1->asset->get_metadata().catalog_id); + asset1->get_metadata().catalog_id); const asset_system::AssetCatalog *catalog2 = asset_library2.catalog_service().find_catalog( - entry2->asset->get_metadata().catalog_id); + asset2->get_metadata().catalog_id); /* Order by catalog. Always keep assets without catalog last. */ int order = 0; @@ -806,10 +832,10 @@ static bool is_filtered_id_file_type(const FileListInternEntry *file, */ static AssetMetaData *filelist_file_internal_get_asset_data(const FileListInternEntry *file) { - if (!file->asset) { - return nullptr; + if (asset_system::AssetRepresentation *asset = file->get_asset()) { + return &asset->get_metadata(); } - return &file->asset->get_metadata(); + return nullptr; } static void prepare_filter_asset_library(const FileList *filelist, FileListFilter *filter) @@ -1456,11 +1482,9 @@ static void filelist_direntryarr_free(FileDirEntryArr *array) static void filelist_intern_entry_free(FileList *filelist, FileListInternEntry *entry) { - /* Asset system storage might be cleared already on file exit. Asset library - * pointers are dangling then, so don't access (#120466). */ - if (AS_asset_libraries_available() && entry->asset) { + if (auto asset_ptr = entry->asset.lock()) { BLI_assert(filelist->asset_library); - filelist->asset_library->remove_asset(*entry->asset); + filelist->asset_library->remove_asset(*asset_ptr); } if (entry->relpath) { @@ -1472,7 +1496,7 @@ static void filelist_intern_entry_free(FileList *filelist, FileListInternEntry * if (entry->name && entry->free_name) { MEM_freeN((char *)entry->name); } - MEM_freeN(entry); + MEM_delete(entry); } static void filelist_intern_free(FileList *filelist) @@ -1985,8 +2009,8 @@ int filelist_files_num_entries(FileList *filelist) static const char *fileentry_uiname(const char *root, FileListInternEntry *entry, char *buff) { - if (entry->asset) { - const StringRefNull asset_name = entry->asset->get_name(); + if (asset_system::AssetRepresentation *asset = entry->get_asset()) { + const StringRefNull asset_name = asset->get_name(); return BLI_strdupn(asset_name.c_str(), asset_name.size()); } @@ -2133,7 +2157,7 @@ static FileDirEntry *filelist_file_create_entry(FileList *filelist, const int in ret->redirection_path = BLI_strdup(entry->redirection_path); } ret->id = entry->local_data.id; - ret->asset = entry->asset; + ret->asset = entry->get_asset(); /* For some file types the preview is already available. */ if (entry->local_data.preview_image && BKE_previewimg_is_finished(entry->local_data.preview_image, ICON_SIZE_PREVIEW)) @@ -2270,7 +2294,7 @@ asset_system::AssetRepresentation *filelist_entry_get_asset_representation( const FileList *filelist, const int index) { const FileListInternEntry *intern_entry = filelist_entry_intern_get(filelist, index); - return intern_entry->asset; + return intern_entry->get_asset(); } ID *filelist_file_get_id(const FileDirEntry *file) @@ -3094,7 +3118,7 @@ static int filelist_readjob_list_dir(FileListReadJob *job_params, continue; } - entry = MEM_cnew(__func__); + entry = MEM_new(__func__); entry->relpath = current_relpath_append(job_params, files[i].relname); entry->st = files[i].s; @@ -3186,7 +3210,7 @@ ENUM_OPERATORS(ListLibOptions, LIST_LIB_ADD_PARENT); static FileListInternEntry *filelist_readjob_list_lib_group_create( const FileListReadJob *job_params, const int idcode, const char *group_name) { - FileListInternEntry *entry = MEM_cnew(__func__); + FileListInternEntry *entry = MEM_new(__func__); entry->relpath = current_relpath_append(job_params, group_name); entry->typeflag |= FILE_TYPE_BLENDERLIB | FILE_TYPE_DIR; entry->blentype = idcode; @@ -3204,7 +3228,7 @@ static void filelist_readjob_list_lib_add_datablock(FileListReadJob *job_params, const int idcode, const char *group_name) { - FileListInternEntry *entry = MEM_cnew(__func__); + FileListInternEntry *entry = MEM_new(__func__); if (prefix_relpath_with_group_name) { std::string datablock_path = StringRef(group_name) + SEP_STR + datablock_info->name; entry->relpath = current_relpath_append(job_params, datablock_path.c_str()); @@ -3229,7 +3253,7 @@ static void filelist_readjob_list_lib_add_datablock(FileListReadJob *job_params, datablock_info->asset_data = metadata.get(); datablock_info->free_asset_data = false; - entry->asset = &job_params->load_asset_library->add_external_asset( + entry->asset = job_params->load_asset_library->add_external_asset( entry->relpath, datablock_info->name, idcode, std::move(metadata)); } } @@ -3273,7 +3297,7 @@ static void filelist_readjob_list_lib_add_from_indexer_entries( static FileListInternEntry *filelist_readjob_list_lib_navigate_to_parent_entry_create( const FileListReadJob *job_params) { - FileListInternEntry *entry = MEM_cnew(__func__); + FileListInternEntry *entry = MEM_new(__func__); entry->relpath = current_relpath_append(job_params, FILENAME_PARENT); entry->typeflag |= (FILE_TYPE_BLENDERLIB | FILE_TYPE_DIR); return entry; @@ -3893,7 +3917,7 @@ static void filelist_readjob_main_assets_add_items(FileListReadJob *job_params, const char *id_code_name = BKE_idtype_idcode_to_name(GS(id_iter->name)); - entry = MEM_cnew(__func__); + entry = MEM_new(__func__); std::string datablock_path = StringRef(id_code_name) + SEP_STR + (id_iter->name + 2); entry->relpath = current_relpath_append(job_params, datablock_path.c_str()); entry->name = id_iter->name + 2; @@ -3905,7 +3929,7 @@ static void filelist_readjob_main_assets_add_items(FileListReadJob *job_params, id_iter); entry->local_data.id = id_iter; if (job_params->load_asset_library) { - entry->asset = &job_params->load_asset_library->add_local_id_asset(entry->relpath, *id_iter); + entry->asset = job_params->load_asset_library->add_local_id_asset(entry->relpath, *id_iter); } entries_num++; BLI_addtail(&tmp_entries, entry);