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
Partial fix for #130007.
The catalog tree mutex would be set in `catalog_tree()` and
`invalidate_catalog_tree()`. The former could end up adding catalogs and thus
calling `invalidate_catalog_tree()`, which would attempt to set a mutex that the
caller set already. This seems like a good use-case for `std::recursive_mutex`.
would make sure there's a catalog for each catalog path by calling
`create_missing_catalogs()`
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
When using clangd or running clang-tidy on headers there are
currently many errors. These are noisy in IDEs, make auto fixes
impossible, and break features like code completion, refactoring
and navigation.
This makes source/blender headers work by themselves, which is
generally the goal anyway. But #includes and forward declarations
were often incomplete.
* Add #includes and forward declarations
* Add IWYU pragma: export in a few places
* Remove some unused #includes (but there are many more)
* Tweak ShaderCreateInfo macros to work better with clangd
Some types of headers still have errors, these could be fixed or
worked around with more investigation. Mostly preprocessor
template headers like NOD_static_types.h.
Note that that disabling WITH_UNITY_BUILD is required for clangd to
work properly, otherwise compile_commands.json does not contain
the information for the relevant source files.
For more details see the developer docs:
https://developer.blender.org/docs/handbook/tooling/clangd/
Pull Request: https://projects.blender.org/blender/blender/pulls/132608
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.
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.
