Match function and declaration names, picking names based on
consistency with related code & clarity.
Also changes for old conventions, missed in previous cleanups:
- name -> filepath
- tname -> newname
- maxlen -> maxncpy
Rather than a manually managed union, use `std::variant` which is
generally safer. E.g. an invalid access will now throw an exception
instead of causing undefined behavior (which may or may not crash, or
cause a data corruption). Code is also simplified this way.
Pull Request: https://projects.blender.org/blender/blender/pulls/125494
Use "asset storage" instead of just "storage", because there are other
members acting as storage (e.g. holding all catalog data), while this is
asset specific.
This was just rather useless level of abstraction. I heard from other
devs that these helper classes caused confusion, so better to avoid
this.
Now the asset representation has all the needed bits to create its full
path, blend-file path and asset library relative path. In fact only the
asset library relative path needs to be stored to make all this
available, since the asset representation already stores a reference to
the asset library owning it, so the paths can be recreated easily.
This was just a useless level of abstraction, where the asset library
would have functions with the same name, just to pass the call on to the
asset storage. Now asset library stores and manages the asset
representations itself directly.
This should simplify the asset system a bit. I heard from other devs
that these kind of helper classes caused confusion for them.
Switching to an asset browser, then back to a different editor would
cause a crash when loading a new file.
Basically the file/asset browser tried to free dangling asset and asset
library pointers when trying to free itself. That's because the asset
system gets destructed with a `AS_asset_libraries_exit()` call before
the screen and with that the asset browser were freed.
Pull Request: https://projects.blender.org/blender/blender/pulls/125084
Two issues:
- The `AS_asset_libraries_available()` function would always return
true, because `AssetLibraryService::get()` would actually allocate the
service if not available.
- The issue would still happen if another code path would call
`AssetLibraryService::get()` after the service was destroyed, and
before the UI data was destroyed. This was happening now, so the crash
was back, see #124167.
I'm working on a new fix, for now, remove this broken one.
Issue wasn't directly related to material assets or the shader editor.
Simpler steps to reproduce:
- Open Asset Browser
- Change Asset Browser to different editor type
- Open new file (Ctrl+N)
The asset browser would remain in storage as inactive editor, including
pointers to the asset system. When opening a new file, the asset system
would get freed before the asset browser, which would then access
dangling pointers as part of its own freeing process.
Part of the issue is that `SpaceType.exit()` doesn't get called in this
case, which would remove the asset system references before the asset
system is freed. Will address this in a follow up in main, but best to
not depend on the `exit()` callback too much. Easy to do here.
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.
