WIP: Fix #115413: Missing PropertyRNA rawtype set for some DNA types #115506

Thomas Barlow · 2023-11-28T06:43:12+01:00

Thomas Barlow commented

2023-11-28 06:43:12 +01:00

makesrna.cc#rna_set_raw_property was missing support for the int8_t,
uchar (uint8_t) and ushort (uint16_t) DNA types.

For #115413 specifically, 27a5da4dc3 replaced the char fields used by
the handle_left_type and handle_right_type enum properties with
uint8_t fields. Without an RNA raw type being set for properties using
uint8_t fields, rna_access.cc#RNA_property_raw_type would fall
back to assuming the raw type was PROP_RAW_INT which would then fail
due to #92621.

This patch adds the missing int8_t, uchar and ushort DNA types to
makesrna.cc#rna_set_raw_property.

The int64_t and uint64_t DNA types remain absent from
makesrna.cc#rna_set_raw_property because these types have no
corresponding RawPropertyType and these types can only be used by bool
properties (see IS_DNATYPE_BOOLEAN_COMPAT and the other macros in
RNA_define.hh), and bool properties use PROP_RAW_BOOLEAN.

`makesrna.cc#rna_set_raw_property` was missing support for the `int8_t`, `uchar` (`uint8_t`) and `ushort` (`uint16_t`) DNA types. For #115413 specifically, 27a5da4dc3 replaced the `char` fields used by the `handle_left_type` and `handle_right_type` enum properties with `uint8_t` fields. Without an RNA raw type being set for properties using `uint8_t` fields, `rna_access.cc#RNA_property_raw_type` would fall back to assuming the raw type was `PROP_RAW_INT` which would then fail due to #92621. This patch adds the missing `int8_t`, `uchar` and `ushort` DNA types to `makesrna.cc#rna_set_raw_property`. The `int64_t` and `uint64_t` DNA types remain absent from `makesrna.cc#rna_set_raw_property` because these types have no corresponding `RawPropertyType` and these types can only be used by bool properties (see `IS_DNATYPE_BOOLEAN_COMPAT` and the other macros in `RNA_define.hh`), and bool properties use `PROP_RAW_BOOLEAN`.

Thomas Barlow added 1 commit 2023-11-28 06:43:28 +01:00

ff1d83658c Fix #115413 : Missing PropertyRNA rawtype set for some DNA types

makesrna.cc#rna_set_raw_property was missing support for the `int8_t`,
`uchar` (`uint8_t`) and `ushort` (`uint16_t`) DNA types.

For #115413 specifically, 27a5da4dc3 replaced the `char` fields used by
the `handle_left_type` and `handle_right_type` enum properties with
`uint8_t` fields. Without an RNA raw type being set for properties using
`uint8_t` fields, `rna_access.cc#RNA_property_raw_type()` would fall
back to assuming the raw type was `PROP_RAW_INT` which would then fail
due to #92621.

This patch adds the missing `int8_t`, `uchar` and `ushort` DNA types to
makesrna.cc#rna_set_raw_property.

The `int64_t` and `uint64_t` DNA types remain absent from
makesrna.cc#rna_set_raw_property because these types have no
corresponding RawPropertyType and these types can only be used by bool
properties (see `IS_DNATYPE_BOOLEAN_COMPAT` and the other macros in
RNA_define.hh), and bool properties use `PROP_RAW_BOOLEAN`.

Bastien Montagne commented

2023-12-04 15:20:47 +01:00

I have doubts about unsigned data types having the same PROP_RAW_ mapping as signed types. By reading RAW_GET/RAW_SET code, these feel totally incompatible with handling of unsigned values currently, since they interpret the pointers as signed ones? So the expected unsigned range cannot be properly covered.

Would like to summon @brecht and/or @ideasman42 here too?

Not directly related to this patch:

Am kinda wondering why we should not add the other missing types as well (all the [u]int_xxt, uint, etc) while we are at it?
Also think we should add explicit full support of (u)int_64t to RNA?

I have doubts about unsigned data types having the same `PROP_RAW_` mapping as signed types. By reading `RAW_GET`/`RAW_SET` code, these feel totally incompatible with handling of unsigned values currently, since they interpret the pointers as signed ones? So the expected unsigned range cannot be properly covered. Would like to summon @brecht and/or @ideasman42 here too? --------------------------- Not directly related to this patch: * Am kinda wondering why we should not add the other missing types as well (all the `[u]int_xxt`, `uint`, etc) while we are at it? * Also think we should add explicit full support of `(u)int_64t` to RNA?

