From b8fe12103756076b3170f742329acf2bdc8e02f4 Mon Sep 17 00:00:00 2001 From: Philipp Oeser Date: Mon, 10 Jul 2023 14:57:00 +0200 Subject: [PATCH 1/6] Fix #103410: name collisions between vertex groups and attributes These name collisions should be avoided with customdata, all sorts of issues can arise from those. We already warned in the attributes (but not the vertex groups) list if those were found. Previously, creating a vertex group with the same name as an already existing attribute would allow this (and give said warning). Previously, creating an attribute with the same name as an already existing vertex group would silently fail (as in: dont return a layer) -- and then due to an oversight in 101d04f41f (which assumed a valid layer would always be returned by `BKE_id_attribute_new` and would access stuff in the NULL layer) would even crash. Now name collisions between vertex groups and attributes are handled by ensuring unique names correctly. This is done by running `BLI_uniquename_cb` twice (onnce for attributes and once for vertex groups) -- doing this in one callback is probably not possible since we would run into recursive infinite loops between the two (at least afaict). --- source/blender/blenkernel/BKE_attribute.h | 7 ++++ source/blender/blenkernel/BKE_deform.h | 8 +++++ source/blender/blenkernel/intern/attribute.cc | 25 +++++++++---- source/blender/blenkernel/intern/deform.cc | 35 +++++++++++++------ 4 files changed, 58 insertions(+), 17 deletions(-) diff --git a/source/blender/blenkernel/BKE_attribute.h b/source/blender/blenkernel/BKE_attribute.h index e8538ae19a2..10c8779c0dc 100644 --- a/source/blender/blenkernel/BKE_attribute.h +++ b/source/blender/blenkernel/BKE_attribute.h @@ -9,6 +9,8 @@ #pragma once +#include "DNA_ID.h" + #include "BLI_sys_types.h" #include "BKE_customdata.h" @@ -22,6 +24,10 @@ struct CustomDataLayer; struct ID; struct ReportList; +typedef struct AttrUniqueData { + ID *id; +} AttrUniqueData; + /** #Attribute.domain */ typedef enum eAttrDomain { ATTR_DOMAIN_AUTO = -1, /* Use for nodes to choose automatically based on other data. */ @@ -117,6 +123,7 @@ void BKE_id_attributes_default_color_set(struct ID *id, const char *name); struct CustomDataLayer *BKE_id_attributes_color_find(const struct ID *id, const char *name); +bool BKE_id_attribute_unique_name_check(void *arg, const char *name); bool BKE_id_attribute_calc_unique_name(struct ID *id, const char *name, char *outname); const char *BKE_uv_map_vert_select_name_get(const char *uv_map_name, char *buffer); diff --git a/source/blender/blenkernel/BKE_deform.h b/source/blender/blenkernel/BKE_deform.h index 9d9f1948669..d22789f83ac 100644 --- a/source/blender/blenkernel/BKE_deform.h +++ b/source/blender/blenkernel/BKE_deform.h @@ -4,6 +4,8 @@ #pragma once +#include "DNA_object_types.h" + #ifdef __cplusplus # include "BLI_math_vector_types.hh" # include "BLI_offset_indices.hh" @@ -26,6 +28,11 @@ struct MDeformVert; struct Object; struct bDeformGroup; +typedef struct DeformGroupUniqueNameData { + ID *id; + bDeformGroup *dg; +} DeformGroupUniqueNameData; + bool BKE_object_supports_vertex_groups(const struct Object *ob); const struct ListBase *BKE_object_defgroup_list(const struct Object *ob); struct ListBase *BKE_object_defgroup_list_mutable(struct Object *ob); @@ -89,6 +96,7 @@ int *BKE_object_defgroup_flip_map_single(const struct Object *ob, int BKE_object_defgroup_flip_index(const struct Object *ob, int index, bool use_default); int BKE_object_defgroup_name_index(const struct Object *ob, const char *name); void BKE_object_defgroup_unique_name(struct bDeformGroup *dg, struct Object *ob); +bool BKE_defgroup_unique_name_check(void *arg, const char *name); struct MDeformWeight *BKE_defvert_find_index(const struct MDeformVert *dv, int defgroup); /** diff --git a/source/blender/blenkernel/intern/attribute.cc b/source/blender/blenkernel/intern/attribute.cc index ac6187dfb19..3a027c12f24 100644 --- a/source/blender/blenkernel/intern/attribute.cc +++ b/source/blender/blenkernel/intern/attribute.cc @@ -31,6 +31,7 @@ #include "BKE_attribute.hh" #include "BKE_curves.hh" #include "BKE_customdata.h" +#include "BKE_deform.h" #include "BKE_editmesh.h" #include "BKE_mesh.hh" #include "BKE_pointcloud.h" @@ -223,11 +224,7 @@ bool BKE_id_attribute_rename(ID *id, return true; } -struct AttrUniqueData { - ID *id; -}; - -static bool unique_name_cb(void *arg, const char *name) +bool BKE_id_attribute_unique_name_check(void *arg, const char *name) { AttrUniqueData *data = (AttrUniqueData *)arg; @@ -254,6 +251,8 @@ static bool unique_name_cb(void *arg, const char *name) bool BKE_id_attribute_calc_unique_name(ID *id, const char *name, char *outname) { + bool success = false; + AttrUniqueData data{id}; const int name_maxncpy = CustomData_name_maxncpy_calc(name); @@ -261,8 +260,17 @@ bool BKE_id_attribute_calc_unique_name(ID *id, const char *name, char *outname) * NOTE: We only call IFACE_() if needed to avoid locale lookup overhead. */ BLI_strncpy_utf8(outname, (name && name[0]) ? name : IFACE_("Attribute"), name_maxncpy); + /* Correct name collisions with attributes. */ const char *defname = ""; /* Dummy argument, never used as `name` is never zero length. */ - return BLI_uniquename_cb(unique_name_cb, &data, defname, '.', outname, name_maxncpy); + success |= BLI_uniquename_cb( + BKE_id_attribute_unique_name_check, &data, defname, '.', outname, name_maxncpy); + + /* Also correct name collisions with vertex groups. */ + DeformGroupUniqueNameData data_dg{id, nullptr}; + success |= BLI_uniquename_cb( + BKE_defgroup_unique_name_check, &data_dg, defname, '.', outname, name_maxncpy); + + return success; } CustomDataLayer *BKE_id_attribute_new(ID *id, @@ -301,6 +309,11 @@ CustomDataLayer *BKE_id_attribute_new(ID *id, attributes->add(uniquename, domain, eCustomDataType(type), AttributeInitDefaultValue()); const int index = CustomData_get_named_layer_index(customdata, type, uniquename); + if (index == -1) { + BKE_reportf( + reports, RPT_WARNING, "Layer '%s' could not be created with a unique name", uniquename); + } + return (index == -1) ? nullptr : &(customdata->layers[index]); } diff --git a/source/blender/blenkernel/intern/deform.cc b/source/blender/blenkernel/intern/deform.cc index f315f07bbe0..dffc17b3df9 100644 --- a/source/blender/blenkernel/intern/deform.cc +++ b/source/blender/blenkernel/intern/deform.cc @@ -29,6 +29,7 @@ #include "BLT_translation.h" +#include "BKE_attribute.hh" #include "BKE_customdata.h" #include "BKE_data_transfer.h" #include "BKE_deform.h" /* own include */ @@ -447,6 +448,9 @@ bool BKE_object_supports_vertex_groups(const Object *ob) const ListBase *BKE_id_defgroup_list_get(const ID *id) { switch (GS(id->name)) { + case ID_OB: { + return BKE_object_defgroup_list((const Object *)id); + } case ID_ME: { const Mesh *me = (const Mesh *)id; return &me->vertex_group_names; @@ -681,9 +685,9 @@ int BKE_object_defgroup_flip_index(const Object *ob, int index, const bool use_d return (flip_index == -1 && use_default) ? index : flip_index; } -static bool defgroup_find_name_dupe(const char *name, bDeformGroup *dg, Object *ob) +static bool defgroup_find_name_dupe(const char *name, bDeformGroup *dg, ID *id) { - const ListBase *defbase = BKE_object_defgroup_list(ob); + const ListBase *defbase = BKE_id_defgroup_list_get(id); bDeformGroup *curdef; for (curdef = static_cast(defbase->first); curdef; curdef = curdef->next) { @@ -697,21 +701,30 @@ static bool defgroup_find_name_dupe(const char *name, bDeformGroup *dg, Object * return false; } -struct DeformGroupUniqueNameData { - Object *ob; - bDeformGroup *dg; -}; - -static bool defgroup_unique_check(void *arg, const char *name) +bool BKE_defgroup_unique_name_check(void *arg, const char *name) { DeformGroupUniqueNameData *data = static_cast(arg); - return defgroup_find_name_dupe(name, data->dg, data->ob); + return defgroup_find_name_dupe(name, data->dg, data->id); } void BKE_object_defgroup_unique_name(bDeformGroup *dg, Object *ob) { - DeformGroupUniqueNameData data{ob, dg}; - BLI_uniquename_cb(defgroup_unique_check, &data, DATA_("Group"), '.', dg->name, sizeof(dg->name)); + /* Correct name name collisions with vertex groups. */ + DeformGroupUniqueNameData data{&ob->id, dg}; + BLI_uniquename_cb( + BKE_defgroup_unique_name_check, &data, DATA_("Group"), '.', dg->name, sizeof(dg->name)); + + /* Also correct name name collisions with (mesh) attributes. */ + if (ob->type == OB_MESH) { + Mesh *me = static_cast(ob->data); + AttrUniqueData data_attr{&me->id}; + BLI_uniquename_cb(BKE_id_attribute_unique_name_check, + &data_attr, + DATA_("Group"), + '.', + dg->name, + sizeof(dg->name)); + } } float BKE_defvert_find_weight(const MDeformVert *dvert, const int defgroup) -- 2.30.2 From 9361ad5f19f29895d0b88a4075bf069bba83d22c Mon Sep 17 00:00:00 2001 From: Philipp Oeser Date: Tue, 11 Jul 2023 09:08:26 +0200 Subject: [PATCH 2/6] Unify into a single callback --- source/blender/blenkernel/BKE_attribute.h | 8 +++--- source/blender/blenkernel/BKE_deform.h | 5 ---- source/blender/blenkernel/intern/attribute.cc | 27 +++++++++---------- source/blender/blenkernel/intern/deform.cc | 21 ++++++++------- 4 files changed, 29 insertions(+), 32 deletions(-) diff --git a/source/blender/blenkernel/BKE_attribute.h b/source/blender/blenkernel/BKE_attribute.h index 10c8779c0dc..113a706e736 100644 --- a/source/blender/blenkernel/BKE_attribute.h +++ b/source/blender/blenkernel/BKE_attribute.h @@ -10,6 +10,7 @@ #pragma once #include "DNA_ID.h" +#include "DNA_object_types.h" #include "BLI_sys_types.h" @@ -24,9 +25,10 @@ struct CustomDataLayer; struct ID; struct ReportList; -typedef struct AttrUniqueData { +typedef struct AttributeAndDefgroupUniqueNameData { ID *id; -} AttrUniqueData; + bDeformGroup *dg; +} AttributeAndDefgroupUniqueNameData; /** #Attribute.domain */ typedef enum eAttrDomain { @@ -123,7 +125,7 @@ void BKE_id_attributes_default_color_set(struct ID *id, const char *name); struct CustomDataLayer *BKE_id_attributes_color_find(const struct ID *id, const char *name); -bool BKE_id_attribute_unique_name_check(void *arg, const char *name); +bool BKE_id_attribute_and_defgroup_unique_name_check(void *arg, const char *name); bool BKE_id_attribute_calc_unique_name(struct ID *id, const char *name, char *outname); const char *BKE_uv_map_vert_select_name_get(const char *uv_map_name, char *buffer); diff --git a/source/blender/blenkernel/BKE_deform.h b/source/blender/blenkernel/BKE_deform.h index d22789f83ac..55db113b574 100644 --- a/source/blender/blenkernel/BKE_deform.h +++ b/source/blender/blenkernel/BKE_deform.h @@ -28,11 +28,6 @@ struct MDeformVert; struct Object; struct bDeformGroup; -typedef struct DeformGroupUniqueNameData { - ID *id; - bDeformGroup *dg; -} DeformGroupUniqueNameData; - bool BKE_object_supports_vertex_groups(const struct Object *ob); const struct ListBase *BKE_object_defgroup_list(const struct Object *ob); struct ListBase *BKE_object_defgroup_list_mutable(struct Object *ob); diff --git a/source/blender/blenkernel/intern/attribute.cc b/source/blender/blenkernel/intern/attribute.cc index 3a027c12f24..a0841020ab5 100644 --- a/source/blender/blenkernel/intern/attribute.cc +++ b/source/blender/blenkernel/intern/attribute.cc @@ -224,10 +224,17 @@ bool BKE_id_attribute_rename(ID *id, return true; } -bool BKE_id_attribute_unique_name_check(void *arg, const char *name) +bool BKE_id_attribute_and_defgroup_unique_name_check(void *arg, const char *name) { - AttrUniqueData *data = (AttrUniqueData *)arg; + AttributeAndDefgroupUniqueNameData *data = static_cast( + arg); + /* Checking vertex groups first. */ + if (BKE_defgroup_unique_name_check(data, name)) { + return true; + } + + /* Checking attributes next. */ DomainInfo info[ATTR_DOMAIN_NUM]; get_domains(data->id, info); @@ -251,26 +258,18 @@ bool BKE_id_attribute_unique_name_check(void *arg, const char *name) bool BKE_id_attribute_calc_unique_name(ID *id, const char *name, char *outname) { - bool success = false; + AttributeAndDefgroupUniqueNameData data{id, nullptr}; - AttrUniqueData data{id}; const int name_maxncpy = CustomData_name_maxncpy_calc(name); /* Set default name if none specified. * NOTE: We only call IFACE_() if needed to avoid locale lookup overhead. */ BLI_strncpy_utf8(outname, (name && name[0]) ? name : IFACE_("Attribute"), name_maxncpy); - /* Correct name collisions with attributes. */ + /* Avoid name collisions with vertex groups and other attributes. */ const char *defname = ""; /* Dummy argument, never used as `name` is never zero length. */ - success |= BLI_uniquename_cb( - BKE_id_attribute_unique_name_check, &data, defname, '.', outname, name_maxncpy); - - /* Also correct name collisions with vertex groups. */ - DeformGroupUniqueNameData data_dg{id, nullptr}; - success |= BLI_uniquename_cb( - BKE_defgroup_unique_name_check, &data_dg, defname, '.', outname, name_maxncpy); - - return success; + return BLI_uniquename_cb( + BKE_id_attribute_and_defgroup_unique_name_check, &data, defname, '.', outname, name_maxncpy); } CustomDataLayer *BKE_id_attribute_new(ID *id, diff --git a/source/blender/blenkernel/intern/deform.cc b/source/blender/blenkernel/intern/deform.cc index dffc17b3df9..8320d1011b7 100644 --- a/source/blender/blenkernel/intern/deform.cc +++ b/source/blender/blenkernel/intern/deform.cc @@ -703,28 +703,29 @@ static bool defgroup_find_name_dupe(const char *name, bDeformGroup *dg, ID *id) bool BKE_defgroup_unique_name_check(void *arg, const char *name) { - DeformGroupUniqueNameData *data = static_cast(arg); + AttributeAndDefgroupUniqueNameData *data = static_cast( + arg); return defgroup_find_name_dupe(name, data->dg, data->id); } void BKE_object_defgroup_unique_name(bDeformGroup *dg, Object *ob) { - /* Correct name name collisions with vertex groups. */ - DeformGroupUniqueNameData data{&ob->id, dg}; - BLI_uniquename_cb( - BKE_defgroup_unique_name_check, &data, DATA_("Group"), '.', dg->name, sizeof(dg->name)); - - /* Also correct name name collisions with (mesh) attributes. */ + /* Avoid name collisions with other vertex groups and (mesh) attributes. */ if (ob->type == OB_MESH) { Mesh *me = static_cast(ob->data); - AttrUniqueData data_attr{&me->id}; - BLI_uniquename_cb(BKE_id_attribute_unique_name_check, - &data_attr, + AttributeAndDefgroupUniqueNameData data{&me->id, dg}; + BLI_uniquename_cb(BKE_id_attribute_and_defgroup_unique_name_check, + &data, DATA_("Group"), '.', dg->name, sizeof(dg->name)); } + else { + AttributeAndDefgroupUniqueNameData data{&ob->id, dg}; + BLI_uniquename_cb( + BKE_defgroup_unique_name_check, &data, DATA_("Group"), '.', dg->name, sizeof(dg->name)); + } } float BKE_defvert_find_weight(const MDeformVert *dvert, const int defgroup) -- 2.30.2 From 93eca802fd43c5e12554ede4ff2610393cf3e795 Mon Sep 17 00:00:00 2001 From: Philipp Oeser Date: Fri, 14 Jul 2023 13:17:46 +0200 Subject: [PATCH 3/6] remove unneccessary include --- source/blender/blenkernel/BKE_deform.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/source/blender/blenkernel/BKE_deform.h b/source/blender/blenkernel/BKE_deform.h index 55db113b574..f152123cabb 100644 --- a/source/blender/blenkernel/BKE_deform.h +++ b/source/blender/blenkernel/BKE_deform.h @@ -4,8 +4,6 @@ #pragma once -#include "DNA_object_types.h" - #ifdef __cplusplus # include "BLI_math_vector_types.hh" # include "BLI_offset_indices.hh" -- 2.30.2 From abfd99b9506721c3f2ee5fefed15f975d894604e Mon Sep 17 00:00:00 2001 From: Philipp Oeser Date: Fri, 14 Jul 2023 15:27:36 +0200 Subject: [PATCH 4/6] adress review --- source/blender/blenkernel/BKE_attribute.h | 13 +++++-------- source/blender/blenkernel/intern/attribute.cc | 2 -- source/blender/blenkernel/intern/deform.cc | 8 ++++---- 3 files changed, 9 insertions(+), 14 deletions(-) diff --git a/source/blender/blenkernel/BKE_attribute.h b/source/blender/blenkernel/BKE_attribute.h index ee6b4f137ef..a888a110a0f 100644 --- a/source/blender/blenkernel/BKE_attribute.h +++ b/source/blender/blenkernel/BKE_attribute.h @@ -9,9 +9,6 @@ #pragma once -#include "DNA_ID.h" -#include "DNA_object_types.h" - #include "BLI_sys_types.h" #include "BKE_customdata.h" @@ -25,11 +22,6 @@ struct CustomDataLayer; struct ID; struct ReportList; -typedef struct AttributeAndDefgroupUniqueNameData { - ID *id; - bDeformGroup *dg; -} AttributeAndDefgroupUniqueNameData; - /** #Attribute.domain */ typedef enum eAttrDomain { ATTR_DOMAIN_AUTO = -1, /* Use for nodes to choose automatically based on other data. */ @@ -125,6 +117,11 @@ void BKE_id_attributes_default_color_set(struct ID *id, const char *name); const struct CustomDataLayer *BKE_id_attributes_color_find(const struct ID *id, const char *name); +typedef struct AttributeAndDefgroupUniqueNameData { + struct ID *id; + struct bDeformGroup *dg; +} AttributeAndDefgroupUniqueNameData; + bool BKE_id_attribute_and_defgroup_unique_name_check(void *arg, const char *name); bool BKE_id_attribute_calc_unique_name(struct ID *id, const char *name, char *outname); diff --git a/source/blender/blenkernel/intern/attribute.cc b/source/blender/blenkernel/intern/attribute.cc index 1c0273436c8..f63e06b2a68 100644 --- a/source/blender/blenkernel/intern/attribute.cc +++ b/source/blender/blenkernel/intern/attribute.cc @@ -229,12 +229,10 @@ bool BKE_id_attribute_and_defgroup_unique_name_check(void *arg, const char *name AttributeAndDefgroupUniqueNameData *data = static_cast( arg); - /* Checking vertex groups first. */ if (BKE_defgroup_unique_name_check(data, name)) { return true; } - /* Checking attributes next. */ DomainInfo info[ATTR_DOMAIN_NUM]; get_domains(data->id, info); diff --git a/source/blender/blenkernel/intern/deform.cc b/source/blender/blenkernel/intern/deform.cc index 8320d1011b7..3ee1bc1946d 100644 --- a/source/blender/blenkernel/intern/deform.cc +++ b/source/blender/blenkernel/intern/deform.cc @@ -448,9 +448,6 @@ bool BKE_object_supports_vertex_groups(const Object *ob) const ListBase *BKE_id_defgroup_list_get(const ID *id) { switch (GS(id->name)) { - case ID_OB: { - return BKE_object_defgroup_list((const Object *)id); - } case ID_ME: { const Mesh *me = (const Mesh *)id; return &me->vertex_group_names; @@ -687,7 +684,10 @@ int BKE_object_defgroup_flip_index(const Object *ob, int index, const bool use_d static bool defgroup_find_name_dupe(const char *name, bDeformGroup *dg, ID *id) { - const ListBase *defbase = BKE_id_defgroup_list_get(id); + const ListBase *defbase = (GS(id->name) == ID_OB) ? + BKE_object_defgroup_list((const Object *)id) : + BKE_id_defgroup_list_get(id); + bDeformGroup *curdef; for (curdef = static_cast(defbase->first); curdef; curdef = curdef->next) { -- 2.30.2 From d410b82721dc4bbec89a2c7dc66f3086a271a43a Mon Sep 17 00:00:00 2001 From: Philipp Oeser Date: Sat, 15 Jul 2023 13:46:44 +0200 Subject: [PATCH 5/6] address review --- source/blender/blenkernel/BKE_attribute.h | 1 + source/blender/blenkernel/intern/attribute.cc | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/blender/blenkernel/BKE_attribute.h b/source/blender/blenkernel/BKE_attribute.h index a888a110a0f..0ef9129c036 100644 --- a/source/blender/blenkernel/BKE_attribute.h +++ b/source/blender/blenkernel/BKE_attribute.h @@ -17,6 +17,7 @@ extern "C" { #endif +struct bDeformGroup; struct CustomData; struct CustomDataLayer; struct ID; diff --git a/source/blender/blenkernel/intern/attribute.cc b/source/blender/blenkernel/intern/attribute.cc index f63e06b2a68..53257720b13 100644 --- a/source/blender/blenkernel/intern/attribute.cc +++ b/source/blender/blenkernel/intern/attribute.cc @@ -307,8 +307,7 @@ CustomDataLayer *BKE_id_attribute_new(ID *id, const int index = CustomData_get_named_layer_index(customdata, type, uniquename); if (index == -1) { - BKE_reportf( - reports, RPT_WARNING, "Layer '%s' could not be created with a unique name", uniquename); + BKE_reportf(reports, RPT_WARNING, "Layer '%s' could not be created", uniquename); } return (index == -1) ? nullptr : &(customdata->layers[index]); -- 2.30.2 From 004ff868d7a41f9f8f8e627904f431d59d19089c Mon Sep 17 00:00:00 2001 From: Philipp Oeser Date: Tue, 8 Aug 2023 09:59:20 +0200 Subject: [PATCH 6/6] cast from ob data to ID avoids a distinction in `defgroup_find_name_dupe` --- source/blender/blenkernel/intern/deform.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/source/blender/blenkernel/intern/deform.cc b/source/blender/blenkernel/intern/deform.cc index 73a4c1a3351..937177f07e4 100644 --- a/source/blender/blenkernel/intern/deform.cc +++ b/source/blender/blenkernel/intern/deform.cc @@ -684,9 +684,7 @@ int BKE_object_defgroup_flip_index(const Object *ob, int index, const bool use_d static bool defgroup_find_name_dupe(const char *name, bDeformGroup *dg, ID *id) { - const ListBase *defbase = (GS(id->name) == ID_OB) ? - BKE_object_defgroup_list((const Object *)id) : - BKE_id_defgroup_list_get(id); + const ListBase *defbase = BKE_id_defgroup_list_get(id); LISTBASE_FOREACH (bDeformGroup *, curdef, defbase) { if (dg != curdef) { @@ -720,7 +718,7 @@ void BKE_object_defgroup_unique_name(bDeformGroup *dg, Object *ob) sizeof(dg->name)); } else { - AttributeAndDefgroupUniqueNameData data{&ob->id, dg}; + AttributeAndDefgroupUniqueNameData data{static_cast(ob->data), dg}; BLI_uniquename_cb( BKE_defgroup_unique_name_check, &data, DATA_("Group"), '.', dg->name, sizeof(dg->name)); } -- 2.30.2