Changes based on Review by Hans Goudey (HooglyBoogly)soc-2021-porting-modifiers-to-nodes-merge-by-distance

8 files changed, 127 insertions, 135 deletions

diff --git a/source/blender/blenlib/BLI_multi_value_map.hh b/source/blender/blenlib/BLI_multi_value_map.hh
index d3073c98417..3f1f8333ed3 100644
--- a/source/blender/blenlib/BLI_multi_value_map.hh
+++ b/source/blender/blenlib/BLI_multi_value_map.hh
@@ -145,6 +145,14 @@ template<typename Key, typename Value> class MultiValueMap {
   }
 
   /**
+   * Return how many keys are currently stored in the Map.
+   */
+  int64_t size() const
+  {
+    return std::distance(this->keys().begin(), this->keys().end());
+  }
+
+  /**
    * NOTE: This signature will change when the implementation changes.
    */
   typename MapType::ValueIterator values() const
diff --git a/source/blender/geometry/GEO_mesh_merge_by_distance.hh b/source/blender/geometry/GEO_mesh_merge_by_distance.hh
index 3bf32b01e2e..01a40d88e33 100644
--- a/source/blender/geometry/GEO_mesh_merge_by_distance.hh
+++ b/source/blender/geometry/GEO_mesh_merge_by_distance.hh
@@ -24,15 +24,11 @@
 
 namespace blender::geometry {
 
-/* Weld mode is only used when welding mesh data. */
 enum class WeldMode {
   all = 0,
   connected = 1,
 };
 
-WeldMode weld_mode_from_int(const int16_t type);
-int16_t weld_mode_to_int(const WeldMode weld_mode);
-
 struct Mesh *mesh_merge_by_distance(struct Mesh *mesh,
                                     const Span<bool> mask,
                                     const float merge_distance,
diff --git a/source/blender/geometry/intern/mesh_merge_by_distance.cc b/source/blender/geometry/intern/mesh_merge_by_distance.cc
index 9eb2fe1afd2..5b98bbd2b87 100644
--- a/source/blender/geometry/intern/mesh_merge_by_distance.cc
+++ b/source/blender/geometry/intern/mesh_merge_by_distance.cc
@@ -1542,31 +1542,6 @@ struct WeldVertexCluster {
 
 namespace blender::geometry {
 
-WeldMode weld_mode_from_int(const short type)
-{
-  switch (static_cast<WeldMode>(type)) {
-    case WeldMode::all:
-      return WeldMode::all;
-    case WeldMode::connected:
-      return WeldMode::connected;
-  }
-  BLI_assert_unreachable();
-  return WeldMode::all;
-}
-
-int16_t weld_mode_to_int(const WeldMode weld_mode)
-{
-  switch (weld_mode) {
-    case WeldMode::all:
-      return static_cast<int16_t>(WeldMode::all);
-    case WeldMode::connected:
-      return static_cast<int16_t>(WeldMode::connected);
-  }
-
-  BLI_assert_unreachable();
-  return static_cast<int16_t>(WeldMode::all);
-}
-
 Mesh *mesh_merge_by_distance(Mesh *mesh,
                              const Span<bool> mask,
                              const float merge_distance,
diff --git a/source/blender/geometry/intern/pointcloud_merge_by_distance.cc b/source/blender/geometry/intern/pointcloud_merge_by_distance.cc
index 6e5e18980fb..b7540f250e1 100644
--- a/source/blender/geometry/intern/pointcloud_merge_by_distance.cc
+++ b/source/blender/geometry/intern/pointcloud_merge_by_distance.cc
@@ -34,6 +34,7 @@
 using blender::MutableSpan;
 
 namespace blender::geometry {
+const static int POINT_NOT_MERGED = -1;
 
 static KDTree_3d *build_kdtree(Span<float3> positions, Span<bool> selection)
 {
@@ -76,8 +77,8 @@ static void build_merge_map(Span<float3> positions,
           CallbackData &callback_data = *static_cast<CallbackData *>(user_data);
           int target_point_index = callback_data.index;
           if (source_point_index != target_point_index &&
-              callback_data.merge_map[source_point_index] == -1 &&
-              callback_data.merge_map[target_point_index] == -1 &&
+              callback_data.merge_map[source_point_index] == POINT_NOT_MERGED &&
+              callback_data.merge_map[target_point_index] == POINT_NOT_MERGED &&
               callback_data.selection[source_point_index] &&
               callback_data.selection[target_point_index]) {
             callback_data.merge_map[source_point_index] = target_point_index;
@@ -95,80 +96,58 @@ PointCloud *pointcloud_merge_by_distance(PointCloudComponent &pointcloud_compone
                                          Span<bool> selection)
 {
   const PointCloud &src_pointcloud = *pointcloud_component.get_for_read();
-  Array<int> merge_map(src_pointcloud.totpoint, -1);
+  Array<int> merge_map(src_pointcloud.totpoint, POINT_NOT_MERGED);
   Span<float3> positions((const float3 *)src_pointcloud.co, src_pointcloud.totpoint);
 
   build_merge_map(positions, merge_map, merge_threshold, selection);
 
   MultiValueMap<int, int> copy_map;
   for (const int i : merge_map.index_range()) {
-    if (merge_map[i] != -1) {
+    if (merge_map[i] != POINT_NOT_MERGED) {
       copy_map.add(merge_map[i], i);
     }
   }
 
-  int copy_count = std::distance(copy_map.keys().begin(), copy_map.keys().end());
-
-  PointCloud *pointcloud = BKE_pointcloud_new_nomain(copy_count);
+  PointCloud *pointcloud = BKE_pointcloud_new_nomain(copy_map.size());
   PointCloudComponent dst_component;
   dst_component.replace(pointcloud, GeometryOwnershipType::Editable);
 
-  int index_new = 0;
-  for (const int index_old : copy_map.keys()) {
-    Span<int> merged_points = copy_map.lookup(index_old);
-    if (merged_points.size() > 0) {
-      copy_v3_v3(pointcloud->co[index_new], src_pointcloud.co[index_old]);
-      pointcloud->radius[index_new] = src_pointcloud.radius[index_old];
-      float weight = 1 / (float(merged_points.size() + 1));
-      for (const int j : merged_points) {
-        add_v3_v3(pointcloud->co[index_new], src_pointcloud.co[j]);
-        pointcloud->radius[index_new] += src_pointcloud.radius[j];
-      }
-      mul_v3_fl(pointcloud->co[index_new], weight);
-      pointcloud->radius[index_new] *= weight;
-    }
-    index_new++;
-  }
-
   pointcloud_component.attribute_foreach(
       [&](const bke::AttributeIDRef &attribute_id, const AttributeMetaData &meta_data) {
         fn::GVArrayPtr read_attribute = pointcloud_component.attribute_get_for_read(
             attribute_id, meta_data.domain, meta_data.data_type);
 
-        if (dst_component.attribute_try_create(
+        if (!dst_component.attribute_exists(attribute_id) &&
+            !dst_component.attribute_try_create(
                 attribute_id, meta_data.domain, meta_data.data_type, AttributeInitDefault())) {
+          return true;
+        }
 
-          bke::OutputAttribute target_attribute = dst_component.attribute_try_get_for_output_only(
-              attribute_id, meta_data.domain, meta_data.data_type);
+        bke::OutputAttribute target_attribute = dst_component.attribute_try_get_for_output_only(
+            attribute_id, meta_data.domain, meta_data.data_type);
 
-          if (target_attribute->is_empty()) {
-            target_attribute.save();
-            return true;
-          }
+        blender::attribute_math::convert_to_static_type(meta_data.data_type, [&](auto dummy) {
+          using T = decltype(dummy);
+          const fn::GVArray_Typed<T> src_span = read_attribute->typed<T>();
+
+          attribute_math::DefaultMixer<T> mixer(target_attribute.as_span<T>());
 
-          blender::attribute_math::convert_to_static_type(meta_data.data_type, [&](auto dummy) {
-            using T = decltype(dummy);
-            const fn::GVArray_Typed<T> src_span = read_attribute->typed<T>();
-
-            attribute_math::DefaultMixer<T> mixer(target_attribute.as_span<T>());
-
-            index_new = 0;
-            for (const int index_old : copy_map.keys()) {
-              Span<int> merged_points = copy_map.lookup(index_old);
-              if (merged_points.size() > 0) {
-                float weight = 1 / (float(merged_points.size() + 1));
-                mixer.mix_in(index_new, src_span[index_old], weight);
-                for (const int j : merged_points) {
-                  mixer.mix_in(index_new, src_span[j], weight);
-                }
+          int index_new = 0;
+          for (const int index_old : copy_map.keys()) {
+            Span<int> merged_points = copy_map.lookup(index_old);
+            if (merged_points.size() > 0) {
+              float weight = 1.0f / (float(merged_points.size() + 1.0f));
+              mixer.mix_in(index_new, src_span[index_old], weight);
+              for (const int j : merged_points) {
+                mixer.mix_in(index_new, src_span[j], weight);
               }
-              index_new++;
             }
-            mixer.finalize();
-          });
+            index_new++;
+          }
+          mixer.finalize();
+        });
 
-          target_attribute.save();
-        }
+        target_attribute.save();
         return true;
       });
 
diff --git a/source/blender/makesrna/intern/rna_nodetree.c b/source/blender/makesrna/intern/rna_nodetree.c
index 684ff182988..6556eaa8b50 100644
--- a/source/blender/makesrna/intern/rna_nodetree.c
+++ b/source/blender/makesrna/intern/rna_nodetree.c
@@ -10754,21 +10754,21 @@ static void def_geo_string_to_curves(StructRNA *srna)
   RNA_def_property_update(prop, NC_NODE | NA_EDITED, "rna_Node_update");
 }
 
-/* Weld mode is only used when welding mesh data. */
-const EnumPropertyItem rna_enum_geometry_nodes_weld_mode_items[] = {
-    {0, "ALL", 0, "All", "Full merge by distance"},
-    {1, "CONNECTED", 0, "Connected", "Only merge along the edges"},
-    {0, NULL, 0, NULL, NULL},
-};
-
 static void def_geo_merge_by_distance(StructRNA *srna)
 {
+  static const EnumPropertyItem rna_enum_geometry_nodes_weld_mode_items[] = {
+      {0, "ALL", 0, "All", "Full merge by distance."},
+      {1, "CONNECTED", 0, "Connected", "Only merge along the edges"},
+      {0, NULL, 0, NULL, NULL},
+  };
+
   PropertyRNA *prop;
 
   prop = RNA_def_property(srna, "merge_mode", PROP_ENUM, PROP_NONE);
   RNA_def_property_enum_sdna(prop, NULL, "custom1");
   RNA_def_property_enum_items(prop, rna_enum_geometry_nodes_weld_mode_items);
-  RNA_def_property_ui_text(prop, "Mode", "Mode defines the merge rule");
+  RNA_def_property_ui_text(
+      prop, "Mode", "Mode defines the merge rule. Only used when welding mesh data.");
   RNA_def_property_update(prop, NC_NODE | NA_EDITED, "rna_Node_update");
 }
 /* -------------------------------------------------------------------------- */
diff --git a/source/blender/modifiers/CMakeLists.txt b/source/blender/modifiers/CMakeLists.txt
index 0a1f3918aaa..75045263b1d 100644
--- a/source/blender/modifiers/CMakeLists.txt
+++ b/source/blender/modifiers/CMakeLists.txt
@@ -114,7 +114,7 @@ set(SRC
   intern/MOD_weightvgedit.c
   intern/MOD_weightvgmix.c
   intern/MOD_weightvgproximity.c
-    intern/MOD_weld.cc
+  intern/MOD_weld.cc
   intern/MOD_wireframe.c
 
   MOD_modifiertypes.h
diff --git a/source/blender/modifiers/intern/MOD_weld.cc b/source/blender/modifiers/intern/MOD_weld.cc
index 888ab7709ee..5035ab3e98b 100644
--- a/source/blender/modifiers/intern/MOD_weld.cc
+++ b/source/blender/modifiers/intern/MOD_weld.cc
@@ -62,6 +62,32 @@
 #include "RNA_access.h"
 
 #include "MOD_ui_common.h"
+using blender::geometry::WeldMode;
+
+static WeldMode weld_mode_from_int(const short type)
+{
+  switch (static_cast<WeldMode>(type)) {
+    case WeldMode::all:
+      return WeldMode::all;
+    case WeldMode::connected:
+      return WeldMode::connected;
+  }
+  BLI_assert_unreachable();
+  return WeldMode::all;
+}
+
+static int16_t weld_mode_to_int(const WeldMode weld_mode)
+{
+  switch (weld_mode) {
+    case WeldMode::all:
+      return static_cast<int16_t>(WeldMode::all);
+    case WeldMode::connected:
+      return static_cast<int16_t>(WeldMode::connected);
+  }
+
+  BLI_assert_unreachable();
+  return static_cast<int16_t>(WeldMode::all);
+}
 
 static Mesh *modifyMesh(ModifierData *md, const ModifierEvalContext *UNUSED(ctx), Mesh *mesh)
 {
@@ -89,7 +115,7 @@ static Mesh *modifyMesh(ModifierData *md, const ModifierEvalContext *UNUSED(ctx)
   }
 
   Mesh *result = blender::geometry::mesh_merge_by_distance(
-      mesh, mask, wmd->merge_dist, blender::geometry::weld_mode_from_int(wmd->mode));
+      mesh, mask, wmd->merge_dist, weld_mode_from_int(wmd->mode));
 
   return result;
 }
@@ -125,12 +151,12 @@ static void panel_draw(const bContext *UNUSED(C), Panel *panel)
 
   uiLayoutSetPropSep(layout, true);
 
-  uiItemR(layout, ptr, "mode", 0, NULL, ICON_NONE);
+  uiItemR(layout, ptr, "mode", 0, nullptr, ICON_NONE);
   uiItemR(layout, ptr, "merge_threshold", 0, IFACE_("Distance"), ICON_NONE);
-  if (blender::geometry::weld_mode_from_int(weld_mode) == blender::geometry::WeldMode::connected) {
-    uiItemR(layout, ptr, "loose_edges", 0, NULL, ICON_NONE);
+  if (weld_mode_from_int(weld_mode) == blender::geometry::WeldMode::connected) {
+    uiItemR(layout, ptr, "loose_edges", 0, nullptr, ICON_NONE);
   }
-  modifier_vgroup_ui(layout, ptr, &ob_ptr, "vertex_group", "invert_vertex_group", NULL);
+  modifier_vgroup_ui(layout, ptr, &ob_ptr, "vertex_group", "invert_vertex_group", nullptr);
 
   modifier_panel_end(layout, ptr);
 }
@@ -154,27 +180,27 @@ ModifierTypeInfo modifierType_Weld = {
 
     /* copyData */ BKE_modifier_copydata_generic,
 
-    /* deformVerts */ NULL,
-    /* deformMatrices */ NULL,
-    /* deformVertsEM */ NULL,
-    /* deformMatricesEM */ NULL,
+    /* deformVerts */ nullptr,
+    /* deformMatrices */ nullptr,
+    /* deformVertsEM */ nullptr,
+    /* deformMatricesEM */ nullptr,
     /* modifyMesh */ modifyMesh,
-    /* modifyHair */ NULL,
-    /* modifyGeometrySet */ NULL,
+    /* modifyHair */ nullptr,
+    /* modifyGeometrySet */ nullptr,
 
     /* initData */ initData,
     /* requiredDataMask */ requiredDataMask,
-    /* freeData */ NULL,
-    /* isDisabled */ NULL,
-    /* updateDepsgraph */ NULL,
-    /* dependsOnTime */ NULL,
-    /* dependsOnNormals */ NULL,
-    /* foreachIDLink */ NULL,
-    /* foreachTexLink */ NULL,
-    /* freeRuntimeData */ NULL,
+    /* freeData */ nullptr,
+    /* isDisabled */ nullptr,
+    /* updateDepsgraph */ nullptr,
+    /* dependsOnTime */ nullptr,
+    /* dependsOnNormals */ nullptr,
+    /* foreachIDLink */ nullptr,
+    /* foreachTexLink */ nullptr,
+    /* freeRuntimeData */ nullptr,
     /* panelRegister */ panelRegister,
-    /* blendWrite */ NULL,
-    /* blendRead */ NULL,
+    /* blendWrite */ nullptr,
+    /* blendRead */ nullptr,
 };
 
 /** \} */
diff --git a/source/blender/nodes/geometry/nodes/node_geo_merge_by_distance.cc b/source/blender/nodes/geometry/nodes/node_geo_merge_by_distance.cc
index d1f125d78aa..15e3efcc675 100644
--- a/source/blender/nodes/geometry/nodes/node_geo_merge_by_distance.cc
+++ b/source/blender/nodes/geometry/nodes/node_geo_merge_by_distance.cc
@@ -35,12 +35,39 @@ using blender::Array;
 using blender::float3;
 using blender::Span;
 using blender::Vector;
+using blender::geometry::WeldMode;
+
+static WeldMode weld_mode_from_int(const short type)
+{
+  switch (static_cast<WeldMode>(type)) {
+    case WeldMode::all:
+      return WeldMode::all;
+    case WeldMode::connected:
+      return WeldMode::connected;
+  }
+  BLI_assert_unreachable();
+  return WeldMode::all;
+}
+
+static int16_t weld_mode_to_int(const WeldMode weld_mode)
+{
+  switch (weld_mode) {
+    case WeldMode::all:
+      return static_cast<int16_t>(WeldMode::all);
+    case WeldMode::connected:
+      return static_cast<int16_t>(WeldMode::connected);
+  }
+
+  BLI_assert_unreachable();
+  return static_cast<int16_t>(WeldMode::all);
+}
 
 namespace blender::nodes {
+
 static void geo_node_merge_by_distance_declare(NodeDeclarationBuilder &b)
 {
   b.add_input<decl::Geometry>("Geometry");
-  b.add_input<decl::Float>("Distance").min(0.0f).max(10000.0f);
+  b.add_input<decl::Float>("Distance").min(0.0f).max(10000.0f).subtype(PROP_DISTANCE);
   b.add_input<decl::Bool>("Selection").default_value(true).hide_value().supports_field();
 
   b.add_output<decl::Geometry>("Geometry");
@@ -55,11 +82,11 @@ static void geo_node_merge_by_distance_layout(uiLayout *layout,
 
 static void geo_merge_by_distance_init(bNodeTree *UNUSED(ntree), bNode *node)
 {
-  node->custom1 = weld_mode_to_int(geometry::WeldMode::all);
+  node->custom1 = weld_mode_to_int(WeldMode::all);
 }
 
 static void process_mesh(GeoNodeExecParams &params,
-                         const geometry::WeldMode weld_mode,
+                         const WeldMode weld_mode,
                          const float distance,
                          GeometrySet &geometry_set)
 {
@@ -102,29 +129,10 @@ static void geo_node_merge_by_distance_exec(GeoNodeExecParams params)
 {
   GeometrySet geometry_set = params.extract_input<GeometrySet>("Geometry");
 
-  const geometry::WeldMode weld_mode = geometry::weld_mode_from_int(params.node().custom1);
+  const geometry::WeldMode weld_mode = weld_mode_from_int(params.node().custom1);
   const float distance = params.extract_input<float>("Distance");
 
-  if (geometry_set.has_instances()) {
-    InstancesComponent &instances = geometry_set.get_component_for_write<InstancesComponent>();
-    instances.ensure_geometry_instances();
-
-    threading::parallel_for(IndexRange(instances.references_amount()), 16, [&](IndexRange range) {
-      for (int i : range) {
-        GeometrySet &geometry_set = instances.geometry_set_from_reference(i);
-        geometry_set = bke::geometry_set_realize_instances(geometry_set);
-
-        if (geometry_set.has_mesh()) {
-          process_mesh(params, weld_mode, distance, geometry_set);
-        }
-
-        if (geometry_set.has_pointcloud()) {
-          process_pointcloud(params, distance, geometry_set);
-        }
-      }
-    });
-  }
-  else {
+  geometry_set.modify_geometry_sets([&](GeometrySet &geometry_set) {
     if (geometry_set.has_mesh()) {
       process_mesh(params, weld_mode, distance, geometry_set);
     }
@@ -132,7 +140,7 @@ static void geo_node_merge_by_distance_exec(GeoNodeExecParams params)
     if (geometry_set.has_pointcloud()) {
       process_pointcloud(params, distance, geometry_set);
     }
-  }
+  });
 
   params.set_output("Geometry", std::move(geometry_set));
 }