Assets: Implement Traits #105841
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
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
Overlay
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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 Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#105841
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "Harley/blender:AssetTraits"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Allow Assets to have system-defined keywords to provide information
about the type of the asset.
Implementation of task #105808 for design #105807. This is mostly a copy of the code that implements tags, but without any user-facing addition and deletion of traits.
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.
Hans Goudey referenced this pull request2023-07-06 16:48:34 +02:00
With #109526 we have a more solid understanding of how these traits will be used. Each trait in this popover has controls which object types and modes a node group is usable as an operator for.
![image](/blender/blender/attachments/51d2f0ab-694c-4bdb-bf6f-f760f5cf4bea)
I think it makes sense to remove the object type name traits from this PR, since it's not clear how they will be used, and it will simplify this change.
730e0645fb
to83d76c28f0
From the examples of traits that I could see, they are exposing some RNA property of the datablock on the asset. Either an existing property like the brush mode, or one specifically created for this purpose like the node group asset settings. Even for more complex traits you could imagine defining a read-only RNA property specifically for that purpose.
The traits system doesn't have to be limited to that especially if there is an intent to integrate with OpenAssetIO traits. But for Blender native traits, if it's all RNA properties you immediately get an unambiguous meaning, unique identifier, UI name and description. It would be like marking some boolean and enum properties to be indexed more than a new type of data.
So for example traits could be:
@ -136,1 +137,4 @@
static AssetTrait *asset_metadata_trait_add(AssetMetaData *asset_data, const char *const value)
{
AssetTrait *trait = (AssetTrait *)MEM_callocN(sizeof(*trait), __func__);
MEM_cnew<AssetTrait>(__func__)
is simpler in C++ code@ -137,0 +151,4 @@
return result;
}
AssetTrait *trait = (AssetTrait *)BLI_findstring(&asset_data->traits, value, offsetof(AssetTrait, value));
static_cast<AssetTrait *>
(https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast)@ -102,1 +111,4 @@
char _pad[4];
/** See #AssetTrait. */
ListBase traits;
Not sure what Julian thinks about this, personally I think it's better these days to use an array of pointers in DNA rather than a linked list. i.e.
AssetTrait **
. That makes adding and removing elements a bit more complex, but it means code can be type safe when dealing with an array of traits, because we can pass aroundSpan<const AssetTrait *>
. That's really nice, in comparison with justListBase
.@HooglyBoogly - leaving the traits with a ListBase (for now) to remain consistent with tags, until we here one way or the other.
Cool, works for me. I'll accept and we'll see what Julian thinks about that point (hopefully agrees? :). (For a recent example, see
bNodePanel **panels_array
)Based on discussions about "traits" for node group assets and node tools, it seems like they won't be necessary. They ended up not being a necessary abstraction, since they were basically just a cache of properties that made more sense stored on the data-block itself (not the asset). Not sure if there's still need for them longer term-- I also just don't like leaving this PR in a limbo state (and I'm personally not that convinced by the need for them either). If we don't have a need for them, might make sense to close this for now.
Julian Eisel referenced this pull request2024-05-29 14:46:20 +02:00
Closing this as there have been objections on the design, see !105807.
Pull request closed