Thomas Barlow commented

2023-12-04 15:48:52 +01:00

I have doubts about unsigned data types having the same PROP_RAW_ mapping as signed types. By reading RAW_GET/RAW_SET code, these feel totally incompatible with handling of unsigned values currently, since they interpret the pointers as signed ones? So the expected unsigned range cannot be properly covered.

Not directly related to this patch:

Am kinda wondering why we should not add the other missing types as well (all the [u]int_xxt, uint, etc) while we are at it?

Also think we should add explicit full support of (u)int_64t to RNA?

This is pretty much what I did first, but I decided to keep the patch simpler and it wasn't
clear to me how foreach_getset should deal with unsigned types because the signed/unsigned of the raw type is not exposed to the Python API, which can make it more difficult to write performant foreach_get/set code, and there's the issue of OverflowError being raised when setting unsigned types to negative values: 06abf39164 edit: Now submitted as !115761 so it can be commented on/reviewed as necessary.

> I have doubts about unsigned data types having the same `PROP_RAW_` mapping as signed types. By reading `RAW_GET`/`RAW_SET` code, these feel totally incompatible with handling of unsigned values currently, since they interpret the pointers as signed ones? So the expected unsigned range cannot be properly covered. > > --------------------------- > > Not directly related to this patch: > > * Am kinda wondering why we should not add the other missing types as well (all the `[u]int_xxt`, `uint`, etc) while we are at it? > * Also think we should add explicit full support of `(u)int_64t` to RNA? This is pretty much what I did first, but I decided to keep the patch simpler and it wasn't clear to me how `foreach_getset` should deal with unsigned types because the signed/unsigned of the raw type is not exposed to the Python API, which can make it more difficult to write performant foreach_get/set code, and there's the issue of OverflowError being raised when setting unsigned types to negative values: https://projects.blender.org/Mysteryem/blender/commit/06abf39164e589e4f019a135dd315853e1f4c9c2 edit: Now submitted as !115761 so it can be commented on/reviewed as necessary.

~~Thomas Barlow referenced this pull request 2023-12-04 16:59:35 +01:00~~

RNA: Add missing raw types for DNA types #115761

Brecht Van Lommel commented

2023-12-04 18:38:02 +01:00

I would expect any differences between signed/unsigned to be taken care of by min/max range of the property? Did you check if it does?

As long as setting those values clamps to the range, it should be ok to add the unsigned types.

I would expect any differences between signed/unsigned to be taken care of by min/max range of the property? Did you check if it does? As long as setting those values clamps to the range, it should be ok to add the unsigned types.

Thomas Barlow commented

2023-12-11 06:26:56 +01:00

There's definitely issues with the data types with this patch. The example that I've tested is accessing an int8_t PROP_INT property set to -1 with foreach_get, which returns 255 because the Python ints are constructed from an array that is considered to be char (PROP_RAW_CHAR) type which is unsigned in Blender. The Python ints being created in bpy_rna.cc#foreach_getset() with item = PyLong_FromLong(long(((char *)array)[i]));.

I expect there's similar issues using unsigned types with the existing signed raw types too.

There's definitely issues with the data types with this patch. The example that I've tested is accessing an `int8_t` `PROP_INT` property set to `-1` with `foreach_get`, which returns `255` because the Python ints are constructed from an array that is considered to be `char` (`PROP_RAW_CHAR`) type which is unsigned in Blender. The Python ints being created in `bpy_rna.cc#foreach_getset()` with `item = PyLong_FromLong(long(((char *)array)[i]));`. I expect there's similar issues using unsigned types with the existing signed raw types too.

Thomas Barlow changed title from ~~Fix #115413: Missing PropertyRNA rawtype set for some DNA types~~ to WIP: Fix #115413: Missing PropertyRNA rawtype set for some DNA types

2023-12-13 05:10:53 +01:00

Thomas Barlow commented

2023-12-20 18:51:43 +01:00

Closing in favour of !115761.

Thomas Barlow closed this pull request

2023-12-20 18:51:44 +01:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

WIP: Fix #115413: Missing PropertyRNA rawtype set for some DNA types #115506

Pull request closed