griefith/test
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

RNA: Improve handling of ID refcounting by Pointer properties.

Browse Source 
This commit works on two related issues:
1. It is too easy to forget to handle ID refcounting in custom RNA setters.
2. It is not possible to fully force RNA refcounting ON or OFF for some
   properties.

Issue 1. has bit us several times recently (e.g. in animation or
compositor areas, see latest fix blender/blender!143577).

This commit addresses it by adding some debug-only checks around the
call to `PointerPropertyRNA::set()` in `RNA_property_pointer_set()`,
that validates that usercount of both previously and newly assigned IDs
matches expected (changes of) values.

While not ideal, this should make it way easier for developers to catch
that issue in the future.

The second issue was discovered while working on the first, leading to
e.g. questionable work-arounds like the `rna_DriverTarget_id_set`
setter, only defined to by-pass the automatically assigned
`PROP_ID_USERCOUNT` flag (and related automatic refcounting of the
default setter code).

It is addressed by adding an internal `PROP_INTERN_PTR_ID_REFCOUNT_FORCED`
flag, which keeps track of calls to
`RNA_def_property_flag`/`RNA_def_property_clear_flag` which explicitely
sets or clears the `PROP_ID_USERCOUNT` flag, and prevents automatic
setting of that flag in that case.

Pull Request: https://projects.blender.org/blender/blender/pulls/143660
This commit is contained in:
Bastien Montagne
2025-08-01 14:01:24 +02:00
committed by Bastien Montagne
parent 2c27d2be54
commit f4ae983dfb
9 changed files with 122 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
source/blender/makesrna/RNA_types.hh
View File

