Details

Reviewers

Summary

Simple implementation of an AssetCatalogService that's responsible
for managing asset catalogs and loading them from asset *catalog definition
files* (CDFs).

The following classes are implemented in this patch:

struct AssetLibrary: an opaque struct that C code can just pass around; it is actually defined in C++. It is created with BKE_asset_library_load(library_path) and freed with BKE_asset_library_free(the_pointer). It has an owning pointer to a AssetCatalogService, which is also created from the BKE_asset_library_load() call.
AssetCatalogService: a C++ class that is responsible for managing asset catalogs of a single asset library. A future version will be able to load multiple catalog definition files and join the catalogs defined therein; this version only loads a single file, single_catalog_definition_file.cats.txt, from the top directory of the asset library.
AssetCatalogDefinitionFile: representation of the file the AssetCatalogs were loaded from. This is mostly important for supporting multiple CDFs; when their catalogs change, it should be known which files they came from.
AssetCatalog: contains the catalog ID and the full catalog path.

Features

The following features are implemented here:

Added field char AssetMetaData::catalog_id[MAX_NAME], to hold the catalog ID of the asset. This field should be set using the BKE_asset_metadata_catalog_id_set() function, which ensures that there are no spaces in the ID and that the field is not too long.
A little UI extension to show the active asset's catalog ID in the asset browser.
Loading a single CDF
Parsing the CDF into AssetCatalog structs
The AssetCatalogService::find_catalog(catalog_id) method, which returns the AssetCatalog identified by the given ID.
Creating an AssetCatalogDefinitionFile, which can also save the catalogs back to disk (in arbitrary order)
Creating & storing an AssetLibrary in the FileList struct, to be able to find & parse the CDFs in a background thread.

Future work

After this patch is accepted, the following work should be done:

Change from std::filesystem to another library, depending on the outcome of D12117: Initial work for std::filesystem transition
Provide a hierarchical view on catalogs, such that a tree can be drawn in the UI
Provide an API for renaming catalogs, both their ID and their path.
Save catalogs to the catalog definition file, ordered by their path. This'll make the order in which they are saved predictable & stable, playing nice with versioning systems.

Diff Detail

Repository

rB Blender

Branch

temp-asset-browser-catalogs

Build Status

Buildable 16294
Build 16294: arc lint + arc unit

Event Timeline

Sybren A. Stüvel (sybren) requested review of this revision.Aug 9 2021, 3:08 PM

Sybren A. Stüvel (sybren) created this revision.

Harbormaster completed remote builds in B16294: Diff 40474.Aug 9 2021, 3:08 PM

Cleanup: C++ to C comment style

Harbormaster completed remote builds in B16295: Diff 40475.Aug 9 2021, 3:09 PM

Sybren A. Stüvel (sybren) planned changes to this revision.Aug 9 2021, 3:12 PM

Andres Stephens (Draise) awarded a token.Aug 9 2021, 3:14 PM

Fix pointer ownership issue in AssetCatalogService::parse_catalog_file(), and solve some feature envy by adding a few AssetCatalogDefinitionFile methods.

Harbormaster completed remote builds in B16296: Diff 40477.Aug 9 2021, 3:29 PM

Cleanup: Assets, move AssetLibrary struct to global namespace

Harbormaster completed remote builds in B16299: Diff 40481.Aug 9 2021, 5:02 PM

Generally this makes sense. I would have to properly test it to make sure the asserts in the file-list code are correct.

source/blender/blenkernel/BKE_asset_catalog.hh
49–50	I'd suggest to default to Doxygen comments for all definitions.
53	People may expect that on Windows they can use `\`. Maybe explicitly note that this isn't supported.
58	Style guide is to use a trailing `_` for private/protected vars. https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Class_data_member_names
source/blender/blenkernel/intern/asset_library.hh
36	I wouldn't put namespace aliases in headers since that propagates to source files including this. Unless of course that's an intentional part of the API design (like the `blender::bli::filesystem` alias I proposed). Is this just a WIP thing?

Sybren A. Stüvel (sybren) added inline comments.Aug 9 2021, 5:17 PM

source/blender/blenkernel/BKE_asset_catalog.hh
53	We could simply replace those with `/` as soon as they're entered. Something similar to the cleanup in the `BKE_asset_metadata_catalog_id_set()` function.
58	I know, hence the `TODO(@sybren): determine which properties should be private / get accessors.` note ;-) Now that things are at the "ready for initial review" stage, I'll just mark everything as private suffix with `_`. If it's not proven to be necessarily public, I'd rather have them private.