Brush Assets: Operator to edit asset metadata #120081

Hans Goudey · 2024-03-29T21:18:42+01:00

Hans Goudey commented

2024-03-29 21:18:42 +01:00

Add an operator to edit the catalog, author, and description of brush
assets. This doesn't edit the preview image yet, I prefer to add that
separately.

Add an operator to edit the catalog, author, and description of brush assets. This doesn't edit the preview image yet, I prefer to add that separately. ![image](/attachments/50ab300d-f130-44be-9134-df7a3a15b233)

image.png

39 KiB

Hans Goudey added 1 commit 2024-03-29 21:18:52 +01:00

d39830f839 WIP: Brush Assets: Operator to edit asset metadata

Hans Goudey added 2 commits 2024-04-03 17:10:20 +02:00

80b33a99a9 Merge branch 'brush-assets-project' into brush-assets-metadata-edit-operator

80da848598 Finish

Hans Goudey changed title from ~~WIP: Brush Assets: Operator to edit asset metadata~~ to Brush Assets: Operator to edit asset metadata

2024-04-03 17:16:07 +02:00

Hans Goudey requested review from Julian Eisel 2024-04-03 17:16:14 +02:00

Hans Goudey requested review from Brecht Van Lommel 2024-04-03 17:16:20 +02:00

Nika Kutsniashvili commented

2024-04-03 17:30:03 +02:00

@HooglyBoogly how about license and copyright? They're part of the asset metadata in asset browser. Thumbnail too technically

Hans Goudey commented

2024-04-03 17:32:49 +02:00

Since brushes aren't included in renders or shipped in files, I'm not sure either of those are so important. They can always be edited in the files directly. I could add them of course, just not convinced it's worth the space. I mentioned thumbnails in the description-- I'll consider those separately.

Julian Eisel commented

2024-04-03 17:56:30 +02:00

I think just the catalog and the description should be enough. This is just for the user's own quick organization. Author might be useful as well, but I don't think it's necessary? No strong opinion.

Maybe the popup should have a label "Use the Asset Browser to access the full asset metadata"?

On that note, I wonder if the operator should be called "Quick Edit Metadata"?

I think just the catalog and the description should be enough. This is just for the user's own quick organization. Author might be useful as well, but I don't think it's necessary? No strong opinion. Maybe the popup should have a label "Use the Asset Browser to access the full asset metadata"? On that note, I wonder if the operator should be called "Quick Edit Metadata"?

Julian Eisel reviewed 2024-04-05 11:52:24 +02:00

