From 7540842ca7632dad4c2806989027a3a6b2a15668 Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Mon, 28 Nov 2022 12:42:08 -0600 Subject: [PATCH] Fix T99592: Exact Boolean: Skip empty materials, add index-based option **Empty Slot Fix** Currently the boolean modifier transfers the default material from meshes with no materials and empty material slots to the faces on the base mesh. I added this in a2d59b2dac9e for the sake of consistency, but the behavior is actually not useful at all. The default empty material isn't chosen by users, it just signifies "nothing," so when it replaces a material chosen by users, it feels like a bug. This commit corrects that behavior by only transferring materials from non-empty material slots. The implementation is now consistent between exact mode of the boolean modifier and the geometry node. **Index-Based Option** "Index-based" is the new default material method for the boolean modifier, to access the old behavior from before the breaking commit. a2d59b2dac9e actually broke some Boolean workflows fundamentally, since it was important to set up matching slot indices on each operand. That isn't the cleanest workflow, and it breaks when materials change procedurally, but historically that hasn't been a problem. The "transfer" behavior transfers all materials except for empty slots, but the fundamental problem is that there isn't a good way to specify the result materials besides using the slot indices. Even then, the transfer option is a bit more intuitive and useful for some simpler situations, and it allows accessing the behavior that has been in 3.2 and 3.3 for a long time, so it's also left in as an option. The geometry node doesn't get this new option, in the hope that we'll find a better solution in the future. Differential Revision: https://developer.blender.org/D16187 --- .../blenkernel/BKE_mesh_boolean_convert.hh | 2 +- .../blenkernel/intern/mesh_boolean_convert.cc | 6 +- source/blender/makesdna/DNA_modifier_types.h | 8 +++ source/blender/makesrna/intern/rna_modifier.c | 24 +++++++ .../blender/modifiers/intern/MOD_boolean.cc | 72 +++++++++++++------ .../nodes/geometry/nodes/node_geo_boolean.cc | 19 +++-- 6 files changed, 96 insertions(+), 35 deletions(-) diff --git a/source/blender/blenkernel/BKE_mesh_boolean_convert.hh b/source/blender/blenkernel/BKE_mesh_boolean_convert.hh index 441783d46a1..77c90716d42 100644 --- a/source/blender/blenkernel/BKE_mesh_boolean_convert.hh +++ b/source/blender/blenkernel/BKE_mesh_boolean_convert.hh @@ -22,7 +22,7 @@ namespace blender::meshintersect { * It is allowed for the pointers to be null, meaning the transformation is the identity. * \param material_remaps: An array of maps from material slot numbers in the corresponding mesh * to the material slot in the first mesh. It is OK for material_remaps or any of its constituent - * arrays to be empty. + * arrays to be empty. A -1 value means that the original index should be used with no mapping. * \param r_intersecting_edges: Array to store indices of edges on the resulting mesh in. These * 'new' edges are the result of the intersections. */ diff --git a/source/blender/blenkernel/intern/mesh_boolean_convert.cc b/source/blender/blenkernel/intern/mesh_boolean_convert.cc index 21d9baf7f7e..1252e90e11c 100644 --- a/source/blender/blenkernel/intern/mesh_boolean_convert.cc +++ b/source/blender/blenkernel/intern/mesh_boolean_convert.cc @@ -430,12 +430,14 @@ static void copy_poly_attributes(Mesh *dest_mesh, const VArray src_material_indices = orig_me->attributes().lookup_or_default( "material_index", ATTR_DOMAIN_FACE, 0); const int src_index = src_material_indices[index_in_orig_me]; - if (material_remap.size() > 0 && material_remap.index_range().contains(src_index)) { - dst_material_indices[mp_index] = material_remap[src_index]; + if (material_remap.index_range().contains(src_index)) { + const int remapped_index = material_remap[src_index]; + dst_material_indices[mp_index] = remapped_index >= 0 ? remapped_index : src_index; } else { dst_material_indices[mp_index] = src_index; } + BLI_assert(dst_material_indices[mp_index] >= 0); } /* Similar to copy_vert_attributes but for edge attributes. */ diff --git a/source/blender/makesdna/DNA_modifier_types.h b/source/blender/makesdna/DNA_modifier_types.h index c4180071352..97e42efd5ac 100644 --- a/source/blender/makesdna/DNA_modifier_types.h +++ b/source/blender/makesdna/DNA_modifier_types.h @@ -899,10 +899,18 @@ typedef struct BooleanModifierData { float double_threshold; char operation; char solver; + /** #BooleanModifierMaterialMode. */ + char material_mode; char flag; char bm_flag; + char _pad[7]; } BooleanModifierData; +typedef enum BooleanModifierMaterialMode { + eBooleanModifierMaterialMode_Index = 0, + eBooleanModifierMaterialMode_Transfer = 1, +} BooleanModifierMaterialMode; + /** #BooleanModifierData.operation */ typedef enum { eBooleanModifierOp_Intersect = 0, diff --git a/source/blender/makesrna/intern/rna_modifier.c b/source/blender/makesrna/intern/rna_modifier.c index c5da15003e1..edcf07b5336 100644 --- a/source/blender/makesrna/intern/rna_modifier.c +++ b/source/blender/makesrna/intern/rna_modifier.c @@ -2760,6 +2760,24 @@ static void rna_def_modifier_boolean(BlenderRNA *brna) {0, NULL, 0, NULL, NULL}, }; + static const EnumPropertyItem material_mode_items[] = { + {eBooleanModifierMaterialMode_Index, + "INDEX", + 0, + "Index Based", + "Set the material on new faces based on the order of the material slot lists. If a " + "material doesn't exist on the modifier object, the face will use the same material slot " + "or the first if the object doesn't have enough slots"}, + {eBooleanModifierMaterialMode_Transfer, + "TRANSFER", + 0, + "Transfer", + "Transfer materials from non-empty slots to the result mesh, adding new materials as " + "necessary. For empty slots, fall back to using the same material index as the operand " + "mesh"}, + {0, NULL, 0, NULL, NULL}, + }; + srna = RNA_def_struct(brna, "BooleanModifier", "Modifier"); RNA_def_struct_ui_text(srna, "Boolean Modifier", "Boolean operations modifier"); RNA_def_struct_sdna(srna, "BooleanModifierData"); @@ -2819,6 +2837,12 @@ static void rna_def_modifier_boolean(BlenderRNA *brna) RNA_def_property_ui_text(prop, "Hole Tolerant", "Better results when there are holes (slower)"); RNA_def_property_update(prop, 0, "rna_Modifier_update"); + prop = RNA_def_property(srna, "material_mode", PROP_ENUM, PROP_NONE); + RNA_def_property_enum_items(prop, material_mode_items); + RNA_def_property_enum_default(prop, eBooleanModifierMaterialMode_Index); + RNA_def_property_ui_text(prop, "Material Mode", "Method for setting materials on the new faces"); + RNA_def_property_update(prop, 0, "rna_Modifier_update"); + /* BMesh debugging options, only used when G_DEBUG is set */ /* BMesh intersection options */ diff --git a/source/blender/modifiers/intern/MOD_boolean.cc b/source/blender/modifiers/intern/MOD_boolean.cc index 21f05158e8b..178be2d8962 100644 --- a/source/blender/modifiers/intern/MOD_boolean.cc +++ b/source/blender/modifiers/intern/MOD_boolean.cc @@ -377,21 +377,31 @@ static void BMD_mesh_intersection(BMesh *bm, #ifdef WITH_GMP +/* Get a mapping from material slot numbers in the src_ob to slot numbers in the dst_ob. + * If a material doesn't exist in the dst_ob, the mapping just goes to the same slot + * or to zero if there aren't enough slots in the destination. */ +static Array get_material_remap_index_based(Object *dest_ob, Object *src_ob) +{ + int n = src_ob->totcol; + if (n <= 0) { + n = 1; + } + Array remap(n); + BKE_object_material_remap_calc(dest_ob, src_ob, remap.data()); + return remap; +} + /* Get a mapping from material slot numbers in the source geometry to slot numbers in the result * geometry. The material is added to the result geometry if it doesn't already use it. */ -static Array get_material_remap(Object &object, - const Mesh &mesh, - VectorSet &materials) +static Array get_material_remap_transfer(Object &object, + const Mesh &mesh, + VectorSet &materials) { const int material_num = mesh.totcol; - if (material_num == 0) { - /* Necessary for faces using the default material when there are no material slots. */ - return Array({materials.index_of_or_add(nullptr)}); - } Array map(material_num); for (const int i : IndexRange(material_num)) { Material *material = BKE_object_material_get_eval(&object, i + 1); - map[i] = materials.index_of_or_add(material); + map[i] = material ? materials.index_of_or_add(material) : -1; } return map; } @@ -403,7 +413,6 @@ static Mesh *exact_boolean_mesh(BooleanModifierData *bmd, Vector meshes; Vector obmats; - VectorSet materials; Vector> material_remaps; # ifdef DEBUG_TIME @@ -417,12 +426,18 @@ static Mesh *exact_boolean_mesh(BooleanModifierData *bmd, meshes.append(mesh); obmats.append((float4x4 *)&ctx->object->object_to_world); material_remaps.append({}); - if (mesh->totcol == 0) { - /* Necessary for faces using the default material when there are no material slots. */ - materials.add(nullptr); - } - else { - materials.add_multiple({mesh->mat, mesh->totcol}); + + const BooleanModifierMaterialMode material_mode = BooleanModifierMaterialMode( + bmd->material_mode); + VectorSet materials; + if (material_mode == eBooleanModifierMaterialMode_Transfer) { + if (mesh->totcol == 0) { + /* Necessary for faces using the default material when there are no material slots. */ + materials.add(nullptr); + } + else { + materials.add_multiple({mesh->mat, mesh->totcol}); + } } if (bmd->flag & eBooleanModifierFlag_Object) { @@ -433,7 +448,12 @@ static Mesh *exact_boolean_mesh(BooleanModifierData *bmd, BKE_mesh_wrapper_ensure_mdata(mesh_operand); meshes.append(mesh_operand); obmats.append((float4x4 *)&bmd->object->object_to_world); - material_remaps.append(get_material_remap(*bmd->object, *mesh_operand, materials)); + if (material_mode == eBooleanModifierMaterialMode_Index) { + material_remaps.append(get_material_remap_index_based(ctx->object, bmd->object)); + } + else { + material_remaps.append(get_material_remap_transfer(*bmd->object, *mesh_operand, materials)); + } } else if (bmd->flag & eBooleanModifierFlag_Collection) { Collection *collection = bmd->collection; @@ -448,7 +468,12 @@ static Mesh *exact_boolean_mesh(BooleanModifierData *bmd, BKE_mesh_wrapper_ensure_mdata(collection_mesh); meshes.append(collection_mesh); obmats.append((float4x4 *)&ob->object_to_world); - material_remaps.append(get_material_remap(*ob, *collection_mesh, materials)); + if (material_mode == eBooleanModifierMaterialMode_Index) { + material_remaps.append(get_material_remap_index_based(ctx->object, ob)); + } + else { + material_remaps.append(get_material_remap_transfer(*ob, *collection_mesh, materials)); + } } } FOREACH_COLLECTION_OBJECT_RECURSIVE_END; @@ -466,10 +491,14 @@ static Mesh *exact_boolean_mesh(BooleanModifierData *bmd, hole_tolerant, bmd->operation, nullptr); - MEM_SAFE_FREE(result->mat); - result->mat = (Material **)MEM_malloc_arrayN(materials.size(), sizeof(Material *), __func__); - result->totcol = materials.size(); - MutableSpan(result->mat, result->totcol).copy_from(materials); + + if (material_mode == eBooleanModifierMaterialMode_Transfer) { + MEM_SAFE_FREE(result->mat); + result->mat = (Material **)MEM_malloc_arrayN(materials.size(), sizeof(Material *), __func__); + result->totcol = materials.size(); + MutableSpan(result->mat, result->totcol).copy_from(materials); + } + return result; } #endif @@ -610,6 +639,7 @@ static void solver_options_panel_draw(const bContext * /*C*/, Panel *panel) uiLayout *col = uiLayoutColumn(layout, true); if (use_exact) { + uiItemR(col, ptr, "material_mode", 0, IFACE_("Materials"), ICON_NONE); /* When operand is collection, we always use_self. */ if (RNA_enum_get(ptr, "operand_type") == eBooleanModifierFlag_Object) { uiItemR(col, ptr, "use_self", 0, nullptr, ICON_NONE); diff --git a/source/blender/nodes/geometry/nodes/node_geo_boolean.cc b/source/blender/nodes/geometry/nodes/node_geo_boolean.cc index 61780ee25bb..dcae073acdd 100644 --- a/source/blender/nodes/geometry/nodes/node_geo_boolean.cc +++ b/source/blender/nodes/geometry/nodes/node_geo_boolean.cc @@ -83,8 +83,12 @@ static void node_geo_exec(GeoNodeExecParams params) if (mesh_in_a != nullptr) { meshes.append(mesh_in_a); transforms.append(nullptr); - for (Material *material : Span(mesh_in_a->mat, mesh_in_a->totcol)) { - materials.add(material); + if (mesh_in_a->totcol == 0) { + /* Necessary for faces using the default material when there are no material slots. */ + materials.add(nullptr); + } + else { + materials.add_multiple({mesh_in_a->mat, mesh_in_a->totcol}); } material_remaps.append({}); } @@ -98,20 +102,13 @@ static void node_geo_exec(GeoNodeExecParams params) bke::geometry_set_gather_instances(geometry_set, set_groups); } - for (const bke::GeometryInstanceGroup &set_group : set_groups) { - const Mesh *mesh = set_group.geometry_set.get_mesh_for_read(); - if (mesh != nullptr) { - for (Material *material : Span(mesh->mat, mesh->totcol)) { - materials.add(material); - } - } - } for (const bke::GeometryInstanceGroup &set_group : set_groups) { const Mesh *mesh = set_group.geometry_set.get_mesh_for_read(); if (mesh != nullptr) { Array map(mesh->totcol); for (const int i : IndexRange(mesh->totcol)) { - map[i] = materials.index_of(mesh->mat[i]); + Material *material = mesh->mat[i]; + map[i] = material ? materials.index_of_or_add(material) : -1; } material_remaps.append(std::move(map)); }