Pass strings by value and move their result. This gives the caller
the potential to move existing strings into the class. Moving the
std::shared_ptr should just avoid reference counting here.
These containers (Set, Vector, Map, Span), etc. have default constructors,
making the braces unnecessary for default initialization. Better to depend
on that consistently rather than having braces in some places and not others.
There's no need for the catalog ID to change ever with the current
catalog system design. If that changes, a new catalog can probably be
created instead. Avoiding modifications also helps avoiding data races.
Also means the currently unused default constructor becomes unavailable.
- Pass null instead of an empty string to BKE_tempdir_init
because the string isn't meant to be used.
- Never pass null to BLI_temp_directory_path_copy_if_valid
(the caller must check).
- Additional comments for which checks are performed & why
from discussion about #95411.
Note that this still isn't entirely thread safe since the catalogs of
the asset libraries may still be edited in various ways while building
the all asset library. But at least this avoids a data race when
assigning the catalog service once done building it.
Some of these members are not expected to change throughout the lifetime
of the class instances, so make them constant and only set them via the
constructor. Making them immutable this way helps making clear which
data needs extra attention for thread safety.
These shouldn't need to be accessed from outside the asset system
itself, so they should not be exposed with its API. Move them to an own
asset system private headers. This is a further step towards making
asset system classes more encapuslated, so behavior and data flow can be
controlled better (which helps addressing threat safety issues).
Personally I've found it quite confusing to work on higher level issues
of the asset catalog system, because I got lost in the multiple classes.
Hopefully separating them more clearly helps with that too.
Making the member private (or at least protected) makes threat safety
more tangible, and we don't need to expose locking in the API. Generally
we need to make data more encapsulated, so we can make edits more
controlled and threat safe.
Asset libraries also always have a catalog-service, so it can accessed
by reference, rather than pointer that would have to be null-checked.
These expose internal data for unit testing purposes. While it might be
better to avoid this entirely, at least make the returned data const, so
there's no unexpected modification from outside the catalog service
internals.
Making this thread safe is quite trivial now. Note that for building the
tree we iterate the catalogs map, which may still be modified from
another thread in parallel. Making this thread safe is kept for a
separate commit.
Unit tests were assuming that creating a catalog from a path would not
create catalogs for the parent path elements if missing. I'd argue this
should not be unit tested since it's internal behavior that isn't
visible to API users. But for now I'll keep the test working as is, also
to avoid indirect recursive calls of `create_missing_catalogs()`.
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.
Basic motivation is that `AssetCatalogService::get_catalog_tree()`
should return a const tree, since this tree is internal state and
shouldn't be modified from outside. This exposed a whole bunch of const
incorrectnesses and just generally allows to make much more of the API
const (as it should be).
Also use references instead of pointers in testing functions, where null
is not an expected value.
`refresh_catalogs()` for the "All" asset library effectively does the
same as iterating over all other asset libraries and calling
`get_asset_library()` on them. So doing both just performs the same work
twice.
Mistake in #118463.
Updating the catalogs of the "All" asset library would also reload
catalog data of the other asset libraries from disk. This wasn't
intended, this should be done with an explicit load request only (and on
a thread to not block the main thread).
PR #118382 exposed an issue where creating a catalog with non-existing
parents would trigger an endless recursion. When creating the missing
parents, the tree would be rebuilt, which would again try to create the
missing parents, for which the tree would be rebuild, etc. It just
happend to work fine for creating single catalogs with existing parents.
Rather than rebuilding the tree immediately, tag the asset library as
having "dirty" catalog data, and let code requiring the up-to-date
catalog tree (node tool menus) ensure the tree is updated if needed.
The asset library loading API should be made clearer and generally
better defined/designed, currently it's all a bit messy & confusing.
Pull Request: https://projects.blender.org/blender/blender/pulls/118463
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).
Multiple threads may try to load the "All" asset library catalogs, so
avoid working on the same catalog service in parallel.
Found when testing #118463 on top of #118382, by creating a sculpt brush
asset while the asset shelf is open. The 3D View header would trigger
reloading of the "All" asset library catalogs to display node assets in
header pulldowns, while the asset shelf triggered a threaded reload of
the asset library.
This is generally more flexible and less error prone. The struct
implements a proper descructorfor this anyway. That also makes the
separate free function unnecessary-- it's redundant with the destructor.
It should not be possible to end up with an asset representation that
does not have a valid pointer/reference to the asset library owning it,
it's injected via all constructors. So reflect that by using a reference
instead of a (possibly null) pointer.
The ID remapper code was already largely defined in a CPP struct
(IDRemapper). Make this an actual class, and remove the C API wrapper
around.
This makes the code cleaner, easier to follow, and easier to extend or
modify in the future.
Pull Request: https://projects.blender.org/blender/blender/pulls/118146
Continuation of bdfb1f6a04. Adds derived classes for the essentials and
preferences asset libraries, to allow specialized classes with a simple
and common interface. This should help untangling the asset library
service code.
Initial changes to make the asset library class polymorphic, plus using
that to cleanup some basic loading logic. I'm confident that this will
help us improve the asset library management code quite a bit, currently
it's quite confusing. Plus it will be extendable this way in future, so
new kinds of asset libraries (e.g. Blender project asset libraries or
arbitrary online asset libraries) can be added much easier.
