From abe5d7869a0fc79ed5388e9e3cd74c6d8b78d5bf Mon Sep 17 00:00:00 2001 From: Philipp Oeser Date: Wed, 11 Jan 2023 12:51:18 +0100 Subject: [PATCH 1/2] Fix #103761: creating a color attribute doesn't make it active Originally caused by 6514bb05ea5a More cases where the active/default color attributes were not set correctly: [1] Using the old Python vertex_colors API (vertex_colors.new) [2] OBJ importer [3] Collada importer [4] Data Transfer layout (both standalone operator and "Generate Data Layers" from the modifier) Similar to 101d04f41ffb. Brought over from https://archive.blender.org/developer/D16977, see discussion there why some of the code for data transfer is not for the genereal attribute API. --- .../blenkernel/intern/data_transfer.cc | 130 ++++++++++++++++++ source/blender/io/collada/MeshImporter.cpp | 2 + .../wavefront_obj/importer/obj_import_mesh.cc | 1 + source/blender/makesrna/intern/rna_mesh.c | 7 + 4 files changed, 140 insertions(+) diff --git a/source/blender/blenkernel/intern/data_transfer.cc b/source/blender/blenkernel/intern/data_transfer.cc index 740af7fa4db..b1633a78e51 100644 --- a/source/blender/blenkernel/intern/data_transfer.cc +++ b/source/blender/blenkernel/intern/data_transfer.cc @@ -250,6 +250,120 @@ int BKE_object_data_transfer_dttype_to_srcdst_index(const int dtdata_type) /* ********** */ +/* Helpers to match active color attributes + * The special distinction of domain and type does not fit the general attribute API well and is + * only really needed here (since the data transfer can go over multiple of these "at once" while + * internally this still goes sequentially one after the other, so special care needs to be taken + * to not set wrong attributes active). */ + +static CustomDataLayer *data_transfer_attributes_color_float_find(const ID *id, const char *name) +{ + if (!name) { + return nullptr; + } + CustomDataLayer *layer = BKE_id_attribute_find(id, name, CD_PROP_COLOR, ATTR_DOMAIN_POINT); + if (layer == nullptr) { + layer = BKE_id_attribute_find(id, name, CD_PROP_COLOR, ATTR_DOMAIN_CORNER); + } + + return layer; +} + +static CustomDataLayer *data_transfer_attributes_color_byte_find(const ID *id, const char *name) +{ + if (!name) { + return nullptr; + } + CustomDataLayer *layer = BKE_id_attribute_find(id, name, CD_PROP_BYTE_COLOR, ATTR_DOMAIN_POINT); + if (layer == nullptr) { + layer = BKE_id_attribute_find(id, name, CD_PROP_BYTE_COLOR, ATTR_DOMAIN_CORNER); + } + + return layer; +} + +/** + * When transfering color attributes, also transfer the active color attribute string. + * If a match cant be found, use the first color layer that can be found (to ensure a valid string + * is set). + */ +static void data_transfer_mesh_attributes_transfer_active_color_string( + Mesh *mesh_dst, const Mesh *mesh_src, const eAttrDomainMask mask_domain, const int data_type) +{ + if (mesh_dst->active_color_attribute) { + return; + } + + const char *active_color_src = BKE_id_attributes_active_color_name(&mesh_src->id); + + if ((data_type == CD_PROP_COLOR) && + !data_transfer_attributes_color_float_find(&mesh_src->id, active_color_src)) { + return; + } + else if ((data_type == CD_PROP_BYTE_COLOR) && + !data_transfer_attributes_color_byte_find(&mesh_src->id, active_color_src)) { + return; + } + + if ((data_type == CD_PROP_COLOR) && + data_transfer_attributes_color_float_find(&mesh_dst->id, active_color_src)) { + mesh_dst->active_color_attribute = BLI_strdup(active_color_src); + } + else if ((data_type == CD_PROP_BYTE_COLOR) && + data_transfer_attributes_color_byte_find(&mesh_dst->id, active_color_src)) { + mesh_dst->active_color_attribute = BLI_strdup(active_color_src); + } + else { + CustomDataLayer *first_color_layer = BKE_id_attribute_from_index( + &mesh_dst->id, 0, mask_domain, CD_MASK_COLOR_ALL); + if (first_color_layer != nullptr) { + mesh_dst->active_color_attribute = BLI_strdup(first_color_layer->name); + } + } +} + +/** + * When transfering color attributes, also transfer the default color attribute string. + * If a match cant be found, use the first color layer that can be found (to ensure a valid string + * is set). + */ +static void data_transfer_mesh_attributes_transfer_default_color_string( + Mesh *mesh_dst, const Mesh *mesh_src, const eAttrDomainMask mask_domain, const int data_type) +{ + if (mesh_dst->default_color_attribute) { + return; + } + + const char *default_color_src = BKE_id_attributes_default_color_name(&mesh_src->id); + + if ((data_type == CD_PROP_COLOR) && + !data_transfer_attributes_color_float_find(&mesh_src->id, default_color_src)) { + return; + } + else if ((data_type == CD_PROP_BYTE_COLOR) && + !data_transfer_attributes_color_byte_find(&mesh_src->id, default_color_src)) { + return; + } + + if ((data_type == CD_PROP_COLOR) && + data_transfer_attributes_color_float_find(&mesh_dst->id, default_color_src)) { + mesh_dst->default_color_attribute = BLI_strdup(default_color_src); + } + else if ((data_type == CD_PROP_BYTE_COLOR) && + data_transfer_attributes_color_byte_find(&mesh_dst->id, default_color_src)) { + mesh_dst->default_color_attribute = BLI_strdup(default_color_src); + } + else { + CustomDataLayer *first_color_layer = BKE_id_attribute_from_index( + &mesh_dst->id, 0, mask_domain, CD_MASK_COLOR_ALL); + if (first_color_layer != nullptr) { + mesh_dst->default_color_attribute = BLI_strdup(first_color_layer->name); + } + } +} + +/* ********** */ + /* Generic pre/post processing, only used by custom loop normals currently. */ static void data_transfer_dtdata_type_preprocess(Mesh *me_src, @@ -1124,6 +1238,14 @@ void BKE_object_data_transfer_layout(struct Depsgraph *depsgraph, fromlayers, tolayers, nullptr); + /* Make sure we have active/defaut color layers if none existed before. + * Use the active/defaut from src (if it was transferred), otherwise the first. */ + if (ELEM(cddata_type, CD_PROP_COLOR, CD_PROP_BYTE_COLOR)) { + data_transfer_mesh_attributes_transfer_active_color_string( + me_dst, me_src, ATTR_DOMAIN_MASK_POINT, cddata_type); + data_transfer_mesh_attributes_transfer_default_color_string( + me_dst, me_src, ATTR_DOMAIN_MASK_POINT, cddata_type); + } } if (DT_DATATYPE_IS_EDGE(dtdata_type)) { const int num_elem_dst = me_dst->totedge; @@ -1164,6 +1286,14 @@ void BKE_object_data_transfer_layout(struct Depsgraph *depsgraph, fromlayers, tolayers, nullptr); + /* Make sure we have active/defaut color layers if none existed before. + * Use the active/defaut from src (if it was transferred), otherwise the first. */ + if (ELEM(cddata_type, CD_PROP_COLOR, CD_PROP_BYTE_COLOR)) { + data_transfer_mesh_attributes_transfer_active_color_string( + me_dst, me_src, ATTR_DOMAIN_MASK_CORNER, cddata_type); + data_transfer_mesh_attributes_transfer_default_color_string( + me_dst, me_src, ATTR_DOMAIN_MASK_CORNER, cddata_type); + } } if (DT_DATATYPE_IS_POLY(dtdata_type)) { const int num_elem_dst = me_dst->totpoly; diff --git a/source/blender/io/collada/MeshImporter.cpp b/source/blender/io/collada/MeshImporter.cpp index 288e208fe6c..e3a20aae5fd 100644 --- a/source/blender/io/collada/MeshImporter.cpp +++ b/source/blender/io/collada/MeshImporter.cpp @@ -491,6 +491,8 @@ void MeshImporter::allocate_poly_data(COLLADAFW::Mesh *collada_mesh, Mesh *me) } BKE_id_attributes_active_color_set( &me->id, CustomData_get_layer_name(&me->ldata, CD_PROP_BYTE_COLOR, 0)); + BKE_id_attributes_default_color_set( + &me->id, CustomData_get_layer_name(&me->ldata, CD_PROP_BYTE_COLOR, 0)); } } } diff --git a/source/blender/io/wavefront_obj/importer/obj_import_mesh.cc b/source/blender/io/wavefront_obj/importer/obj_import_mesh.cc index aa89be9780c..ac27f53184e 100644 --- a/source/blender/io/wavefront_obj/importer/obj_import_mesh.cc +++ b/source/blender/io/wavefront_obj/importer/obj_import_mesh.cc @@ -401,6 +401,7 @@ void MeshFromGeometry::create_colors(Mesh *mesh) CustomDataLayer *color_layer = BKE_id_attribute_new( &mesh->id, "Color", CD_PROP_COLOR, ATTR_DOMAIN_POINT, nullptr); BKE_id_attributes_active_color_set(&mesh->id, color_layer->name); + BKE_id_attributes_default_color_set(&mesh->id, color_layer->name); float4 *colors = (float4 *)color_layer->data; int offset = mesh_geometry_.vertex_index_min_ - block.start_vertex_index; for (int i = 0, n = mesh_geometry_.get_vertex_count(); i != n; ++i) { diff --git a/source/blender/makesrna/intern/rna_mesh.c b/source/blender/makesrna/intern/rna_mesh.c index e660513a243..060efd979ca 100644 --- a/source/blender/makesrna/intern/rna_mesh.c +++ b/source/blender/makesrna/intern/rna_mesh.c @@ -2292,6 +2292,13 @@ static PointerRNA rna_Mesh_vertex_color_new(struct Mesh *me, if (index != -1) { ldata = rna_mesh_ldata_helper(me); cdl = &ldata->layers[CustomData_get_layer_index_n(ldata, CD_PROP_BYTE_COLOR, index)]; + + if (!me->active_color_attribute) { + me->active_color_attribute = BLI_strdup(cdl->name); + } + if (!me->default_color_attribute) { + me->default_color_attribute = BLI_strdup(cdl->name); + } } RNA_pointer_create(&me->id, &RNA_MeshLoopColorLayer, cdl, &ptr); -- 2.30.2 From bbb8575ee3bc8a1eb6d39b94481dd78d771bb5a0 Mon Sep 17 00:00:00 2001 From: Philipp Oeser Date: Wed, 22 Feb 2023 14:38:32 +0100 Subject: [PATCH 2/2] Address Code review: - use BKE_id_attribute_search() to simplify code - typo in comment --- .../blenkernel/intern/data_transfer.cc | 54 ++++--------------- 1 file changed, 11 insertions(+), 43 deletions(-) diff --git a/source/blender/blenkernel/intern/data_transfer.cc b/source/blender/blenkernel/intern/data_transfer.cc index b1633a78e51..225011f1556 100644 --- a/source/blender/blenkernel/intern/data_transfer.cc +++ b/source/blender/blenkernel/intern/data_transfer.cc @@ -250,45 +250,13 @@ int BKE_object_data_transfer_dttype_to_srcdst_index(const int dtdata_type) /* ********** */ -/* Helpers to match active color attributes - * The special distinction of domain and type does not fit the general attribute API well and is - * only really needed here (since the data transfer can go over multiple of these "at once" while - * internally this still goes sequentially one after the other, so special care needs to be taken - * to not set wrong attributes active). */ - -static CustomDataLayer *data_transfer_attributes_color_float_find(const ID *id, const char *name) -{ - if (!name) { - return nullptr; - } - CustomDataLayer *layer = BKE_id_attribute_find(id, name, CD_PROP_COLOR, ATTR_DOMAIN_POINT); - if (layer == nullptr) { - layer = BKE_id_attribute_find(id, name, CD_PROP_COLOR, ATTR_DOMAIN_CORNER); - } - - return layer; -} - -static CustomDataLayer *data_transfer_attributes_color_byte_find(const ID *id, const char *name) -{ - if (!name) { - return nullptr; - } - CustomDataLayer *layer = BKE_id_attribute_find(id, name, CD_PROP_BYTE_COLOR, ATTR_DOMAIN_POINT); - if (layer == nullptr) { - layer = BKE_id_attribute_find(id, name, CD_PROP_BYTE_COLOR, ATTR_DOMAIN_CORNER); - } - - return layer; -} - /** * When transfering color attributes, also transfer the active color attribute string. - * If a match cant be found, use the first color layer that can be found (to ensure a valid string + * If a match can't be found, use the first color layer that can be found (to ensure a valid string * is set). */ static void data_transfer_mesh_attributes_transfer_active_color_string( - Mesh *mesh_dst, const Mesh *mesh_src, const eAttrDomainMask mask_domain, const int data_type) + Mesh *mesh_dst, Mesh *mesh_src, const eAttrDomainMask mask_domain, const int data_type) { if (mesh_dst->active_color_attribute) { return; @@ -297,20 +265,20 @@ static void data_transfer_mesh_attributes_transfer_active_color_string( const char *active_color_src = BKE_id_attributes_active_color_name(&mesh_src->id); if ((data_type == CD_PROP_COLOR) && - !data_transfer_attributes_color_float_find(&mesh_src->id, active_color_src)) { + !BKE_id_attribute_search(&mesh_src->id, active_color_src, CD_MASK_PROP_COLOR, ATTR_DOMAIN_MASK_COLOR)) { return; } else if ((data_type == CD_PROP_BYTE_COLOR) && - !data_transfer_attributes_color_byte_find(&mesh_src->id, active_color_src)) { + !BKE_id_attribute_search(&mesh_src->id, active_color_src, CD_MASK_PROP_BYTE_COLOR, ATTR_DOMAIN_MASK_COLOR)) { return; } if ((data_type == CD_PROP_COLOR) && - data_transfer_attributes_color_float_find(&mesh_dst->id, active_color_src)) { + BKE_id_attribute_search(&mesh_dst->id, active_color_src, CD_MASK_PROP_COLOR, ATTR_DOMAIN_MASK_COLOR)) { mesh_dst->active_color_attribute = BLI_strdup(active_color_src); } else if ((data_type == CD_PROP_BYTE_COLOR) && - data_transfer_attributes_color_byte_find(&mesh_dst->id, active_color_src)) { + BKE_id_attribute_search(&mesh_dst->id, active_color_src, CD_MASK_PROP_BYTE_COLOR, ATTR_DOMAIN_MASK_COLOR)) { mesh_dst->active_color_attribute = BLI_strdup(active_color_src); } else { @@ -328,7 +296,7 @@ static void data_transfer_mesh_attributes_transfer_active_color_string( * is set). */ static void data_transfer_mesh_attributes_transfer_default_color_string( - Mesh *mesh_dst, const Mesh *mesh_src, const eAttrDomainMask mask_domain, const int data_type) + Mesh *mesh_dst, Mesh *mesh_src, const eAttrDomainMask mask_domain, const int data_type) { if (mesh_dst->default_color_attribute) { return; @@ -337,20 +305,20 @@ static void data_transfer_mesh_attributes_transfer_default_color_string( const char *default_color_src = BKE_id_attributes_default_color_name(&mesh_src->id); if ((data_type == CD_PROP_COLOR) && - !data_transfer_attributes_color_float_find(&mesh_src->id, default_color_src)) { + !BKE_id_attribute_search(&mesh_src->id, default_color_src, CD_MASK_PROP_COLOR, ATTR_DOMAIN_MASK_COLOR)) { return; } else if ((data_type == CD_PROP_BYTE_COLOR) && - !data_transfer_attributes_color_byte_find(&mesh_src->id, default_color_src)) { + !BKE_id_attribute_search(&mesh_src->id, default_color_src, CD_MASK_PROP_BYTE_COLOR, ATTR_DOMAIN_MASK_COLOR)) { return; } if ((data_type == CD_PROP_COLOR) && - data_transfer_attributes_color_float_find(&mesh_dst->id, default_color_src)) { + BKE_id_attribute_search(&mesh_dst->id, default_color_src, CD_MASK_PROP_COLOR, ATTR_DOMAIN_MASK_COLOR)) { mesh_dst->default_color_attribute = BLI_strdup(default_color_src); } else if ((data_type == CD_PROP_BYTE_COLOR) && - data_transfer_attributes_color_byte_find(&mesh_dst->id, default_color_src)) { + BKE_id_attribute_search(&mesh_dst->id, default_color_src, CD_MASK_PROP_BYTE_COLOR, ATTR_DOMAIN_MASK_COLOR)) { mesh_dst->default_color_attribute = BLI_strdup(default_color_src); } else { -- 2.30.2