ID properties: Support enum values with items #114362
No reviewers
Labels
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
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
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
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
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
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
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date
No due date set.
Blocks
#113445 Geometry Nodes: Menu Switch Node
blender/blender
Reference: blender/blender#114362
Loading…
Reference in New Issue
No description provided.
Delete Branch "LukasTonne/blender:enum-idproperty"
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?
Add support for enum values in ID properties. This is needed for the "Menu Switch" node implementation (#113445) which relies on ID properties for the top-level modifier UI.
Enums properties are stored with their unique integer values. Each enum value stores a full set of the enum items along with the current value. In the future there could be a mechanism for sharing such ID definitions, but for now this keeps the feature simple (or as simple as it can be). Enum items are stored in the ID property UI data, like min/max values etc. for other types.
There is no support yet for editing enum-type custom property items through the UI. This is because the "Edit Property" feature is implemented entirely through a single operator (
WM_OT_properties_edit
) and its properties. Buttons to add/remove/move items would be operator changing another operator's properties. A refactor of the custom properties UI is likely required to make this work, which is out of scope of this PR.WIP: ID properties: Support enum values with itemsto ID properties: Support enum values with itemsLooks pretty good to me. I'd still like to test it, didn't get to that. But here are a few comments anyway.
@ -431,0 +496,4 @@
}
if (!used_identifiers.add(item.identifier)) {
if (error_fn) {
std::string msg = std::string("Item identifier '") + std::string(item.identifier) +
Not sure if these messages will end up in the UI eventually, but I'd change how they're created slightly so they would be easily translate-able if that becomes the case. Generally
fmt
is what we've been using in C++ code for this sort of thing.This can be:
const std::string msg = fmt::format("Item identifier '{}' is already used, item.identifier);
Same below.
@ -119,0 +123,4 @@
char *name;
/* Optional description. */
char *description;
/* Unique value, generated automatically. */
Might be worth mentioning how this relates to
identifier
in the comment. Just reading this it's sort of hard to guess its purpose.Comment was somewhat outdated, this can actually be specified when creating the enum from python, same as with RNA enum properties created from scripts.
Comments on RNA
EnumPropertyItem
call it "The internal value of the enum, not exposed to users" which is a bit of a blurry line. I guess "not exposed in the UI" would be a bit more accurate.RNA comments don't explain how these are used, but if i have to explain i'd say:
The enum values of the menu switch node can be managed internally to ensure unique and persistent values, but the ID property enums get replaced entirely each time, so the ID has to be "user"-defined.
@ -1501,6 +1508,36 @@ void RNA_property_enum_items_ex(bContext *C,
int *r_totitem,
bool *r_free)
{
if (!use_static && prop->magic != RNA_MAGIC) {
Are these two checks redundant conceptually? If so, might be worth just checking for
RNA_MAGIC
and asserting for!use_static
inside. Just for clarity anywayI've interpreted
use_static
to mean that the function is supposed to return just the static enum items, and ignore any dynamic items. The ID properties have dummy static items like other dynamic enums (rna_enum_dummy_DEFAULT_items
). They do not have an item callback though.So these are not redundant, the
RNA_MAGIC
flag just means dynamic items are generated by the ID property ui data instead of a callback.Something that's not clear to is why
IDP_ENUM
is needed.Couldn't
IDP_INT
be used withIDPropertyUIDataEnum
to show that it's an enum?If it's important to tag the INT as an
enum
a subtype or flag could be used. Mentioning this since it seems that practically all access of IDP_ENUM's is as an int except via the UI, so it might make more sense to keep usingIDP_INT
internally.This adds the need for
BPy_EnumValue_Type
... which seems like API overhead we could avoid if enums are simply a UI-view on integers.Looking into this further, EnumValue isn't used for return values, so assigning values between ID properties will loose the enum, assigning a value to it's self will also convert it from an enum to an integer (in general it should be possible to assign a value between ID properties or assign values to themselves without loosing information).
From reading the patch it's also not clear exactly what states are expected/supported regarding versioning.
RNA_property_enum_set(...)
will createIDP_ENUM
types internally.IDProperty
data will only haveIDP_INT
for these values.IDP_INT
&IDP_ENUM
underlying types as well as adding exceptions so they compare as equal is possible but is a hint that the choice to use a different type is causing more problems than it solves.So from what I can see it's most straightforward not to add
IDP_ENUM
and make enum access UI-only.An enum that supports toggling (
PROP_ENUM_FLAG
would be nice to support, while I don't think it's a requirement, it also doesn't seem like it should be difficult to add).Other notes inline...
@ -116,6 +116,26 @@ typedef struct IDPropertyUIDataID {
char _pad[6];
} IDPropertyUIDataID;
typedef struct IDPropertyUIDataEnumItem {
Worth mentioning in the doc-string that this is a DNA version of
#EnumPropertyItem
(run-time RNA).@ -682,0 +694,4 @@
IDPropertyTemplate val = {0};
val.i = enum_ob->value;
if (val.i == -1 && PyErr_Occurred()) {
The
PyErr_Occurred()
check only makes sense if integer conversion just ran (and failed), from what I can see this isn't the case here and the error check will have been caught when creating theBPy_EnumValue
.@ -432,6 +432,172 @@ static bool idprop_ui_data_update_string(IDProperty *idprop, PyObject *args, PyO
return true;
}
static int icon_id_from_name(const char *name)
It looks like
RNA_enum_value_from_identifier
could be called instead of defining a new function.@ -435,0 +458,4 @@
return false;
}
static bool try_parse_enum_item(PyObject *py_item, const int index, IDPropertyUIDataEnumItem &item)
This looks similar to
enum_items_from_py
inbpy_props.cc
, worth mentioning any differences and that they follow the same conventions.@ -435,0 +547,4 @@
*/
static bool idprop_ui_data_update_enum(IDProperty *idprop, PyObject *args, PyObject *kwargs)
{
PyObject *items;
Since it's optional, it should be initialized to null.
To me it sounds much simpler to keep the existing design and add
IDP_ENUM
, the same way we addedIDP_BOOL
. That way every RNA property type has a matching IDProperty type, and there's a simple association of UI data types and ID property types. The cost for adding a new type is just boilerplate, it doesn't add much more complexity, whereas making the mapping more complex has a larger cost IMO.Technically Campbell's suggestion can work, i've tested removing
IDP_ENUM
and moving the ui data into the Int property.However, i agree with Hans that having a distinct
IDP_ENUM
type is helpful. Otherwise we have to do a subtype check on IDP_INT in a lot of places, which doesn't make things much easier (esp. given how confusingly subtype is handled in id props).It's still not clear to me why a subtype is necessary, couldn't an enum be a "view" on an integer which is stored in the
IDPropertyUIData
?While it's a bit odd to make an ENUM part of
IDPropertyUIDataInt
it's localized to UI data and not impact the underlying structure ofIDProperty
. With longer term implications for file versioning, data API and access methods to property detect and maintain the int/enum state when manipulating data.Otherwise
IDProperty
are no longer a dictionary of primitives which C/Python can represent natively, it means your can't do a round-trip conversion to a Python dictionary & back without loosing data, evenidprop["key"] = idprop["key"]
would be a destructive operation that changes an enum type back to an integer *¹. Having two kinds of numbers that can't be usefully differentiated manipulated will lead to awkward access methods.This seems error prone and something we would be better off avoiding long term. Part of the value of
IDProperty
is that they're a collection of primitive, the advantage ofIDPropertyUIData
is we can add complexity there, keeping the underlying data simple.Where the
IDPropertyUIData
can define a float3 as a color or a euler ... etc. I think it makes sense to treat enums this way too.*¹ while the example may seem contrived - storing values & setting them back to their original value - or copying values between data-blocks is a common enough use-case, where a script author would reasonably expect the values not to have their type changed on re-assignment.
@ideasman42 Thanks for the explanation, i think this is a compelling argument. If @HooglyBoogly agrees i will implement it that way to get this thing un-stuck.
My first impression was that it made more sense to have a new IDP type, it feels cleaner and clearer.
However, the more I think about it, the more I agree with @ideasman42 and think that at least for now, it's best to keep it as some UI-data only 'view' built on top of a normal
IDP_INT
property.Compatibility and versioning
This is the most compelling point. While not getting data in Blender 4.0 is not really a problem (loss of data when opening in older version is expected), the backward compatibility issue caused by storing py-defined RNA enum properties in a new IDP type are not acceptable. And I cannot find a satisfying way to do versioning here, we could use some tricks (like re-using and converting an existing
IDP_INT
storage property when a new RNA enum property is defined for the same identifier). But these would be weak at best, and would likely become a source of problems later on.Potential enum data type
I am not happy seeing the 'ui data' of IDProperties becoming more and more like a second RNA property definition system. In that sense, I would at least expect a fully new IDP type to have info about it's own values stored as part of the IDProperty itself, not as part of some optional 'ui data'. Since this is not a realistic option currently, I'd indeed rather keep the storage itself as what it is: an integer.
This also relates to the topic of having a proper enum type on Python side of our API, currently we don't [1]. RNA enum properties are exposed as strings, so ideally we'd also expose IDProps enum values as strings? Not sure though how complicated this would be to deal with, especially when it comes to assignment.
So I think that an int is 'good enough' (or the 'least worse solution') for the time being. This should also solve the issue of assigning a value from python directly (having to assign a whole specific type of data instead of the int you got on access is... really not something we want in our API).
[1] Was checking briefly yesterday on Python's Enum (meta)type, but by the look of it this is still completely incompatible with the concept of dynamic enum items, so I doubt we'd get anything new here in the near future?
Notes
While checking this patch I also noted that clicking on the 'Edit' button in the UI next to an enum custom property raises a python error (which is expected I think? But then this operator should be disabled for this type of data).
Do we need an actual
eIDPropertySubType
for enums? That seems somewhat redundant: if the property has enum items it's an enum, otherwise it's a plain int. TheeIDPropertySubType
also has a comment saying it's used only for string properties.EDIT: One reason i can think of that would justify a subtype is that the enum items are stored only in UI data, while the subtype is part of the basic
IDProperty
. I'm not sure if there is ever a situation where we don't have access to the UI data to determine if anIDP_INT
is actually an enum - but then knowing the subtype would be quite useless anyway because we wouldn't have any enum items.Tend to agree with Campbell on this as well... Could be 'nice to have' in absolute, but I am not convinced it would really be useful in current situation, so would rather keep the whole 'is an enum' thing exclusively in the 'ui data' for the time being.
Precisely. If we do not have a full-fledged Enum IDProp type, including its valid values etc., then the storage itself is as in C: a mere integer. So I cannot see any useful case to have that extra sub-type info for the time being, besides potentially for debugging/reporting info.
@blender-bot build
Still have to add a special case in
rna_access.cc
to actually display custom int properties with enum items as anPROP_ENUM
property. It's currently based only on the base type, which always maps it to aPROP_INT
.That was surprisingly easy.
Example below shows adding an int property, then add enum items, and the button switches to a dropdown display. Also includes a demo of the modifier nodes case, which generates such ID properties internally (#113445).
@blender-bot build
@ -54,6 +57,7 @@ static size_t idp_size_table[] = {
sizeof(double), /* #IDP_DOUBLE */
0, /* #IDP_IDPARRAY (no fixed size). */
sizeof(int8_t), /* #IDP_BOOLEAN */
sizeof(int), /* #IDP_ENUM */
Should be removed.
@ -119,0 +223,4 @@
"items",
"subtype",
"description",
nullptr};
picky - prefer trailing comma, to reduce right shift.
@ -176,0 +300,4 @@
PyErr_SetString(PyExc_ValueError, msg);
}))
{
return false;
Py_DECREF(items_fast);
looks to be missed in both failure or success case.I find it's often simpler to use a error boolean so there only needs to be a single
Py_DECREF(..)
, then return if error is true.Don't think this requires an extra iteration from me, accepting.