Geometry Nodes: Add float solver to mesh boolean node #119294

Howard Trickey · 2024-03-10T20:53:36+01:00

Howard Trickey commented

2024-03-10 20:53:36 +01:00

This adds a "Solver" option to the geo boolean node, with the options Exact and Float.
The current geo boolean node only uses the Exact solver. This adds the ability to use the faster original floating point boolean solver. The float solver has issues with coplanar and other coincident geometry, but is generally much faster than the Exact solver, and users have asked for this option (which is available in the Boolean Modifier and edit mode boolean tool).

Like the modifier, the Float solver needs to convert the Mesh to BMesh, do the operation, and then convert back to Mesh. It also has to do it iteratively if more than two operands are supplied.

This is the first of a planned series of commits that will add a new exact boolean solver, based on the Ember paper, as a solver option. Ember will be much faster than the current exact solver, but may still not be as fast as float, and also will not handle some non-volume-enclosing inputs as well as Float, so it is likely tha the Float solver will always remain. We may eventually retire the old Exact Solver, however.

This commit also prepares for more sensible code in the future by doing two things:

moves the boolean glue code out of blenkernel and into the geometry module
changes the internal enum names for the solvers to better reflect the algorithms used: Fast -> Float, and Exact -> Mesh_Arr (which means "Mesh Arrangments, the name of the paper upon which the current exact solver is based).

This adds a "Solver" option to the geo boolean node, with the options Exact and Float. The current geo boolean node only uses the Exact solver. This adds the ability to use the faster original floating point boolean solver. The float solver has issues with coplanar and other coincident geometry, but is generally much faster than the Exact solver, and users have asked for this option (which is available in the Boolean Modifier and edit mode boolean tool). Like the modifier, the Float solver needs to convert the Mesh to BMesh, do the operation, and then convert back to Mesh. It also has to do it iteratively if more than two operands are supplied. This is the first of a planned series of commits that will add a new exact boolean solver, based on the Ember paper, as a solver option. Ember will be much faster than the current exact solver, but may still not be as fast as float, and also will not handle some non-volume-enclosing inputs as well as Float, so it is likely tha the Float solver will always remain. We may eventually retire the old Exact Solver, however. This commit also prepares for more sensible code in the future by doing two things: - moves the boolean glue code out of blenkernel and into the geometry module - changes the internal enum names for the solvers to better reflect the algorithms used: Fast -> Float, and Exact -> Mesh_Arr (which means "Mesh Arrangments, the name of the paper upon which the current exact solver is based).

👍 9 👀 4 ❤️ 15 🎉 8 🚀 5

Howard Trickey added 6 commits 2024-03-10 20:53:47 +01:00

f23184bcd2 Geometry Nodes: Add float solver to mesh boolean.

Not completely implemented or tested yet.
Also moved mesh boolean stuff from kernel to geometry module.
Also renamed solver enums: Fast ->Float, Exact->Mesh_Arr,
to better reflect the algorithms used and prepare for future
Mesh_Ember solver.

5082ae34fa Fix: tests/data hash differs from main.

76d8a306a1 Make float boolean take 1 or n > 2 args too.

304a5a56b9 Changed some old math functions to C++ ones.

214189fa25 Do material remaps for float boolean.

10d153043e Fixed float boolean: material remap sometimes -1.

Howard Trickey requested review from Hans Goudey 2024-03-10 20:54:36 +01:00

Howard Trickey referenced this pull request

2024-03-10 21:03:02 +01:00

Exact Boolean V2 #114476

Hans Goudey requested changes 2024-03-11 15:25:37 +01:00

Hans Goudey left a comment

Great! I think it would be good to move the existing mesh_boolean_convert.cc (which doesn't seem to be removed here BTW?) to the geometry module first in a separate commit with minimal changes so that git history is preserved. Other than that this PR seems reasonable.

Great! I think it would be good to move the existing `mesh_boolean_convert.cc` (which doesn't seem to be removed here BTW?) to the geometry module first in a separate commit with minimal changes so that git history is preserved. Other than that this PR seems reasonable.

source/blender/geometry/intern/mesh_boolean.cc Outdated

						
				@ -0,0 +919,4 @@

				                             BMLoop *(**r_looptris)[3],

				                             int *r_looptris_tot)

				{

				  const int n = meshes.size();

Hans Goudey commented

2024-03-11 15:19:22 +01:00

Personally I find this single letter variable name hurts readability below, I had to go check what it was set to, compared to seeing meshes.size() or meshes.index_range() below

Personally I find this single letter variable name hurts readability below, I had to go check what it was set to, compared to seeing `meshes.size()` or `meshes.index_range()` below

Howard Trickey commented

2024-03-14 02:41:14 +01:00

changed n to meshes_num and used meshes.index_range() in loops.

HooglyBoogly marked this conversation as resolved

source/blender/geometry/intern/mesh_boolean.cc

						
				@ -0,0 +1051,4 @@

				                                const float4x4 &target_transform,

				                                Span<Array<short>> material_remaps,

				                                int boolean_mode,

				                                Vector<int> * /* r_intersecting_edges */)

Hans Goudey commented

2024-03-11 15:19:48 +01:00

Could just remove this argument i think

Howard Trickey commented

2024-03-14 02:42:27 +01:00

I think I may want to implement the intersecting edges return at some point (if users complain), so would like to leave this as is for now.

HooglyBoogly marked this conversation as resolved

source/blender/geometry/intern/mesh_boolean.cc Outdated

						
				@ -0,0 +1090,4 @@

				    Array<float4x4> two_transforms = {transforms[0], transforms[1]};

				    Array<Array<short>> two_remaps = {material_remaps[0], material_remaps[1]};

				    for (int i = 0; i < nmesh - 1; i++) {

				      BMesh *bm = mesh_bm_concat(two_meshes.as_span(),

Hans Goudey commented

2024-03-11 15:15:43 +01:00

.as_span() should be unnecessary, the conversion from Array to Span can be done implicitly

`.as_span()` should be unnecessary, the conversion from `Array` to `Span` can be done implicitly

Howard Trickey commented

2024-03-14 02:45:21 +01:00

You are right, so I removed the .as_span()'s here. (There is an earlier case of a ? : operation where it was needed).

HooglyBoogly marked this conversation as resolved

source/blender/geometry/intern/mesh_boolean.cc Outdated

						
				@ -0,0 +1116,4 @@

				        /* Except in the first iteration, two_meshes[0] holds the intermediate

				         * mesh result from the previous iteraiton. */

				        Mesh *me = const_cast<Mesh*>(two_meshes[0]);

				        BKE_mesh_eval_delete(me);

Hans Goudey commented

2024-03-11 15:17:24 +01:00

What's going on here with the const cast and BKE_mesh_eval_delete? That seems potentially problematic

What's going on here with the const cast and `BKE_mesh_eval_delete`? That seems potentially problematic

Howard Trickey commented

2024-03-14 02:54:54 +01:00

I found a better way to do what I needed to do without requiring the const_cast. The strange code before was there because I was using two_meshes for mixed purposes.

HooglyBoogly marked this conversation as resolved

source/blender/geometry/intern/mesh_boolean.cc Outdated

						
				@ -0,0 +1140,4 @@

				  return nullptr;

				}

				Mesh *GEO_mesh_boolean(Span<const Mesh *> meshes,

Hans Goudey commented

2024-03-11 15:18:08 +01:00

The GEO_ prefix is a relic of C code with no namespaces. Now this can just be mesh_boolean I guess

The `GEO_` prefix is a relic of C code with no namespaces. Now this can just be `mesh_boolean` I guess

Howard Trickey commented

2024-03-14 02:58:46 +01:00

Thanks. I thought I had seen something in the style guide about doing this, but when I looked again, I couldn't find it.

HooglyBoogly marked this conversation as resolved

source/blender/geometry/intern/mesh_boolean.cc

						
				@ -0,0 +1157,4 @@

				                                material_remaps,

				                                op_params.boolean_mode,

				                                r_intersecting_edges);

				      break;

Hans Goudey commented

2024-03-11 15:14:16 +01:00

break unnecessary after return

`break` unnecessary after `return`

Howard Trickey commented

2024-03-14 03:00:10 +01:00

removed the break

HooglyBoogly marked this conversation as resolved

source/blender/geometry/intern/mesh_boolean.cc Outdated

						
				@ -0,0 +1169,4 @@

				                                   r_intersecting_edges);

				      break;

				    default:

				      BLI_assert(false);

Hans Goudey commented

2024-03-11 15:14:26 +01:00

BLI_assert_unreachable()

`BLI_assert_unreachable()`

Howard Trickey commented

2024-03-14 03:01:09 +01:00

fixed

HooglyBoogly marked this conversation as resolved

source/blender/geometry/intern/mesh_boolean.cc Outdated

						
				@ -0,0 +1171,4 @@

				    default:

				      BLI_assert(false);

				  }

				  return BKE_mesh_new_nomain(0, 0, 0, 0);

Hans Goudey commented

2024-03-11 15:14:43 +01:00

Returning null should be okay here IMO

Howard Trickey commented

2024-03-14 03:02:18 +01:00

sure, changed.

HooglyBoogly marked this conversation as resolved

source/blender/makesdna/DNA_node_types.h Outdated

						
				@ -2660,6 +2660,11 @@ typedef enum GeometryNodeBooleanOperation {

				  GEO_NODE_BOOLEAN_DIFFERENCE = 2,

				} GeometryNodeBooleanOperation;

				typedef enum GeometryNodeBooleanSolver {

Hans Goudey commented

2024-03-11 15:23:00 +01:00

How about defining this in GEO_mesh_boolean.hh in the boolean namespace as:

enum class Solver {
  Float = 0,
  MeshArr = 1,
};

How about defining this in `GEO_mesh_boolean.hh` in the boolean namespace as: ``` enum class Solver { Float = 0, MeshArr = 1, }; ```

Howard Trickey commented

2024-03-14 03:13:08 +01:00

Hmm, somehow I thought everything that was serialized to a .blend file as an enum value needed to be declared in this style in a DNA_.h file. But I guess not. OK, I'll make this change, which I certainly like.

Hmm, somehow I thought everything that was serialized to a .blend file as an enum value needed to be declared in this style in a DNA_<somthing>.h file. But I guess not. OK, I'll make this change, which I certainly like.

Howard Trickey commented

2024-03-14 03:35:08 +01:00

Note that it does require some casts to ints now, but that's OK.

👍 1

HooglyBoogly marked this conversation as resolved

source/blender/modifiers/intern/MOD_boolean.cc

						
				@ -486,3 +481,1 @@

				                                                             hole_tolerant,

				                                                             bmd->operation,

				                                                             nullptr);

				  const GeometryNodeBooleanOperation op = static_cast<GeometryNodeBooleanOperation>(