ID properties: Support enum values with items #114362

Merged
Lukas Tönne merged 38 commits from LukasTonne/blender:enum-idproperty into main 2023-12-15 10:20:55 +01:00
Member

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.

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.
Lukas Tönne added 16 commits 2023-11-01 11:20:07 +01:00
Lukas Tönne added a new dependency 2023-11-01 11:26:38 +01:00
Lukas Tönne added 1 commit 2023-11-01 12:24:34 +01:00
Lukas Tönne added 2 commits 2023-11-01 14:09:12 +01:00
fd933ee7cb Implement RNA enum item list in rna_access instead of a regular callback.
The callback does not actually work because the data pointer is always
the data that owns the IDProperty rather than the property itselfs.
Since this can be added to any ID or similar property-supporting types
there is no general way of finding the actual IDProperty.
Lukas Tönne changed title from WIP: ID properties: Support enum values with items to ID properties: Support enum values with items 2023-11-02 10:07:08 +01:00
Lukas Tönne requested review from Campbell Barton 2023-11-02 10:07:23 +01:00
Lukas Tönne requested review from Hans Goudey 2023-11-02 10:07:23 +01:00
Hans Goudey reviewed 2023-11-02 22:35:53 +01:00
Hans Goudey left a comment
Member

Looks pretty good to me. I'd still like to test it, didn't get to that. But here are a few comments anyway.

Looks 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) +
Member

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.

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.
LukasTonne marked this conversation as resolved
@ -119,0 +123,4 @@
char *name;
/* Optional description. */
char *description;
/* Unique value, generated automatically. */
Member

Might be worth mentioning how this relates to identifier in the comment. Just reading this it's sort of hard to guess its purpose.

Might be worth mentioning how this relates to `identifier` in the comment. Just reading this it's sort of hard to guess its purpose.
Author
Member

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:

  /* Unique identifier, used for string lookup. */
  char *identifier;
  /* Unique integer value, should never change. */
  int value;

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.

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: ``` /* Unique identifier, used for string lookup. */ char *identifier; /* Unique integer value, should never change. */ int value; ``` 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) {
Member

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 anyway

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 anyway
Author
Member

I'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.

I'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.
Lukas Tönne added 2 commits 2023-11-03 11:12:16 +01:00
Lukas Tönne added 1 commit 2023-11-03 11:13:50 +01:00

Something that's not clear to is why IDP_ENUM is needed.
Couldn't IDP_INT be used with IDPropertyUIDataEnum 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 using IDP_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 create IDP_ENUM types internally.
  • Existing blend files will have enums stored as integers (add-ons settings if ID-data-blocks also tool settings & key-map preferences ... perhaps some other areas too).
  • Existing blend files that contain IDProperty data will only have IDP_INT for these values.
  • Any comparison with old/new data is likely to consider the IDProperty data different because the types differ.
  • I don't think we can version-patch blend files as the run-time RNA data isn't always known when loading the file.
  • From what I can tell loading blend files in 4.0 will loose data unless 4.0 is patched to support this.
  • Supporting both 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...

Something that's not clear to is why `IDP_ENUM` is needed. Couldn't `IDP_INT` be used with `IDPropertyUIDataEnum` 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 using `IDP_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 create `IDP_ENUM` types internally. - Existing blend files will have enums stored as integers (add-ons settings if ID-data-blocks also tool settings & key-map preferences ... perhaps some other areas too). - Existing blend files that contain `IDProperty` data will only have `IDP_INT` for these values. - Any comparison with old/new data is likely to consider the IDProperty data different because the types differ. - I don't think we can version-patch blend files as the run-time RNA data isn't always known when loading the file. - From what I can tell loading blend files in 4.0 will loose data unless 4.0 is patched to support this. - Supporting both `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..._
Campbell Barton requested changes 2023-11-12 05:19:14 +01:00
@ -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).

Worth mentioning in the doc-string that this is a DNA version of `#EnumPropertyItem` (run-time RNA).
LukasTonne marked this conversation as resolved
@ -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 the BPy_EnumValue.

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 the `BPy_EnumValue`.
LukasTonne marked this conversation as resolved
@ -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.

It looks like `RNA_enum_value_from_identifier` could be called instead of defining a new function.
LukasTonne marked this conversation as resolved
@ -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 in bpy_props.cc, worth mentioning any differences and that they follow the same conventions.