source/blender/editors/sculpt_paint/paint_ops.cc Outdated

						
				@ -1088,0 +1119,4 @@

				  if (catalog_path[0]) {

				    const asset_system::AssetCatalog &catalog = asset_library_ensure_catalogs_in_path(

				        *library, catalog_path);

				    meta_data.catalog_id = catalog.catalog_id;

Julian Eisel commented

2024-04-03 17:48:49 +02:00

Use BKE_asset_metadata_catalog_id_set()?

Use `BKE_asset_metadata_catalog_id_set()`?

HooglyBoogly marked this conversation as resolved

Hans Goudey commented

2024-04-11 00:28:51 +02:00

Author might be useful as well, but I don't think it's necessary?

The main idea there was for the common use case of duplicating an essentials brush or a brush from someone else's library-- being able to correct the author earlier and easily sounds good.

Maybe the popup should have a label "Use the Asset Browser to access the full asset metadata"?

I like that idea, but I'd rather wait until we either:

Allow changing arbitrary asset meta-data in the asset browser, which currently isn't possible at all.
Allow saving the .asset.blend files without reverting them to regular .blend files. The current behavior is too strict IMO.

On that note, I wonder if the operator should be called "Quick Edit Metadata"?

Maybe, not sure though, for similar reasons as the last point.

>Author might be useful as well, but I don't think it's necessary? The main idea there was for the common use case of duplicating an essentials brush or a brush from someone else's library-- being able to correct the author earlier and easily sounds good. >Maybe the popup should have a label "Use the Asset Browser to access the full asset metadata"? I like that idea, but I'd rather wait until we either: 1. Allow changing arbitrary asset meta-data in the asset browser, which currently isn't possible at all. 2. Allow saving the `.asset.blend` files without reverting them to regular `.blend` files. The current behavior is too strict IMO. >On that note, I wonder if the operator should be called "Quick Edit Metadata"? Maybe, not sure though, for similar reasons as the last point.

Hans Goudey added 2 commits 2024-04-11 00:29:42 +02:00

742cfce068 Merge branch 'brush-assets-project' into brush-assets-metadata-edit-operator

e08df8fbe8 Use BKE_asset_metadata_catalog_id_set

Hans Goudey referenced this pull request

2024-04-11 17:40:27 +02:00

Brush Assets Tasks #116337

Hans Goudey added 2 commits 2024-04-11 18:12:26 +02:00

73a3936c98 Merge branch 'brush-assets-project' into brush-assets-metadata-edit-operator

2c63517239 Make catalog path fetching more forgiving

Julian Eisel reviewed 2024-04-16 17:46:55 +02:00

Julian Eisel left a comment

Some inline comments that are more asking for clarification than changes I guess. Sorry if I'm being a bit dumb here, I still need to build up more of a mental model for how we deal with brushes, the asset system and the whole separate main storage at runtime.
It does seem like bit of a pain point that we have to query so many different bits of data (local active brush ID, active brush weak reference, asset representation, asset library, asset library ref, ...) in so many different ways... Seems like this should be simpler, but guess it is what it is for now.

Some inline comments that are more asking for clarification than changes I guess. Sorry if I'm being a bit dumb here, I still need to build up more of a mental model for how we deal with brushes, the asset system and the whole separate main storage at runtime. It does seem like bit of a pain point that we have to query so many different bits of data (local active brush ID, active brush weak reference, asset representation, asset library, asset library ref, ...) in so many different ways... Seems like this should be simpler, but guess it is what it is for now.

source/blender/editors/sculpt_paint/paint_ops.cc

						
				@ -1085,0 +1097,4 @@

				  const Brush *brush = (paint) ? BKE_paint_brush_for_read(paint) : nullptr;

				  BLI_assert(ID_IS_ASSET(&brush->id));

				  const AssetWeakReference &brush_weak_ref = *paint->brush_asset_reference;

				  const asset_system::AssetRepresentation &asset = *asset::find_asset_from_weak_ref(

Julian Eisel commented

2024-04-15 19:17:23 +02:00

Usually failure should be expected and handled when looking up the asset from a weak reference. Since this is already done in the poll I guess we can skip that here, but would note that in a comment at least.

That said, an extra check here to handle the lookup failure would be trivial & safer (e.g. for when the operator is called while an asset library is being reloaded). So I'd actually prefer that.

Usually failure should be expected and handled when looking up the asset from a weak reference. Since this is already done in the poll I guess we can skip that here, but would note that in a comment at least. That said, an extra check here to handle the lookup failure would be trivial & safer (e.g. for when the operator is called while an asset library is being reloaded). So I'd actually prefer that.

HooglyBoogly marked this conversation as resolved

source/blender/editors/sculpt_paint/paint_ops.cc

						
				@ -1085,0 +1192,4 @@

				  if (paint == nullptr || brush == nullptr) {

				    return false;

				  }

				  if (!ID_IS_ASSET(&brush->id)) {

Julian Eisel commented

2024-04-16 17:40:57 +02:00

I find it confusing that both the active brush and the active brush asset reference are required. But I guess they are referring to / representing the same brush asset? And the local brush is in the separate main?

Julian Eisel commented

2024-04-16 17:51:17 +02:00

To add to that, if this is indeed the imported brush asset, I guess it relies on not clearing the asset data like we normally do. Maybe good to note somewhere that we're intentionally not doing that. E.g. in brush queries like BKE_paint_brush_for_read() and the import code.

To add to that, if this is indeed the imported brush asset, I guess it relies on not clearing the asset data like we normally do. Maybe good to note somewhere that we're intentionally not doing that. E.g. in brush queries like `BKE_paint_brush_for_read()` and the import code.

Hans Goudey commented

2024-04-17 16:47:31 +02:00

I find it confusing that both the active brush and the active brush asset reference are required. But I guess they are referring to / representing the same brush asset? And the local brush is in the separate main?

Yeah, exactly. I think we just use either one depending on what's more convenient.

To add to that, if this is indeed the imported brush asset, I guess it relies on not clearing the asset data like we normally do

Yes, the imported brushes are still marked as assets. That's not really related to this PR though.

>I find it confusing that both the active brush and the active brush asset reference are required. But I guess they are referring to / representing the same brush asset? And the local brush is in the separate main? Yeah, exactly. I think we just use either one depending on what's more convenient. >To add to that, if this is indeed the imported brush asset, I guess it relies on not clearing the asset data like we normally do Yes, the imported brushes are still marked as assets. That's not really related to this PR though.

source/blender/editors/sculpt_paint/paint_ops.cc

						
				@ -1085,0 +1204,4 @@

				  const asset_system::AssetRepresentation *asset = asset::find_asset_from_weak_ref(

				      *C, *brush_weak_ref, nullptr);

				  if (!asset) {

				    BLI_assert_unreachable();

Julian Eisel commented

2024-04-15 19:12:18 +02:00

I think failure is valid here? The asset might not be available anymore, for various reasons, but the weak reference may still be set. I'd hope that the name "weak" reference makes this clear enough :)

HooglyBoogly marked this conversation as resolved

Hans Goudey added 3 commits 2024-04-17 16:48:22 +02:00

9f3f58156d Merge branch 'brush-assets-project' into brush-assets-metadata-edit-operator

d019c304a2 Always null check result of asset::find_asset_from_weak_ref

9559584268 Remove assert for missing asset

Julian Eisel approved these changes 2024-04-17 17:12:07 +02:00

Hans Goudey merged commit b8bc1c56f7 into brush-assets-project

2024-04-17 17:29:00 +02:00

Hans Goudey referenced this issue from a commit

2024-04-17 17:29:01 +02:00

Brush Assets: Operator to edit asset metadata

Hans Goudey deleted branch brush-assets-metadata-edit-operator