@@ -341,6 +341,9 @@ enum PropertyFlag {
*
* This is done in the auto-generated setter function. If an RNA property has a custom setter,
* this flag is ignored, and the setter is responsible for correctly updating the user count.
*
* \note In most basic cases, makesrna will automatically set this flag, based on the
* `STRUCT_ID_REFCOUNT` flag of the defined pointer type.
*/
PROP_ID_REFCOUNT = (1 << 6),
@@ -867,8 +870,13 @@ struct FunctionRNA;
/* Struct */
enum StructFlag {
/** Indicates that this struct is an ID struct, and to use reference-counting. */
/** Indicates that this struct is an ID struct. */
STRUCT_ID = (1 << 0),
/**
* Indicates that this ID type's usages should typically be refcounted (i.e. makesrna will
* automatically set `PROP_ID_REFCOUNT` to PointerRNA properties that have this RNA type
* assigned).
*/
STRUCT_ID_REFCOUNT = (1 << 1),
/** defaults on, indicates when changes in members of a StructRNA should trigger undo steps. */
STRUCT_UNDO = (1 << 2),

6
source/blender/makesrna/intern/makesrna.cc
View File

@@ -3542,7 +3542,11 @@ static void rna_auto_types()
pprop->type = (StructRNA *)rna_find_type(dp->dnatype);
}
if (pprop->type) {
/* Only automatically define `PROP_ID_REFCOUNT` if it was not already explicitely set or
* cleared by calls to `RNA_def_property_flag` or `RNA_def_property_clear_flag`. */
if ((pprop->property.flag_internal & PROP_INTERN_PTR_ID_REFCOUNT_FORCED) == 0 &&
pprop->type)
{
type = rna_find_struct((const char *)pprop->type);
if (type && (type->flag & STRUCT_ID_REFCOUNT)) {
pprop->property.flag |= PROP_ID_REFCOUNT;

81
source/blender/makesrna/intern/rna_access.cc
View File

@@ -4215,11 +4215,92 @@ void RNA_property_pointer_set(PointerRNA *ptr,
ptr_value.type->identifier);
return;
}
#ifndef NDEBUG
/* NOTE: By design, it can be safely assumed that both old and new ID pointers are valid when
* accessed through RNA. Handling of invalid ID pointers, e.g. freed ones etc., should never
* happen through RNA code, but directly on underlying (DNA) data.
*
* Setters are also not expected to free or otherwise invalidate ID pointers. So storing them
* here should be safe. */
BLI_assert(pprop->get);
const bool is_id_refcounting = (prop->flag & PROP_ID_REFCOUNT) != 0;
const PointerRNA old_id_ptr = pprop->get(ptr);
BLI_assert_msg(!is_id_refcounting || !old_id_ptr.data || RNA_struct_is_ID(old_id_ptr.type),
"If the property is tagged with ID refcounting, its current value should be "
"null or an ID");
const ID *old_id = (old_id_ptr.type && RNA_struct_is_ID(old_id_ptr.type)) ?
old_id_ptr.data_as<ID>() :
nullptr;
const int old_id_old_refcount = old_id ? ID_REFCOUNTING_USERS(old_id) : 0;
const ID *new_id = (ptr_value.type && RNA_struct_is_ID(ptr_value.type)) ?
ptr_value.data_as<ID>() :
nullptr;
const int new_id_old_refcount = new_id ? ID_REFCOUNTING_USERS(new_id) : 0;
#endif
if (!((prop->flag & PROP_NEVER_NULL) && ptr_value.data == nullptr) &&
!((prop->flag & PROP_ID_SELF_CHECK) && ptr->owner_id == ptr_value.owner_id))
{
pprop->set(ptr, ptr_value, reports);
}
#ifndef NDEBUG
/* NOTE: Current checks are relatively flexible, but do expect 'reasonable' behavior (ID
* handling) from custom setters.
*
* Should there be some very uncommon setter behavior, e.g. unassigning an ID from the property
* automatically assigning it to several other refcounting usages, this will have to be
* tweaked, e.g. by adding a special 'skip checks' flag to such RNA properties. */
PointerRNA current_id_ptr = pprop->get(ptr);
BLI_assert_msg(!is_id_refcounting || !current_id_ptr.data ||
RNA_struct_is_ID(current_id_ptr.type),
"If the property is tagged with ID refcounting, its current value should be "
"null or an ID");
ID *current_id = (current_id_ptr.type && RNA_struct_is_ID(current_id_ptr.type)) ?
static_cast<ID *>(current_id_ptr.data) :
nullptr;
if (old_id) {
const int old_id_new_refcount = ID_REFCOUNTING_USERS(old_id);
if (ELEM(old_id, new_id, current_id)) {
BLI_assert_msg(old_id_new_refcount == old_id_old_refcount,
"Reassigning the same ID to a RNA pointer property, or assignment failure, "
"should not modify the original ID usercount");
}
else if (is_id_refcounting) {
BLI_assert_msg(old_id_new_refcount < old_id_old_refcount,
"Unassigning an ID from a refcounting RNA pointer property should decrease "
"its usercount");
}
else {
BLI_assert_msg(old_id_new_refcount == old_id_old_refcount,
"Unassigning an ID from a non-refcounting RNA pointer property should not "
"modify its usercount");
}
}
if (new_id && new_id != old_id) {
const int new_id_new_refcount = ID_REFCOUNTING_USERS(new_id);
if (current_id == old_id) {
BLI_assert_msg(new_id_new_refcount == new_id_old_refcount,
"Failed assigning a new ID to a RNA pointer property, should not modify "
"the new ID usercount");
}
else if (is_id_refcounting) {
BLI_assert_msg(new_id_new_refcount > new_id_old_refcount,
"Assigning an ID to a refcounting RNA pointer property should increase "
"its usercount");
}
else {
BLI_assert_msg(new_id_new_refcount == new_id_old_refcount,
"Assigning an ID to a non-refcounting RNA pointer property should not "
"modify its usercount");
}
}
#endif
return;
}

14
source/blender/makesrna/intern/rna_define.cc
View File

@@ -1570,6 +1570,9 @@ PropertyRNA *RNA_def_property(StructOrFunctionRNA *cont_,
void RNA_def_property_flag(PropertyRNA *prop, PropertyFlag flag)
{
prop->flag |= flag;
if (flag & PROP_ID_REFCOUNT) {
prop->flag_internal |= PROP_INTERN_PTR_ID_REFCOUNT_FORCED;
}
}
void RNA_def_property_clear_flag(PropertyRNA *prop, PropertyFlag flag)
@@ -1578,6 +1581,9 @@ void RNA_def_property_clear_flag(PropertyRNA *prop, PropertyFlag flag)
if (flag & PROP_PTR_NO_OWNERSHIP) {
prop->flag_internal |= PROP_INTERN_PTR_OWNERSHIP_FORCED;
}
if (flag & PROP_ID_REFCOUNT) {
prop->flag_internal |= PROP_INTERN_PTR_ID_REFCOUNT_FORCED;
}
}
void RNA_def_property_override_flag(PropertyRNA *prop, PropertyOverrideFlag flag)
@@ -1970,8 +1976,12 @@ void RNA_def_property_struct_runtime(StructOrFunctionRNA *cont, PropertyRNA *pro
return;
}
if (type && (type->flag & STRUCT_ID_REFCOUNT)) {
RNA_def_property_flag(prop, PROP_ID_REFCOUNT);
if ((prop->flag_internal & PROP_INTERN_PTR_ID_REFCOUNT_FORCED) == 0 && type &&
(type->flag & STRUCT_ID_REFCOUNT))
{
/* Do not use #RNA_def_property_flag, to avoid this automatic flag definition to set
* #PROP_INTERN_PTR_ID_REFCOUNT_FORCED. */
prop->flag |= PROP_ID_REFCOUNT;
}
break;

12
source/blender/makesrna/intern/rna_fcurve.cc
View File

@@ -351,13 +351,6 @@ static void rna_DriverVariable_update_data(Main *bmain, Scene *scene, PointerRNA
/* ----------- */
/* NOTE: this function exists only to avoid id reference-counting. */
static void rna_DriverTarget_id_set(PointerRNA *ptr, PointerRNA value, ReportList * /*reports*/)
{
DriverTarget *dtar = (DriverTarget *)ptr->data;
dtar->id = static_cast<ID *>(value.data);
}
static StructRNA *rna_DriverTarget_id_typef(PointerRNA *ptr)
{
DriverTarget *dtar = (DriverTarget *)ptr->data;
@@ -1976,11 +1969,10 @@ static void rna_def_drivertarget(BlenderRNA *brna)
prop = RNA_def_property(srna, "id", PROP_POINTER, PROP_NONE);
RNA_def_property_struct_type(prop, "ID");
RNA_def_property_flag(prop, PROP_EDITABLE);
RNA_def_property_clear_flag(prop, PROP_ID_REFCOUNT);
RNA_def_property_override_flag(prop, PROPOVERRIDE_OVERRIDABLE_LIBRARY);
RNA_def_property_editable_func(prop, "rna_DriverTarget_id_editable");
/* NOTE: custom set function is ONLY to avoid rna setting a user for this. */
RNA_def_property_pointer_funcs(
prop, nullptr, "rna_DriverTarget_id_set", "rna_DriverTarget_id_typef", nullptr);
RNA_def_property_pointer_funcs(prop, nullptr, nullptr, "rna_DriverTarget_id_typef", nullptr);
RNA_def_property_ui_text(prop,
"ID",
"ID-block that the specific property used can be found from "

7
source/blender/makesrna/intern/rna_internal_types.hh
View File

@@ -439,6 +439,13 @@ enum PropertyFlagIntern {
* used to prevent automatically setting that one in `makesrna` when pointer is an ID.
*/
PROP_INTERN_PTR_OWNERSHIP_FORCED = (1 << 5),
/**
* Indicates that #PROP_ID_REFCOUNT has been explicitely set (using `RNA_def_property_flag`) or
* cleared (using `RNA_def_property_clear_flag`) by property definition code, and should
* therefore not be automatically defined based on #STRUCT_ID_REFCOUNT of the property type (in
* #rna_auto_types or #RNA_def_property_struct_runtime).
*/
PROP_INTERN_PTR_ID_REFCOUNT_FORCED = (1 << 6),
};
/* Property Types. */

13
source/blender/makesrna/intern/rna_mask.cc
View File

@@ -102,14 +102,6 @@ static void rna_Mask_update_parent(Main *bmain, Scene *scene, PointerRNA *ptr)
rna_Mask_update_data(bmain, scene, ptr);
}
/* NOTE: this function exists only to avoid id reference-counting. */
static void rna_MaskParent_id_set(PointerRNA *ptr, PointerRNA value, ReportList * /*reports*/)
{
MaskParent *mpar = (MaskParent *)ptr->data;
mpar->id = static_cast<ID *>(value.data);
}
static StructRNA *rna_MaskParent_id_typef(PointerRNA *ptr)
{
MaskParent *mpar = (MaskParent *)ptr->data;
@@ -635,10 +627,9 @@ static void rna_def_maskParent(BlenderRNA *brna)
prop = RNA_def_property(srna, "id", PROP_POINTER, PROP_NONE);
RNA_def_property_struct_type(prop, "ID");
RNA_def_property_flag(prop, PROP_EDITABLE);
RNA_def_property_clear_flag(prop, PROP_ID_REFCOUNT);
// RNA_def_property_editable_func(prop, "rna_maskSpline_id_editable");
/* NOTE: custom set function is ONLY to avoid rna setting a user for this. */
RNA_def_property_pointer_funcs(
prop, nullptr, "rna_MaskParent_id_set", "rna_MaskParent_id_typef", nullptr);
RNA_def_property_pointer_funcs(prop, nullptr, nullptr, "rna_MaskParent_id_typef", nullptr);
RNA_def_property_ui_text(
prop, "ID", "ID-block to which masking element would be parented to or to its property");
RNA_def_property_update(prop, 0, "rna_Mask_update_parent");

2
source/blender/makesrna/intern/rna_object.cc
View File

@@ -2322,7 +2322,7 @@ static void rna_def_material_slot(BlenderRNA *brna)
prop = RNA_def_property(srna, "material", PROP_POINTER, PROP_NONE);
RNA_def_property_struct_type(prop, "Material");
RNA_def_property_flag(prop, PROP_EDITABLE);
RNA_def_property_flag(prop, PROP_EDITABLE | PROP_ID_REFCOUNT);
RNA_def_property_editable_func(prop, "rna_MaterialSlot_material_editable");
RNA_def_property_pointer_funcs(prop,
"rna_MaterialSlot_material_get",

18
source/blender/makesrna/intern/rna_space.cc
View File

@@ -2072,15 +2072,6 @@ static int rna_SpaceTextEditor_visible_lines_get(PointerRNA *ptr)
/* Space Properties */
/* NOTE: this function exists only to avoid id reference-counting. */
static void rna_SpaceProperties_pin_id_set(PointerRNA *ptr,
PointerRNA value,
ReportList * /*reports*/)
{
SpaceProperties *sbuts = (SpaceProperties *)(ptr->data);
sbuts->pinid = static_cast<ID *>(value.data);
}
static StructRNA *rna_SpaceProperties_pin_id_typef(PointerRNA *ptr)
{
SpaceProperties *sbuts = (SpaceProperties *)(ptr->data);
@@ -5888,13 +5879,10 @@ static void rna_def_space_properties(BlenderRNA *brna)
prop = RNA_def_property(srna, "pin_id", PROP_POINTER, PROP_NONE);
RNA_def_property_pointer_sdna(prop, nullptr, "pinid");
RNA_def_property_struct_type(prop, "ID");
/* NOTE: custom set function is ONLY to avoid rna setting a user for this. */
RNA_def_property_pointer_funcs(prop,
nullptr,
"rna_SpaceProperties_pin_id_set",
"rna_SpaceProperties_pin_id_typef",
nullptr);
RNA_def_property_pointer_funcs(
prop, nullptr, nullptr, "rna_SpaceProperties_pin_id_typef", nullptr);
RNA_def_property_flag(prop, PROP_EDITABLE | PROP_NEVER_UNLINK);
RNA_def_property_clear_flag(prop, PROP_ID_REFCOUNT);
RNA_def_property_update(
prop, NC_SPACE | ND_SPACE_PROPERTIES, "rna_SpaceProperties_pin_id_update");