This looks similar to `enum_items_from_py` in `bpy_props.cc`, worth mentioning any differences and that they follow the same conventions.
LukasTonne marked this conversation as resolved
@ -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.

Since it's optional, it should be initialized to null.
LukasTonne marked this conversation as resolved
Lukas Tönne added 1 commit 2023-11-13 14:02:49 +01:00
Member

To me it sounds much simpler to keep the existing design and add IDP_ENUM, the same way we added IDP_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.

To me it sounds much simpler to keep the existing design and add `IDP_ENUM`, the same way we added `IDP_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.
Author
Member

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).

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 of IDProperty. 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, even idprop["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 of IDPropertyUIData 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.

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 of `IDProperty`. 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, even `idprop["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 of `IDPropertyUIData` 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.
Campbell Barton requested review from Bastien Montagne 2023-12-07 12:02:20 +01:00
Author
Member

@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.

@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).

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](https://docs.python.org/3/library/enum.html), 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).
Author
Member

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. The eIDPropertySubType 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 an IDP_INT is actually an enum - but then knowing the subtype would be quite useless anyway because we wouldn't have any enum items.

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. The `eIDPropertySubType` 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 an `IDP_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.

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.

but then knowing the subtype would be quite useless anyway because we wouldn't have any enum items.

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.

> but then knowing the subtype would be quite useless anyway because we wouldn't have any enum items. 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.
Lukas Tönne added 2 commits 2023-12-14 12:02:47 +01:00
Lukas Tönne added 1 commit 2023-12-14 12:18:39 +01:00
23a0955566 Remove the idprop serializer tests for enums.
To this serializer an enum property is now indistinguishable from an int
property since it does not take UI data into account.
Lukas Tönne added 2 commits 2023-12-14 13:40:42 +01:00
Lukas Tönne added 1 commit 2023-12-14 13:50:20 +01:00
Lukas Tönne added 3 commits 2023-12-14 14:17:08 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
b04b607fdb
Explain similarities and differences of enum item parsing functions.
Author
Member

@blender-bot build

@blender-bot build
Lukas Tönne added 1 commit 2023-12-14 15:14:37 +01:00
ec606446f4 Sanity check for enum item strings.
Identifier and name strings must never be null (assert). Description
can be null, so appropriate checks must be used while copying.
Author
Member

Still have to add a special case in rna_access.cc to actually display custom int properties with enum items as an PROP_ENUM property. It's currently based only on the base type, which always maps it to a PROP_INT.

Still have to add a special case in `rna_access.cc` to actually display custom int properties with enum items as an `PROP_ENUM` property. It's currently based only on the base type, which always maps it to a `PROP_INT`.
Lukas Tönne added 1 commit 2023-12-14 16:15:18 +01:00
01d65b15fe Map IDP_INT properties to PROP_ENUM when it has enum items.
This will automatically display a custom int property as a dropdown
button when enum items are added.
Author
Member

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).

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). <video src="/attachments/2155b349-8489-4e11-aa7b-230e98e5a312" title="2023-12-14 16-11-59.mp4" controls></video>
Lukas Tönne added 1 commit 2023-12-14 17:14:10 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
25deae63de
Fix RNA enum items count returned for ID properties.
The terminator item must not be included in the items count.
Author
Member

@blender-bot build

@blender-bot build
Campbell Barton requested changes 2023-12-14 23:44:03 +01:00
@ -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.

Should be removed.
LukasTonne marked this conversation as resolved
@ -119,0 +223,4 @@
"items",
"subtype",
"description",
nullptr};

picky - prefer trailing comma, to reduce right shift.

*picky* - prefer trailing comma, to reduce right shift.
LukasTonne marked this conversation as resolved
@ -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.

`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.
LukasTonne marked this conversation as resolved
Lukas Tönne added 3 commits 2023-12-15 10:04:31 +01:00
Campbell Barton approved these changes 2023-12-15 10:09:04 +01:00
Campbell Barton left a comment
Owner

Don't think this requires an extra iteration from me, accepting.

Don't think this requires an extra iteration from me, accepting.
Lukas Tönne merged commit 92cf9dd2f2 into main 2023-12-15 10:20:55 +01:00
Lukas Tönne deleted branch enum-idproperty 2023-12-15 10:20:58 +01:00
Sign in to join this conversation.
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
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Reference: blender/blender#114362
No description provided.