From 8db322f1f5a8cf059a224f684f33f0174705762b Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Wed, 23 Apr 2025 20:37:53 +0200 Subject: [PATCH] Fix #137902: Manifold boolean modifier solver doubles object transform The object to world transform was applied to the result (which was meant to be in world space), rather than the inverse. However, the processing of transforms is more complicated than necessary. Instead of passing around a separate "target transform" that's meant to be used inverted after the boolean operation, just make the input transforms transform the input meshes into the desired transform space of the output (object-local space for the modifier). Pull Request: https://projects.blender.org/blender/blender/pulls/137919 --- source/blender/geometry/GEO_mesh_boolean.hh | 5 +- .../blender/geometry/intern/mesh_boolean.cc | 49 +++++-------------- .../geometry/intern/mesh_boolean_manifold.cc | 4 -- .../geometry/intern/mesh_boolean_manifold.hh | 1 - .../blender/modifiers/intern/MOD_boolean.cc | 19 +++---- .../nodes/geometry/nodes/node_geo_boolean.cc | 1 - 6 files changed, 21 insertions(+), 58 deletions(-) diff --git a/source/blender/geometry/GEO_mesh_boolean.hh b/source/blender/geometry/GEO_mesh_boolean.hh index 2081f5c0d50..5e2af7f5c5d 100644 --- a/source/blender/geometry/GEO_mesh_boolean.hh +++ b/source/blender/geometry/GEO_mesh_boolean.hh @@ -72,8 +72,8 @@ struct BooleanOpParameters { * in the way the user hopes. * * \param meshes: The meshes that are operands of the boolean operation. - * \param transforms: An array of transform matrices used for each mesh's positions. - * \param target_transform: the result needs to be transformed by this. + * \param transforms: An array of transform matrices used to transform the input meshes to bring + * them into the transform space of the result. * \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. A -1 value means that the original index should be used with no mapping. @@ -85,7 +85,6 @@ struct BooleanOpParameters { */ Mesh *mesh_boolean(Span meshes, Span transforms, - const float4x4 &target_transform, Span> material_remaps, BooleanOpParameters op_params, Solver solver, diff --git a/source/blender/geometry/intern/mesh_boolean.cc b/source/blender/geometry/intern/mesh_boolean.cc index 2bd71e6acf7..ceec606cef6 100644 --- a/source/blender/geometry/intern/mesh_boolean.cc +++ b/source/blender/geometry/intern/mesh_boolean.cc @@ -247,9 +247,8 @@ void MeshesToIMeshInfo::input_medge_for_orig_index(int orig_index, * All allocation of memory for the IMesh comes from `arena`. */ static meshintersect::IMesh meshes_to_imesh(Span meshes, - Span obmats, + Span transforms, Span> material_remaps, - const float4x4 &target_transform, meshintersect::IMeshArena &arena, MeshesToIMeshInfo *r_info) { @@ -295,7 +294,6 @@ static meshintersect::IMesh meshes_to_imesh(Span meshes, * of the target, multiply each transform by the inverse of the * target matrix. Exact Boolean works better if these matrices are 'cleaned' * -- see the comment for the `clean_transform` function, above. */ - const float4x4 inv_target_mat = math::invert(clean_transform(target_transform)); /* For each input `Mesh`, make `Vert`s and `Face`s for the corresponding * vertices and polygons, and keep track of the original indices (using the @@ -309,10 +307,9 @@ static meshintersect::IMesh meshes_to_imesh(Span meshes, r_info->mesh_face_offset[mi] = f; /* Get matrix that transforms a coordinate in meshes[mi]'s local space * to the target space. */ - const float4x4 objn_mat = obmats.is_empty() ? float4x4::identity() : - clean_transform(obmats[mi]); - r_info->to_target_transform[mi] = inv_target_mat * objn_mat; - r_info->has_negative_transform[mi] = math::is_negative(objn_mat); + r_info->to_target_transform[mi] = transforms.is_empty() ? float4x4::identity() : + clean_transform(transforms[mi]); + r_info->has_negative_transform[mi] = math::is_negative(r_info->to_target_transform[mi]); /* All meshes 1 and up will be transformed into the local space of operand 0. * Historical behavior of the modifier has been to flip the faces of any meshes @@ -329,7 +326,7 @@ static meshintersect::IMesh meshes_to_imesh(Span meshes, * Skip the matrix multiplication for each point when there is no transform for a mesh, * for example when the first mesh is already in the target space. (Note the logic * directly above, which uses an identity matrix with an empty input transform). */ - if (obmats.is_empty() || r_info->to_target_transform[mi] == float4x4::identity()) { + if (transforms.is_empty() || r_info->to_target_transform[mi] == float4x4::identity()) { threading::parallel_for(vert_positions.index_range(), 2048, [&](IndexRange range) { for (int i : range) { float3 co = vert_positions[i]; @@ -830,7 +827,6 @@ static meshintersect::BoolOpType operation_to_mesh_arr_mode(const Operation oper static Mesh *mesh_boolean_mesh_arr(Span meshes, Span transforms, - const float4x4 &target_transform, Span> material_remaps, const bool use_self, const bool hole_tolerant, @@ -849,8 +845,7 @@ static Mesh *mesh_boolean_mesh_arr(Span meshes, } MeshesToIMeshInfo mim; meshintersect::IMeshArena arena; - meshintersect::IMesh m_in = meshes_to_imesh( - meshes, transforms, material_remaps, target_transform, arena, &mim); + meshintersect::IMesh m_in = meshes_to_imesh(meshes, transforms, material_remaps, arena, &mim); std::function shape_fn = [&mim](int f) { for (int mi = 0; mi < mim.mesh_face_offset.size() - 1; ++mi) { if (f < mim.mesh_face_offset[mi + 1]) { @@ -925,30 +920,20 @@ static int face_boolean_operand(BMFace *f, void * /*user_data*/) */ static BMesh *mesh_bm_concat(Span meshes, Span transforms, - const float4x4 &target_transform, Span> material_remaps, Array> &r_looptris) { const int meshes_num = meshes.size(); BLI_assert(meshes_num >= 1); - bool ok; - float4x4 inv_target_mat = math::invert(target_transform, ok); - if (!ok) { - BLI_assert_unreachable(); - inv_target_mat = float4x4::identity(); - } - Array to_target(meshes_num); Array is_negative_transform(meshes_num); Array is_flip(meshes_num); const int tsize = transforms.size(); for (const int i : IndexRange(meshes_num)) { if (tsize > i) { - to_target[i] = inv_target_mat * transforms[i]; is_negative_transform[i] = math::is_negative(transforms[i]); is_flip[i] = is_negative_transform[i] != is_negative_transform[0]; } else { - to_target[i] = inv_target_mat; is_negative_transform[i] = false; is_flip[i] = false; } @@ -1011,7 +996,7 @@ static BMesh *mesh_bm_concat(Span meshes, int i = 0; int mesh_index = 0; BM_ITER_MESH (eve, &iter, bm, BM_VERTS_OF_MESH) { - copy_v3_v3(eve->co, math::transform_point(to_target[mesh_index], float3(eve->co))); + copy_v3_v3(eve->co, math::transform_point(transforms[mesh_index], float3(eve->co))); ++i; if (i == verts_end[mesh_index]) { mesh_index++; @@ -1024,7 +1009,7 @@ static BMesh *mesh_bm_concat(Span meshes, i = 0; mesh_index = 0; BM_ITER_MESH (efa, &iter, bm, BM_FACES_OF_MESH) { - copy_v3_v3(efa->no, math::transform_direction(to_target[mesh_index], float3(efa->no))); + copy_v3_v3(efa->no, math::transform_direction(transforms[mesh_index], float3(efa->no))); if (is_negative_transform[mesh_index]) { negate_v3(efa->no); } @@ -1069,7 +1054,6 @@ static int operation_to_float_mode(const Operation operation) static Mesh *mesh_boolean_float(Span meshes, Span transforms, - const float4x4 &target_transform, Span> material_remaps, const int boolean_mode, Vector * /*r_intersecting_edges*/) @@ -1088,7 +1072,7 @@ static Mesh *mesh_boolean_float(Span meshes, Array> looptris; if (meshes.size() == 2) { - BMesh *bm = mesh_bm_concat(meshes, transforms, target_transform, material_remaps, looptris); + BMesh *bm = mesh_bm_concat(meshes, transforms, material_remaps, looptris); BM_mesh_intersect(bm, looptris, face_boolean_operand, @@ -1112,8 +1096,7 @@ static Mesh *mesh_boolean_float(Span meshes, Array> two_remaps = {material_remaps[0], material_remaps[1]}; Mesh *prev_result_mesh = nullptr; for (const int i : meshes.index_range().drop_back(1)) { - BMesh *bm = mesh_bm_concat( - two_meshes, two_transforms, float4x4::identity(), two_remaps, looptris); + BMesh *bm = mesh_bm_concat(two_meshes, two_transforms, two_remaps, looptris); BM_mesh_intersect(bm, looptris, face_boolean_operand, @@ -1191,7 +1174,6 @@ static void write_boolean_benchmark_time( Mesh *mesh_boolean(Span meshes, Span transforms, - const float4x4 &target_transform, Span> material_remaps, BooleanOpParameters op_params, Solver solver, @@ -1207,7 +1189,6 @@ Mesh *mesh_boolean(Span meshes, *r_error = BooleanError::NoError; ans = mesh_boolean_float(meshes, transforms, - target_transform, material_remaps, operation_to_float_mode(op_params.boolean_mode), r_intersecting_edges); @@ -1217,7 +1198,6 @@ Mesh *mesh_boolean(Span meshes, *r_error = BooleanError::NoError; ans = mesh_boolean_mesh_arr(meshes, transforms, - target_transform, material_remaps, !op_params.no_self_intersections, !op_params.watertight, @@ -1229,13 +1209,8 @@ Mesh *mesh_boolean(Span meshes, break; case Solver::Manifold: #ifdef WITH_MANIFOLD - ans = mesh_boolean_manifold(meshes, - transforms, - target_transform, - material_remaps, - op_params, - r_intersecting_edges, - r_error); + ans = mesh_boolean_manifold( + meshes, transforms, material_remaps, op_params, r_intersecting_edges, r_error); #else *r_error = BooleanError::SolverNotAvailable; #endif diff --git a/source/blender/geometry/intern/mesh_boolean_manifold.cc b/source/blender/geometry/intern/mesh_boolean_manifold.cc index 9dfbc1614aa..d74c49f7cb0 100644 --- a/source/blender/geometry/intern/mesh_boolean_manifold.cc +++ b/source/blender/geometry/intern/mesh_boolean_manifold.cc @@ -1639,7 +1639,6 @@ static bke::GeometrySet join_meshes_with_transforms(const Span mes Mesh *mesh_boolean_manifold(Span meshes, const Span transforms, - const float4x4 &target_transform, const Span> /*material_remaps*/, const BooleanOpParameters op_params, Vector *r_intersecting_edges, @@ -1725,9 +1724,6 @@ Mesh *mesh_boolean_manifold(Span meshes, # endif mesh_result = meshgl_to_mesh(meshgl_result, joined_mesh, mesh_offsets, r_intersecting_edges); } - if (!math::is_identity(target_transform)) { - bke::mesh_transform(*mesh_result, target_transform, false); - } return mesh_result; } catch (const std::exception &e) { diff --git a/source/blender/geometry/intern/mesh_boolean_manifold.hh b/source/blender/geometry/intern/mesh_boolean_manifold.hh index 82ee2a1ab5c..0b48572ff6b 100644 --- a/source/blender/geometry/intern/mesh_boolean_manifold.hh +++ b/source/blender/geometry/intern/mesh_boolean_manifold.hh @@ -10,7 +10,6 @@ namespace blender::geometry::boolean { Mesh *mesh_boolean_manifold(Span meshes, Span transforms, - const float4x4 &target_transform, Span> material_remaps, BooleanOpParameters op_params, Vector *r_intersecting_edges, diff --git a/source/blender/modifiers/intern/MOD_boolean.cc b/source/blender/modifiers/intern/MOD_boolean.cc index c1ea3dfda02..01b1ccf1161 100644 --- a/source/blender/modifiers/intern/MOD_boolean.cc +++ b/source/blender/modifiers/intern/MOD_boolean.cc @@ -403,8 +403,9 @@ static Mesh *non_float_boolean_mesh(BooleanModifierData *bmd, const ModifierEvalContext *ctx, Mesh *mesh) { + const float4x4 &world_to_object = ctx->object->world_to_object(); Vector meshes; - Vector obmats; + Vector transforms; Vector> material_remaps; @@ -420,7 +421,7 @@ static Mesh *non_float_boolean_mesh(BooleanModifierData *bmd, blender::geometry::boolean::Solver::MeshArr : blender::geometry::boolean::Solver::Manifold; meshes.append(mesh); - obmats.append(ctx->object->object_to_world()); + transforms.append(float4x4::identity()); material_remaps.append({}); const BooleanModifierMaterialMode material_mode = BooleanModifierMaterialMode( @@ -443,7 +444,7 @@ static Mesh *non_float_boolean_mesh(BooleanModifierData *bmd, } BKE_mesh_wrapper_ensure_mdata(mesh_operand); meshes.append(mesh_operand); - obmats.append(bmd->object->object_to_world()); + transforms.append(world_to_object * bmd->object->object_to_world()); if (material_mode == eBooleanModifierMaterialMode_Index) { material_remaps.append(get_material_remap_index_based(ctx->object, bmd->object)); } @@ -463,7 +464,7 @@ static Mesh *non_float_boolean_mesh(BooleanModifierData *bmd, } BKE_mesh_wrapper_ensure_mdata(collection_mesh); meshes.append(collection_mesh); - obmats.append(ob->object_to_world()); + transforms.append(world_to_object * ob->object_to_world()); if (material_mode == eBooleanModifierMaterialMode_Index) { material_remaps.append(get_material_remap_index_based(ctx->object, ob)); } @@ -485,14 +486,8 @@ static Mesh *non_float_boolean_mesh(BooleanModifierData *bmd, op_params.no_nested_components = false; blender::geometry::boolean::BooleanError error = blender::geometry::boolean::BooleanError::NoError; - Mesh *result = blender::geometry::boolean::mesh_boolean(meshes, - obmats, - ctx->object->object_to_world(), - material_remaps, - op_params, - solver, - nullptr, - &error); + Mesh *result = blender::geometry::boolean::mesh_boolean( + meshes, transforms, material_remaps, op_params, solver, nullptr, &error); if (error != blender::geometry::boolean::BooleanError::NoError) { if (error == blender::geometry::boolean::BooleanError::NonManifold) { diff --git a/source/blender/nodes/geometry/nodes/node_geo_boolean.cc b/source/blender/nodes/geometry/nodes/node_geo_boolean.cc index 155ad4e1a43..2516511b50c 100644 --- a/source/blender/nodes/geometry/nodes/node_geo_boolean.cc +++ b/source/blender/nodes/geometry/nodes/node_geo_boolean.cc @@ -193,7 +193,6 @@ static void node_geo_exec(GeoNodeExecParams params) Mesh *result = geometry::boolean::mesh_boolean( meshes, transforms, - float4x4::identity(), material_remaps, op_params, solver,