Fix #103410: name collisions between vertex groups and attributes #109910

Merged
Philipp Oeser merged 8 commits from lichtwerk/blender:103410 into main 2023-08-08 10:11:18 +02:00
4 changed files with 58 additions and 17 deletions
Showing only changes of commit b8fe121037 - Show all commits

View File

@ -9,6 +9,8 @@
#pragma once
#include "DNA_ID.h"
lichtwerk marked this conversation as resolved Outdated

These includes should be unnecessary with forward declarations. It's good to avoid including headers in headers where possible, especially for such a common file such as BKE_attribute.h.

These includes should be unnecessary with forward declarations. It's good to avoid including headers in headers where possible, especially for such a common file such as `BKE_attribute.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;
lichtwerk marked this conversation as resolved Outdated

It might be nicer to define this right above BKE_id_attribute_and_defgroup_unique_name_check, leaving the top of the file for more generally used / important things like the attribute domain enum definition.

It might be nicer to define this right above `BKE_id_attribute_and_defgroup_unique_name_check`, leaving the top of the file for more generally used / important things like the attribute domain enum definition.
} 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);

View File

@ -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);
/**

View File

@ -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);
lichtwerk marked this conversation as resolved
Review

"with a unique name" seems a bit off here, a bit like unnecessary information-- there are other things that could go wrong. And actually the unique name seems like one thing that generally shouldn't fail? When do you run into this in practice?

"with a unique name" seems a bit off here, a bit like unnecessary information-- there are other things that could go wrong. And actually the unique name seems like one thing that generally shouldn't fail? When do you run into this in practice?
Review

Yep, this was from the very first iteration where I didnt actually have the real solution implemented...

Yep, this was from the very first iteration where I didnt actually have the real solution implemented...
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]);
}

View File

@ -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: {
lichtwerk marked this conversation as resolved Outdated

This function is meant to be called on object data, adding this is misleading compared to the other cases IMO

This function is meant to be called on object data, adding this is misleading compared to the other cases IMO
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<bDeformGroup *>(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<DeformGroupUniqueNameData *>(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<Mesh *>(ob->data);
AttrUniqueData data_attr{&me->id};
BLI_uniquename_cb(BKE_id_attribute_unique_name_check,
&data_attr,
DATA_("Group"),
'.',
dg->name,

AttributeAndDefgroupUniqueNameData data{static_cast<ID *>(ob->data), dg};

This removes the need for the change in defgroup_find_name_dupe

`AttributeAndDefgroupUniqueNameData data{static_cast<ID *>(ob->data), dg};` This removes the need for the change in `defgroup_find_name_dupe`
sizeof(dg->name));
}
}
float BKE_defvert_find_weight(const MDeformVert *dvert, const int defgroup)