Convert all slashes to native format when initializing an asset library. This
might convert slashes that are valid parts of the file name, but this just
leads to an error about a not found asset library, which is better than
crashing. This is a typical tradeoff when dealing with cross platform paths.
When saving a file, move the catalog service from the runtime asset library into the new on-disk
asset library. This makes all catalog data like the undo history and deleted catalogs be preserved.
The fact that a new asset library type gets allocated is an implementation detail that shouldn't
affect behavior.
Once the service is moved, an undo push is added (so this state can be restored to), and if the new
.blend file location can be associated with a catalog definition file, its catalogs are merged in.
Includes unit tests.
Pull Request: https://projects.blender.org/blender/blender/pulls/135589
Rather new code to build the asset library reference from a library
didn't cover the case where the current file library is saved to disk,
and as such implemented as on-disk library.
Second part to fix#130007, after 50f7666785.
When saving a new file with the current file asset library loaded, the
library wasn't converted properly to an on-disk library. While the
library was loaded again as on-disk library, the previous runtime
version of it wasn't cleared, so catalog definitions could be duplicated
across available libraries.
Make sure the runtime only library is destructed properly, and the UI
refreshed.
There seem to be more issues with converting the runtime current file library to
a on-disk current file library, but these can be addressed after the crashes and
hangs are fixed, see report.
Pull Request: https://projects.blender.org/blender/blender/pulls/133341
Brush and pose asset operators were doing some avoidable roundtrips
through asset types, lookups and rather low level operations. This
indicates that the asset system APIs need some improvements. Moving
lower-level logic there can help avoiding errors, since implementation
details are kept inside the corresponding module.
- Avoid lookups for asset library reference in operator code, make asset
library itself construct it.
- Avoid redundant lookups for asset library definition (was looking up
the asset library definition in the Preferences from the library
reference type, just to turn it into the reference type again).
- Add utility for refreshing loading asset libraries
- Add utility for saving asset catalogs for an asset
- Remove unused function
- Fixes preview flickering on actions like undo/redo in the asset shelf (#93726), not yet for the
file browser.
- Fixes#130861.
Makes the asset shelf use the asynchronous preview loading system of the UI instead of the file
browser one. The issues above where mostly caused by the file browser caching design.
The asset system and its UIs can now manage previews independently of the file browser back-end.
This is another step towards making the asset system independent of the file browser, see
https://developer.blender.org/docs/features/asset_system/fundamentals/from_file_browser_to_asset_system/.
Code to query asset previews through file browser types is removed.
Quite some work was done to prepare the UI preview system for this, to make it on par with the file
browser preview system. E.g.: 9d83061ed4, 315e7e04a8, 5055adc1c0, 16ab6111f7.
Note that the same change should be done to the asset/file browser, but this requires more work.
Pull Request: https://projects.blender.org/blender/blender/pulls/131871
I found the naming of these files awkward, where we prefixed them with
`asset_library_` to keep them grouped together, and then a library type
name. This resulted in rather un-natural names. Also, there are files
like `asset_library_service.cc`, which is in fact not another asset
library type, but seems like one with the old convention.
Moving these files to an own directory keeps the grouping while allowing
more natural sounding names.
For C/C++ doc-strings should be located in headers,
move function comments into the headers, in some cases merging
with existing doc-strings, in other cases, moving implementation
notes into the function body.
Fixes#128751.
As asset libraries were added or removed (through the UI or BPY), the
"All" asset library wouldn't refresh to reflect the changes. In general
this wasn't handled well. Even a manual refresh wouldn't give the right
result.
There were multiple issues really:
- Only the first load of the "All" library would query the preferences
for the available libraries. Further loads would only refresh the
catalogs, ignoring any added/removed libraries.
- Operators and BPY functions didn't clear the asset libraries to
enforce a re-fetch.
- When clearing an asset library, the "All" library wasn't cleared in
some cases, it would show the old state still.
- The API function to clear an asset library's asset list would not
clear the storage of asset browsers so they wouldn't refresh. It makes
no sense to only do one, so let the API handle both cases.
The way we handle asset library updating could be improved generally,
and be more internal to the asset system. For now this explicit clearing
seems fine.
We also need to handle removal of libraries better still, I think they
remain in memory.
Pull Request: https://projects.blender.org/blender/blender/pulls/129541
Design of the essentials asset library is to treat it as part of
Blender (as if it were compiled into the binary), so the location and
state of these assets is clear and can be assumed in code. User edits
are not expected.
Because of that we should only look for this asset library in the
installation ("system") location, not in the user configuration which
would take priority if present. On some devs machines this location
would actually be present, making the essentials unavailable and causing
confusing/misleading warning prints, see #128420.
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.
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).
