From fc06a471f174368bed2b1e0c1fe0ab4f2f90c09a Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Thu, 18 May 2023 15:08:53 -0400 Subject: [PATCH] Geometry Nodes: Only use realized geometry in mesh boolean node Since 44e4f077a9d7b5b609f6 and related commits, geometry nodes doesn't try to hide the difference between real geometry data and instances from the user. Other nodes were updated to only support real geometry, but the "Mesh Boolean" node was never updated and still implicitly gathered all the instances. This commit removes the special instance behavior in the boolean node and adds realize instances nodes to keep existing behavior in most cases. Typically this doesn't make a difference in the result, though it could in the union mode for instance inputs. Shifting more of the work to realizing instances should generally be better for performance, since it's much faster. --- .../blenkernel/BKE_geometry_set_instances.hh | 34 ------- .../intern/geometry_set_instances.cc | 93 ------------------- .../blenkernel/intern/mesh_boolean_convert.cc | 9 +- .../blenloader/intern/versioning_300.cc | 38 -------- .../blenloader/intern/versioning_400.cc | 17 ++++ .../blenloader/intern/versioning_common.cc | 37 ++++++++ .../blenloader/intern/versioning_common.h | 4 + .../nodes/geometry/nodes/node_geo_boolean.cc | 32 +------ 8 files changed, 68 insertions(+), 196 deletions(-) diff --git a/source/blender/blenkernel/BKE_geometry_set_instances.hh b/source/blender/blenkernel/BKE_geometry_set_instances.hh index ec3c9c0d645..244b94d15a7 100644 --- a/source/blender/blenkernel/BKE_geometry_set_instances.hh +++ b/source/blender/blenkernel/BKE_geometry_set_instances.hh @@ -2,8 +2,6 @@ #pragma once -#include "BLI_math_matrix_types.hh" - #include "BKE_geometry_set.hh" namespace blender::bke { @@ -13,36 +11,4 @@ namespace blender::bke { */ GeometrySet object_get_evaluated_geometry_set(const Object &object); -/** - * Used to keep track of a group of instances using the same geometry data. - */ -struct GeometryInstanceGroup { - /** - * The geometry set instanced on each of the transforms. The components are not necessarily - * owned here. For example, they may be owned by the instanced object. This cannot be a - * reference because not all instanced data will necessarily have a #geometry_set_eval. - */ - GeometrySet geometry_set; - - /** - * As an optimization to avoid copying, the same geometry set can be associated with multiple - * instances. Each instance is stored as a transform matrix here. Again, these must be owned - * because they may be transformed from the original data. TODO: Validate that last statement. - */ - Vector transforms; -}; - -/** - * Return flattened vector of the geometry component's recursive instances. I.e. all collection - * instances and object instances will be expanded into the instances of their geometry components. - * Even the instances in those geometry components' will be included. - * - * \note For convenience (to avoid duplication in the caller), the returned vector also contains - * the argument geometry set. - * - * \note This doesn't extract instances from the "dupli" system for non-geometry-nodes instances. - */ -void geometry_set_gather_instances(const GeometrySet &geometry_set, - Vector &r_instance_groups); - } // namespace blender::bke diff --git a/source/blender/blenkernel/intern/geometry_set_instances.cc b/source/blender/blenkernel/intern/geometry_set_instances.cc index e99efc19276..8b1e32230b3 100644 --- a/source/blender/blenkernel/intern/geometry_set_instances.cc +++ b/source/blender/blenkernel/intern/geometry_set_instances.cc @@ -3,28 +3,16 @@ #include "BKE_collection.h" #include "BKE_geometry_set_instances.hh" #include "BKE_instances.hh" -#include "BKE_material.h" #include "BKE_mesh.hh" #include "BKE_mesh_wrapper.h" #include "BKE_modifier.h" -#include "BKE_pointcloud.h" #include "DNA_collection_types.h" #include "DNA_layer_types.h" -#include "DNA_mesh_types.h" -#include "DNA_meshdata_types.h" #include "DNA_object_types.h" namespace blender::bke { -static void geometry_set_collect_recursive(const GeometrySet &geometry_set, - const float4x4 &transform, - Vector &r_sets); - -static void geometry_set_collect_recursive_collection(const Collection &collection, - const float4x4 &transform, - Vector &r_sets); - static void add_final_mesh_as_geometry_component(const Object &object, GeometrySet &geometry_set) { Mesh *mesh = BKE_modifier_get_evaluated_mesh_from_evaluated_object( @@ -74,87 +62,6 @@ GeometrySet object_get_evaluated_geometry_set(const Object &object) return {}; } -static void geometry_set_collect_recursive_collection_instance( - const Collection &collection, const float4x4 &transform, Vector &r_sets) -{ - float4x4 offset_matrix = float4x4::identity(); - offset_matrix.location() -= float3(collection.instance_offset); - const float4x4 instance_transform = transform * offset_matrix; - geometry_set_collect_recursive_collection(collection, instance_transform, r_sets); -} - -static void geometry_set_collect_recursive_object(const Object &object, - const float4x4 &transform, - Vector &r_sets) -{ - GeometrySet instance_geometry_set = object_get_evaluated_geometry_set(object); - geometry_set_collect_recursive(instance_geometry_set, transform, r_sets); -} - -static void geometry_set_collect_recursive_collection(const Collection &collection, - const float4x4 &transform, - Vector &r_sets) -{ - LISTBASE_FOREACH (const CollectionObject *, collection_object, &collection.gobject) { - BLI_assert(collection_object->ob != nullptr); - const Object &object = *collection_object->ob; - const float4x4 object_transform = transform * float4x4_view(object.object_to_world); - geometry_set_collect_recursive_object(object, object_transform, r_sets); - } - LISTBASE_FOREACH (const CollectionChild *, collection_child, &collection.children) { - BLI_assert(collection_child->collection != nullptr); - const Collection &collection = *collection_child->collection; - geometry_set_collect_recursive_collection(collection, transform, r_sets); - } -} - -static void geometry_set_collect_recursive(const GeometrySet &geometry_set, - const float4x4 &transform, - Vector &r_sets) -{ - r_sets.append({geometry_set, {transform}}); - - if (geometry_set.has_instances()) { - const Instances &instances = *geometry_set.get_instances_for_read(); - - Span transforms = instances.transforms(); - Span handles = instances.reference_handles(); - Span references = instances.references(); - for (const int i : transforms.index_range()) { - const InstanceReference &reference = references[handles[i]]; - const float4x4 instance_transform = transform * transforms[i]; - - switch (reference.type()) { - case InstanceReference::Type::Object: { - Object &object = reference.object(); - geometry_set_collect_recursive_object(object, instance_transform, r_sets); - break; - } - case InstanceReference::Type::Collection: { - Collection &collection = reference.collection(); - geometry_set_collect_recursive_collection_instance( - collection, instance_transform, r_sets); - break; - } - case InstanceReference::Type::GeometrySet: { - const GeometrySet &geometry_set = reference.geometry_set(); - geometry_set_collect_recursive(geometry_set, instance_transform, r_sets); - break; - } - case InstanceReference::Type::None: { - break; - } - } - } - } -} - -void geometry_set_gather_instances(const GeometrySet &geometry_set, - Vector &r_instance_groups) -{ - geometry_set_collect_recursive(geometry_set, float4x4::identity(), r_instance_groups); -} - void Instances::foreach_referenced_geometry( blender::FunctionRef callback) const { diff --git a/source/blender/blenkernel/intern/mesh_boolean_convert.cc b/source/blender/blenkernel/intern/mesh_boolean_convert.cc index c00ba6d5008..b4ecbde3540 100644 --- a/source/blender/blenkernel/intern/mesh_boolean_convert.cc +++ b/source/blender/blenkernel/intern/mesh_boolean_convert.cc @@ -291,8 +291,9 @@ static IMesh meshes_to_imesh(Span meshes, r_info->mesh_poly_offset[mi] = f; /* Get matrix that transforms a coordinate in meshes[mi]'s local space * to the target space. */ - const float4x4 objn_mat = (obmats[mi] == nullptr) ? float4x4::identity() : - clean_transform(*obmats[mi]); + const float4x4 objn_mat = (obmats.is_empty() || obmats[mi] == nullptr) ? + 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); @@ -311,7 +312,7 @@ static 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 a null input transform). */ - if (obmats[mi] == nullptr) { + if (obmats.is_empty() || obmats[mi] == nullptr) { threading::parallel_for(vert_positions.index_range(), 2048, [&](IndexRange range) { for (int i : range) { float3 co = vert_positions[i]; @@ -798,7 +799,7 @@ Mesh *direct_mesh_boolean(Span meshes, Vector *r_intersecting_edges) { #ifdef WITH_GMP - BLI_assert(meshes.size() == transforms.size()); + BLI_assert(transforms.is_empty() || meshes.size() == transforms.size()); BLI_assert(material_remaps.size() == 0 || material_remaps.size() == meshes.size()); if (meshes.size() <= 0) { return nullptr; diff --git a/source/blender/blenloader/intern/versioning_300.cc b/source/blender/blenloader/intern/versioning_300.cc index 693b6a668e3..156bd5c3719 100644 --- a/source/blender/blenloader/intern/versioning_300.cc +++ b/source/blender/blenloader/intern/versioning_300.cc @@ -497,44 +497,6 @@ static bool do_versions_sequencer_color_balance_sop(Sequence *seq, void * /*user return true; } -static bNodeLink *find_connected_link(bNodeTree *ntree, bNodeSocket *in_socket) -{ - LISTBASE_FOREACH (bNodeLink *, link, &ntree->links) { - if (link->tosock == in_socket) { - return link; - } - } - return nullptr; -} - -static void add_realize_instances_before_socket(bNodeTree *ntree, - bNode *node, - bNodeSocket *geometry_socket) -{ - BLI_assert(geometry_socket->type == SOCK_GEOMETRY); - bNodeLink *link = find_connected_link(ntree, geometry_socket); - if (link == nullptr) { - return; - } - - /* If the realize instances node is already before this socket, no need to continue. */ - if (link->fromnode->type == GEO_NODE_REALIZE_INSTANCES) { - return; - } - - bNode *realize_node = nodeAddStaticNode(nullptr, ntree, GEO_NODE_REALIZE_INSTANCES); - realize_node->parent = node->parent; - realize_node->locx = node->locx - 100; - realize_node->locy = node->locy; - nodeAddLink(ntree, - link->fromnode, - link->fromsock, - realize_node, - static_cast(realize_node->inputs.first)); - link->fromnode = realize_node; - link->fromsock = static_cast(realize_node->outputs.first); -} - /** * If a node used to realize instances implicitly and will no longer do so in 3.0, add a "Realize * Instances" node in front of it to avoid changing behavior. Don't do this if the node will be diff --git a/source/blender/blenloader/intern/versioning_400.cc b/source/blender/blenloader/intern/versioning_400.cc index 7c2721afec8..e496f172f8e 100644 --- a/source/blender/blenloader/intern/versioning_400.cc +++ b/source/blender/blenloader/intern/versioning_400.cc @@ -12,9 +12,11 @@ #include "BLI_assert.h" #include "BLI_listbase.h" +#include "BLI_set.hh" #include "BKE_main.h" #include "BKE_mesh_legacy_convert.h" +#include "BKE_node.hh" #include "BKE_tracking.h" #include "BLO_readfile.h" @@ -88,6 +90,15 @@ static void version_movieclips_legacy_camera_object(Main *bmain) } } +static void version_geometry_nodes_add_realize_instance_nodes(bNodeTree *ntree) +{ + LISTBASE_FOREACH_MUTABLE (bNode *, node, &ntree->nodes) { + if (STREQ(node->idname, "GeometryNodeMeshBoolean")) { + add_realize_instances_before_socket(ntree, node, nodeFindSocket(node, SOCK_IN, "Mesh 2")); + } + } +} + void blo_do_versions_400(FileData * /*fd*/, Library * /*lib*/, Main *bmain) { if (!MAIN_VERSION_ATLEAST(bmain, 400, 1)) { @@ -108,5 +119,11 @@ void blo_do_versions_400(FileData * /*fd*/, Library * /*lib*/, Main *bmain) */ { /* Keep this block, even when empty. */ + + LISTBASE_FOREACH (bNodeTree *, ntree, &bmain->nodetrees) { + if (ntree->type == NTREE_GEOMETRY) { + version_geometry_nodes_add_realize_instance_nodes(ntree); + } + } } } diff --git a/source/blender/blenloader/intern/versioning_common.cc b/source/blender/blenloader/intern/versioning_common.cc index 50b636356cf..fa6d9899f4c 100644 --- a/source/blender/blenloader/intern/versioning_common.cc +++ b/source/blender/blenloader/intern/versioning_common.cc @@ -286,3 +286,40 @@ void node_tree_relink_with_socket_id_map(bNodeTree &ntree, } } } + +static blender::Vector find_connected_links(bNodeTree *ntree, bNodeSocket *in_socket) +{ + blender::Vector links; + LISTBASE_FOREACH (bNodeLink *, link, &ntree->links) { + if (link->tosock == in_socket) { + links.append(link); + } + } + return links; +} + +void add_realize_instances_before_socket(bNodeTree *ntree, + bNode *node, + bNodeSocket *geometry_socket) +{ + BLI_assert(geometry_socket->type == SOCK_GEOMETRY); + blender::Vector links = find_connected_links(ntree, geometry_socket); + for (bNodeLink *link : links) { + /* If the realize instances node is already before this socket, no need to continue. */ + if (link->fromnode->type == GEO_NODE_REALIZE_INSTANCES) { + return; + } + + bNode *realize_node = nodeAddStaticNode(nullptr, ntree, GEO_NODE_REALIZE_INSTANCES); + realize_node->parent = node->parent; + realize_node->locx = node->locx - 100; + realize_node->locy = node->locy; + nodeAddLink(ntree, + link->fromnode, + link->fromsock, + realize_node, + static_cast(realize_node->inputs.first)); + link->fromnode = realize_node; + link->fromsock = static_cast(realize_node->outputs.first); + } +} diff --git a/source/blender/blenloader/intern/versioning_common.h b/source/blender/blenloader/intern/versioning_common.h index 8c142707fdd..82f382519a2 100644 --- a/source/blender/blenloader/intern/versioning_common.h +++ b/source/blender/blenloader/intern/versioning_common.h @@ -110,6 +110,10 @@ ARegion *do_versions_add_region(int regiontype, const char *name); void sequencer_init_preview_region(ARegion *region); +void add_realize_instances_before_socket(bNodeTree *ntree, + bNode *node, + bNodeSocket *geometry_socket); + #ifdef __cplusplus } #endif diff --git a/source/blender/nodes/geometry/nodes/node_geo_boolean.cc b/source/blender/nodes/geometry/nodes/node_geo_boolean.cc index 6f9842cdb6f..85ea4b1d941 100644 --- a/source/blender/nodes/geometry/nodes/node_geo_boolean.cc +++ b/source/blender/nodes/geometry/nodes/node_geo_boolean.cc @@ -43,12 +43,10 @@ static void node_update(bNodeTree *ntree, bNode *node) case GEO_NODE_BOOLEAN_INTERSECT: case GEO_NODE_BOOLEAN_UNION: bke::nodeSetSocketAvailability(ntree, geometry_1_socket, false); - bke::nodeSetSocketAvailability(ntree, geometry_2_socket, true); node_sock_label(geometry_2_socket, "Mesh"); break; case GEO_NODE_BOOLEAN_DIFFERENCE: bke::nodeSetSocketAvailability(ntree, geometry_1_socket, true); - bke::nodeSetSocketAvailability(ntree, geometry_2_socket, true); node_sock_label(geometry_2_socket, "Mesh 2"); break; } @@ -67,8 +65,6 @@ static void node_geo_exec(GeoNodeExecParams params) const bool hole_tolerant = params.get_input("Hole Tolerant"); Vector meshes; - Vector transforms; - VectorSet materials; Vector> material_remaps; @@ -78,10 +74,8 @@ static void node_geo_exec(GeoNodeExecParams params) /* Note that it technically wouldn't be necessary to realize the instances for the first * geometry input, but the boolean code expects the first shape for the difference operation * to be a single mesh. */ - const Mesh *mesh_in_a = set_a.get_mesh_for_read(); - if (mesh_in_a != nullptr) { + if (const Mesh *mesh_in_a = set_a.get_mesh_for_read()) { meshes.append(mesh_in_a); - transforms.append(nullptr); if (mesh_in_a->totcol == 0) { /* Necessary for faces using the default material when there are no material slots. */ materials.add(nullptr); @@ -93,17 +87,11 @@ static void node_geo_exec(GeoNodeExecParams params) } } - /* The instance transform matrices are owned by the instance group, so we have to - * keep all of them around for use during the boolean operation. */ - Vector set_groups; Vector geometry_sets = params.extract_input>("Mesh 2"); - for (const GeometrySet &geometry_set : geometry_sets) { - 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 (const GeometrySet &geometry : geometry_sets) { + if (const Mesh *mesh = geometry.get_mesh_for_read()) { + meshes.append(mesh); Array map(mesh->totcol); for (const int i : IndexRange(mesh->totcol)) { Material *material = mesh->mat[i]; @@ -113,16 +101,6 @@ static void node_geo_exec(GeoNodeExecParams params) } } - for (const bke::GeometryInstanceGroup &set_group : set_groups) { - const Mesh *mesh_in = set_group.geometry_set.get_mesh_for_read(); - if (mesh_in != nullptr) { - meshes.append_n_times(mesh_in, set_group.transforms.size()); - for (const int i : set_group.transforms.index_range()) { - transforms.append(set_group.transforms.begin() + i); - } - } - } - AttributeOutputs attribute_outputs; attribute_outputs.intersecting_edges_id = params.get_output_anonymous_attribute_id_if_needed( "Intersecting Edges"); @@ -130,7 +108,7 @@ static void node_geo_exec(GeoNodeExecParams params) Vector intersecting_edges; Mesh *result = blender::meshintersect::direct_mesh_boolean( meshes, - transforms, + {}, float4x4::identity(), material_remaps, use_self,