Assets: Implement Traits
#105841
Open
Harley Acheson
wants to merge 1 commits from Harley/blender:AssetTraits
into main
pull from: Harley/blender:AssetTraits
merge into: blender:main
blender:main
blender:blender-v3.6-release
blender:temp-sculpt-dyntopo-hive-alloc
blender:temp-sculpt-dyntopo
blender:asset-shelf
blender:cycles-light-linking
blender:tmp-usd-python-mtl
blender:brush-assets-project
blender:blender-v2.93-release
blender:blender-v3.3-release
blender:universal-scene-description
blender:node-group-operators
blender:asset-browser-frontend-split
blender:temp-sculpt-attr-api
blender:blender-v3.5-release
blender:realtime-clock
blender:sculpt-dev
blender:gpencil-next
blender:bevelv2
blender:microfacet_hair
blender:blender-projects-basics
blender:principled-v2
When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Reviewers
Request review
No reviewers
Labels
Clear labels
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports
Apply labels
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
Eevee & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest/Import
Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest: Wayland
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Eevee & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
Eevee & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest/Import
Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest: Wayland
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Eevee & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Set Project
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#105841
Reference in New Issue
There is no content yet.
Delete Branch "Harley/blender:AssetTraits"
Deleting a branch is permanent. It CANNOT be undone. Continue?
Allow Assets to have system-defined keywords to provide information
about the type of the asset.
This is WIP implementation for task #105808 for design #105807
This is currently just a copy of the code that implements tags. So this includes things we don't need like user-facing addition and deletion of traits, but I needed to make sure that these operations worked and were saved correctly. I imagine these being removed after this is confirmed correct. This doesn't change
ED_asset_filter_matches_asset
because I was unable to find that visible to test.I think we can remove most of the editing capabilities. Let's for now only have a list or traits that is managed internally, with no API functions to edit them. I guess we'd want that later to support custom asset types managed by add-ons, but we may want to use different ways to define those (e.g. a
bl_asset_traits
property for a new asset type definition).Plus, we indeed don't need any UI for the traits. We might want to display the traits as non-editable tags in the tags list, for informal purposes.
@ -104,0 +115,4 @@
return trait;
}
AssetTrait *BKE_asset_metadata_trait_add(AssetMetaData *asset_data, const char *name)
Can be removed. I'd for traits we just want to ensure a specific trait is set, we never want the name to be changed (like no automatic
.001
suffix). So_ensure()
is all we need.@ -27,0 +30,4 @@
*/
typedef struct AssetTrait {
struct AssetTrait *next, *prev;
char name[64];
Add
MAX_NAME
comment.@ -91,0 +103,4 @@
ListBase traits; /* AssetTrait */
short active_trait;
/** Number of traits to avoid counting. Can be moved with tot_tags to runtime data. */
short tot_traits;
Don't think we need this. I'd like to have a runtime only has of the asset traits in
AssetRepresentation
. If we ever need the count of traits we can get the size from that.@ -440,0 +534,4 @@
RNA_def_struct_name_property(srna, prop);
}
static void rna_def_asset_traits_api(BlenderRNA *brna, PropertyRNA *cprop)
Can be removed entirely I guess.
@ -507,0 +641,4 @@
RNA_def_property_struct_type(prop, "AssetTrait");
RNA_def_property_editable_func(prop, "rna_AssetMetaData_editable");
RNA_def_property_ui_text(prop,
"Traits",
I think we should consistently use the term "asset traits", not just "traits".
@ -507,0 +646,4 @@
"general asset management");
rna_def_asset_traits_api(brna, prop);
prop = RNA_def_property(srna, "active_trait", PROP_INT, PROP_NONE);
Shouldn't be needed since we don't allow changing traits in the UI.
427fb6e203
todd518f4313
@JulianEisel - Sorry for the delay; took a while to get rid of all my extras without leaving any cruft.
dd518f4313
to91456a87fa
@JulianEisel @Harley what is the status of this patch?
Could you remove the
ensure_traits_fn
callbacks for now as noted in #105808 (comment)? We don't need most of these traits yet, I rather add traits as needed, and don't set a whole bunch of unused traits just because they might be useful in future. (Although to a degree we may have to do that for forwards compatibility.)Also, if a patch is ready for review, please make sure to remove the "WIP:" from the title. This is meant for patches that you still intend to work on independently of the review process.
@ -1044,0 +1045,4 @@
{
bNodeTree &node_tree = *static_cast<bNodeTree *>(asset_ptr);
BLI_assert(GS(node_tree.id.name) == ID_NT);
BKE_asset_metadata_trait_ensure(asset_data, node_tree.typeinfo->ui_description);
Why would we store the UI description as trait? But again, I wouldn't even define the
ensure_traits_fn
anyway, and just let the nodes module implement the traits they need.@ -1223,0 +1225,4 @@
{
Object *ob = (Object *)asset_ptr;
BLI_assert(GS(ob->id.name) == ID_OB);
BKE_asset_metadata_trait_ensure(asset_data, get_obdata_defname(ob->type));
There are two problems with
get_obdata_defname()
:I guess for now the best option is to have a bunch of
#define
s inDNA_object_types.h
(with a comment noting these may be written to files!), use this inrna_enum_object_type_items()
(to avoid duplication, this stuff easily gets out of sync), and add something likeget_obdata_ui_name()
.We could in theory use
RNA_enum_name()
, but that would mean that changing UI names inrna_enum_object_type_items()
would break compatibility. This isn't the case usually and I rather avoid this, with the#define
s in DNA headers it's more clear I think.@ -41,1 +41,4 @@
BKE_asset_metadata_trait_ensure(id->asset_data, "ID");
BKE_asset_metadata_trait_ensure(id->asset_data, id_type_info->name);
BKE_asset_metadata_trait_ensure(id->asset_data, id->name + 2);
The name as trait doesn't make sense really, this is not type information.
@ -25,2 +25,4 @@
} AssetTag;
/**
* \brief System-defined trait.
What does System-defined mean? The OS? Would put it something like: "Keywords describing type characteristics of an asset".
@ -26,1 +26,4 @@
/**
* \brief System-defined trait.
* Defined separately from AssetTag because we might want to vary name length.
Would remove this, it's just a different kind of entity/concept.
@ -91,0 +101,4 @@
/** System defined keywords for describing the type of asset. */
ListBase traits; /* AssetTrait */
short active_trait;
Not needed since the traits can't be edited right now. Tags only need this for the UI.
@ -136,0 +141,4 @@
return BLI_sprintfN("asset_data.traits[\"%s\"]", asset_trait_name_esc);
}
static int rna_AssetTrait_editable(PointerRNA *ptr, const char **r_info)
Shouldn't be needed since traits are never editable.
@ -507,0 +550,4 @@
RNA_def_property_ui_text(prop,
"Asset Traits",
"Custom traits (name tokens) for the asset, used for filtering and "
"general asset management");
Suggest: "Keywords describing type characteristics of an asset. Used to determine how an asset should behave, and to provide additional information and filtering options to the user"
@ -89,2 +99,4 @@
char _pad[4];
/** System defined keywords for describing the type of asset. */
Again System defined is a bit vague. Would just add a
/** See #AssetTrait. */
comment here.91456a87fa
to352db5ccca
WIP: Assets: Implement Traitsto Assets: Implement TraitsI removed that section so didn't really look at it again, but I think that when I was examining the data there the names had multiple words CamelCased together but the descriptions were nicer. But again, that code is gone now anyway.
I changed it all to your requested terminology, but I do think that "system-defined" was the best counterpoint to "user-defined" as it helped explain the similarities and differences. Now we have "User defined tag" and a very similar looking thing as "Keywords describing type characteristics of an asset." Don't think anyone would think system-defined had anything to do with the OS or the platform - just implies not defined or managed by the user.
@ -42,1 +48,4 @@
/** Tags and Traits to match against. These are newly allocated, and compared against the
* #AssetMetaData.tags and #AssetMetaData.traits. */
ListBase tags; /* AssetTag */
ListBase traits; /* AssetTrait */
Either remove this or actually implement the filtering in
ED_asset_filter_matches_asset()
.@ -2144,6 +2152,47 @@ static const char *get_obdata_defname(int type)
}
}
static const char *get_obdata_uiname(int type)
Note that this string isn't translated in a comment.
@ -1223,3 +1231,3 @@
static AssetTypeInfo AssetType_OB = {
/*pre_save_fn*/ object_asset_pre_save,
};
/*EnsureTraitsFn*/ object_asset_ensure_traits};
/*ensure_traits_fn*/ object_asset_ensure_traits,
The trailing comma ensures the closing
}
is put on the next line, which I find more visually pleasing and we usually do this.@ -35,2 +35,4 @@
} AssetTag;
/**
* \brief Keywords describing type characteristics of an asset. Not user-defined.
Would say: "Managed by Blender, not the user".
Also note that the trait should never be translated for storage (only when displaying in the UI).
I think this is almost ready, just some smaller points.
@ -37,0 +39,4 @@
*/
typedef struct AssetTrait {
struct AssetTrait *next, *prev;
char name[64]; /* MAX_NAME */
Think calling this
value
makes more sense. Same for the tags actually, but yeah, not worth bothering.@ -46,1 +46,4 @@
/** UI Names for object types. These may be written to files,
* so changing these should be done with extreme caution. */
#define OB_UINAME_MESH "Mesh"
Suggest
OB_UI_NAME_MESH
.@ -440,0 +452,4 @@
srna = RNA_def_struct(brna, "AssetTrait", NULL);
RNA_def_struct_path_func(srna, "rna_AssetTrait_path");
RNA_def_struct_ui_text(srna, "Asset Trait", "System defined trait (name token)");
Use the same or a similar description as elsewhere. Here this is especially important, since this will be the description for the Python documentation of the type.
Could the
get_obdata_uiname
stuff be replaced with use of theIDTypeInfo
ofob->data
? It's a bit of a shame to add another set of defines and another switch with Blender trying to move away from that sort of thing and become more easily extensible.Also, I know it's simple to use
ListBase
in DNA, but I wonder if it's worth using an array here instead. That makes it a bit more complex to add and remove items, but it makes everything else much simpler, since there would actually be type safety, etc. For example, the traits could mostly be passed asSpan<AssetTrait>
. Thenext
andprev
pointers could also be removed. That's my personal agenda of moving away from linked lists coming though, but maybe you find it reasonable too.Oops, meant to just comment, now I can't remove myself as a reviewer though :/
We could have a
const char *ui_name
in there and assign that to theID_UI_NAME_
value indeed. Similar toid_code
,id_filter
andmain_listbase_index
.Unfortunately the
#define
s would still be necessary if we want to keep the UI names in the RNA enum in sync and static.For runtime there should be some kind of hash-table inside
AssetRepresentation
(and/or maybe inside anAssetMetaData_Runtime
?). The listbase is more or less just for DNA storage. Don't think it's a big issue, not worth the complexity IMO.I'd argue using
ListBase
everywhere gives its own pervasive complexity that's less obvious when adding it in the first place. But fair enough. I guess the need to store these at all is just a future-proofing thing?@ -43,0 +46,4 @@
if (id_type_info->asset_type_info && id_type_info->asset_type_info->ensure_traits_fn) {
id_type_info->asset_type_info->ensure_traits_fn(id, id->asset_data);
}
Bastien is quite against doing this on asset marking only, so let's do it on file save in
ED_assets_pre_save()
.They are stored in the asset metadata so this type information becomes available when loading asset representations, without loading the entire asset. Or do you mean the object data traits? Yeah we don't actually use them yet, so this is a future proofing thing indeed.
Talked to Bastien, he rejects the idea of something like a
ui_name
field inIDTypeInfo
for this purpose, because then the object callback would have to get theIDTypeInfo
of the object data, which creates dependencies between ID types.So nothing to change there from the current patch, we stick to the
#define
s and the switch-case for the object data.@JulianEisel
Should be done with just the following notes:
I couldn't tell from your comment if you wanted this done for traits, tags, both, or none. So I left it alone. Can certainly do whatever you prefer.
I did so. But that does mean that assets loaded from a prior session have these while newly-created ones will not until saved. Maybe not a problem at all, and probably just stating the obvious.
Almost ready!
@ -2147,0 +2156,4 @@
/* Return a string identifying the object type that is *not* translated. */
static const char *get_obdata_uiname(int type)
{
switch (type) {
Cast this to the enum type, so that the compiler warns when a new enumerator is introduced but not added here.
@ -37,0 +40,4 @@
*/
typedef struct AssetTrait {
struct AssetTrait *next, *prev;
char name[64]; /* MAX_NAME */
Just rename it to
value
, makes more sense to me.Done. But I also changed other related phrasing around it, so in comments, argument names, etc for consistancy.
I did and I was surprised that it worked. Our Object types enum was untyped so I assumed if I added one (here using
ObjectType
) that it would result in a billion warnings/errors all over the place. But seems to be fine with it. Cool.Committed
fe7540d39a
to add anObjectType
typedef, shouldn't cause any merge conflicts. We should really have that, there are plenty ofswitch
-cases
that should cover all enumerators.@JulianEisel - This PR seems to be done? I don't think your last comment (about ObjectType) requested any changes here, but correct me if I am misunderstanding.
Minor points, this is pretty much ready.
@ -1223,2 +1230,4 @@
static AssetTypeInfo AssetType_OB = {
/*pre_save_fn*/ object_asset_pre_save,
/*EnsureTraitsFn*/ object_asset_ensure_traits,
Use the callback name, not type for the comment, like we do elsewhere (
ensure_traits_fn
).@ -2147,0 +2156,4 @@
/* Return a string identifying the object type that is *not* translated. */
static const char *get_obdata_uiname(int type)
{
switch ((ObjectType)type) {
For C++ functional cast:
ObjectType(type)
(see style guide).@ -101,0 +110,4 @@
/** See #AssetTrait. */
ListBase traits;
Trailing empty line, what an eyesore :)
@ -440,0 +452,4 @@
srna = RNA_def_struct(brna, "AssetTrait", NULL);
RNA_def_struct_path_func(srna, "rna_AssetTrait_path");
RNA_def_struct_ui_text(srna, "Asset Trait", "Asset keywords managed by Blender (not user defined)");
"Asset keywords"? Would stick to the definition as below, e.g. "Keywords describing type characteristics of an asset (managed by Blender, not the user)".
1909c7a0e5
to730e0645fb
@blender-bot build
I would like to have some sign off by @mont29 and/or @brecht on the idea of storing UI names as asset traits. This patch stores the
IDTypeInfo.name
andOB_UI_NAME_XXX
as asset traits, so they become relevant for compatibility.@ -48,0 +52,4 @@
#define OB_UI_NAME_SURF "Surface"
#define OB_UI_NAME_METABALL "Metaball"
#define OB_UI_NAME_FONT "Text"
#define OB_UI_NAME_CURVES "Hair Curves"
OB_UI_NAME_CURVE
andOB_UI_NAME_CURVES
are a bit confusing. Should these really be saved to traits as "Curve" and "Hair Curves"? @HooglyBoogly @mont29?I think for
rna_enum_object_type_items()
we can keep using the name "Hair Curve", but for the macro they should probably both read "Curve".I had not seen the design of asset traits before, and I left questions in #105807.
It's not clear to me what it means exactly to have e.g. a "Mesh" trait on an asset. Does it guarantee there exist some interface to query it as a Mesh datablock? Though when used together with an "Object" trait I guess it means there you can get an Object with type mesh instead? But maybe that object has geometry nodes generating curves from the mesh? Is a distinction needed between an asset being a datablock, or being available to be retrieved as a databock?
To me it seems there can be quite a bit of nuance here, where you may need a tooltip to describe it also. And at that point maybe you might as well use an identifier, and then map that to some UI name and description. Which can then also be translated or updated over time.
I'd suggest to clearly define the meaning of traits and have an understanding of how UI and tools are intended to use them, before creating them and storing them in a way that requires us to think about compatibility. Maybe for the first version of traits it's better to only add those that have a specific meaning, like a "use this node group as a mesh modifier" trait.
Reviewers