Curves: Remove redundant custom data pointers

These mutable pointers present problems with ownership in relation to
proper copy-on-write for attributes. The simplest solution is to just
remove them and retrieve the layers from  `CustomData` when they are
needed. This also removes the complexity and redundancy of having to
update the pointers as the curves change. A similar change will apply
to meshes and point clouds.

One downside of this change is that it makes random access with RNA
slower. However, it's simple to just use the RNA attribute API instead,
which is unaffected. In this patch I updated Cycles to do that. With
the future attribute CoW changes, this generic approach makes sense
because Cycles can just request ownership of the existing arrays.

Differential Revision: https://developer.blender.org/D15486

intern/cycles/blender/curves.cpp

@@ -630,6 +630,25 @@ static std::optional<BL::FloatAttribute> find_curves_radius_attribute(BL::Curves
   return std::nullopt;
 }
 
+static BL::FloatVectorAttribute find_curves_position_attribute(BL::Curves b_curves)
+{
+  for (BL::Attribute &b_attribute : b_curves.attributes) {
+    if (b_attribute.name() != "position") {
+      continue;
+    }
+    if (b_attribute.domain() != BL::Attribute::domain_POINT) {
+      continue;
+    }
+    if (b_attribute.data_type() != BL::Attribute::data_type_FLOAT_VECTOR) {
+      continue;
+    }
+    return BL::FloatVectorAttribute{b_attribute};
+  }
+  /* The position attribute must exist. */
+  assert(false);
+  return BL::FloatVectorAttribute{b_curves.attributes[0]};
+}
+
 template<typename TypeInCycles, typename GetValueAtIndex>
 static void fill_generic_attribute(BL::Curves &b_curves,
                                    TypeInCycles *data,
@@ -793,16 +812,16 @@ static void attr_create_generic(Scene *scene,
   }
 }
 
-static float4 hair_point_as_float4(BL::Curves b_curves,
+static float4 hair_point_as_float4(BL::FloatVectorAttribute b_attr_position,
                                    std::optional<BL::FloatAttribute> b_attr_radius,
                                    const int index)
 {
-  float4 mP = float3_to_float4(get_float3(b_curves.position_data[index].vector()));
+  float4 mP = float3_to_float4(get_float3(b_attr_position.data[index].vector()));
   mP.w = b_attr_radius ? b_attr_radius->data[index].value() : 0.0f;
   return mP;
 }
 
-static float4 interpolate_hair_points(BL::Curves b_curves,
+static float4 interpolate_hair_points(BL::FloatVectorAttribute b_attr_position,
                                       std::optional<BL::FloatAttribute> b_attr_radius,
                                       const int first_point_index,
                                       const int num_points,
@@ -812,8 +831,8 @@ static float4 interpolate_hair_points(BL::Curves b_curves,
   const int point_a = clamp((int)curve_t, 0, num_points - 1);
   const int point_b = min(point_a + 1, num_points - 1);
   const float t = curve_t - (float)point_a;
-  return lerp(hair_point_as_float4(b_curves, b_attr_radius, first_point_index + point_a),
-              hair_point_as_float4(b_curves, b_attr_radius, first_point_index + point_b),
+  return lerp(hair_point_as_float4(b_attr_position, b_attr_radius, first_point_index + point_a),
+              hair_point_as_float4(b_attr_position, b_attr_radius, first_point_index + point_b),
               t);
 }
 
@@ -846,6 +865,7 @@ static void export_hair_curves(Scene *scene,
 
   hair->reserve_curves(num_curves, num_keys);
 
+  BL::FloatVectorAttribute b_attr_position = find_curves_position_attribute(b_curves);
   std::optional<BL::FloatAttribute> b_attr_radius = find_curves_radius_attribute(b_curves);
 
   /* Export curves and points. */
@@ -864,7 +884,7 @@ static void export_hair_curves(Scene *scene,
 
     /* Position and radius. */
     for (int i = 0; i < num_points; i++) {
-      const float3 co = get_float3(b_curves.position_data[first_point_index + i].vector());
+      const float3 co = get_float3(b_attr_position.data[first_point_index + i].vector());
       const float radius = b_attr_radius ? b_attr_radius->data[first_point_index + i].value() :
                                            0.005f;
       hair->add_curve_key(co, radius);
@@ -921,6 +941,7 @@ static void export_hair_curves_motion(Hair *hair, BL::Curves b_curves, int motio
   int num_motion_keys = 0;
   int curve_index = 0;
 
+  BL::FloatVectorAttribute b_attr_position = find_curves_position_attribute(b_curves);
   std::optional<BL::FloatAttribute> b_attr_radius = find_curves_radius_attribute(b_curves);
 
   for (int i = 0; i < num_curves; i++) {
@@ -936,7 +957,7 @@ static void export_hair_curves_motion(Hair *hair, BL::Curves b_curves, int motio
         int point_index = first_point_index + i;
 
         if (point_index < num_keys) {
-          mP[num_motion_keys] = hair_point_as_float4(b_curves, b_attr_radius, point_index);
+          mP[num_motion_keys] = hair_point_as_float4(b_attr_position, b_attr_radius, point_index);
           num_motion_keys++;
 
           if (!have_motion) {
@@ -956,7 +977,7 @@ static void export_hair_curves_motion(Hair *hair, BL::Curves b_curves, int motio
       for (int i = 0; i < curve.num_keys; i++) {
         const float step = i * step_size;
         mP[num_motion_keys] = interpolate_hair_points(
-            b_curves, b_attr_radius, first_point_index, num_points, step);
+            b_attr_position, b_attr_radius, first_point_index, num_points, step);
         num_motion_keys++;
       }
       have_motion = true;

source/blender/blenkernel/BKE_curves.hh

@@ -385,8 +385,6 @@ class CurvesGeometry : public ::CurvesGeometry {
 
   void calculate_bezier_auto_handles();
 
-  void update_customdata_pointers();
-
   void remove_points(IndexMask points_to_delete);
   void remove_curves(IndexMask curves_to_delete);
 

source/blender/blenkernel/intern/curves.cc

@@ -53,8 +53,6 @@ using blender::Vector;
 
 static const char *ATTR_POSITION = "position";
 
-static void update_custom_data_pointers(Curves &curves);
-
 static void curves_init_data(ID *id)
 {
   Curves *curves = (Curves *)id;
@@ -97,8 +95,6 @@ static void curves_copy_data(Main *UNUSED(bmain), ID *id_dst, const ID *id_src,
 
   dst.runtime->type_counts = src.runtime->type_counts;
 
-  dst.update_customdata_pointers();
-
   curves_dst->batch_cache = nullptr;
 }
 
@@ -170,7 +166,6 @@ static void curves_blend_read_data(BlendDataReader *reader, ID *id)
   /* Geometry */
   CustomData_blend_read(reader, &curves->geometry.point_data, curves->geometry.point_num);
   CustomData_blend_read(reader, &curves->geometry.curve_data, curves->geometry.curve_num);
-  update_custom_data_pointers(*curves);
 
   BLO_read_int32_array(reader, curves->geometry.curve_num + 1, &curves->geometry.curve_offsets);
 
@@ -233,11 +228,6 @@ IDTypeInfo IDType_ID_CV = {
     /*lib_override_apply_post */ nullptr,
 };
 
-static void update_custom_data_pointers(Curves &curves)
-{
-  blender::bke::CurvesGeometry::wrap(curves.geometry).update_customdata_pointers();
-}
-
 void *BKE_curves_add(Main *bmain, const char *name)
 {
   Curves *curves = static_cast<Curves *>(BKE_id_new(bmain, ID_CV, name));

source/blender/blenkernel/intern/curves_geometry.cc

@@ -68,8 +68,6 @@ CurvesGeometry::CurvesGeometry(const int point_num, const int curve_num)
 #endif
   this->offsets_for_write().first() = 0;
 
-  this->update_customdata_pointers();
-
   this->runtime = MEM_new<CurvesGeometryRuntime>(__func__);
   /* Fill the type counts with the default so they're in a valid state. */
   this->runtime->type_counts[CURVE_TYPE_CATMULL_ROM] = curve_num;
@@ -95,8 +93,6 @@ static void copy_curves_geometry(CurvesGeometry &dst, const CurvesGeometry &src)
 
   /* Though type counts are a cache, they must be copied because they are calculated eagerly. */
   dst.runtime->type_counts = src.runtime->type_counts;
-
-  dst.update_customdata_pointers();
 }
 
 CurvesGeometry::CurvesGeometry(const CurvesGeometry &other)
@@ -130,9 +126,6 @@ static void move_curves_geometry(CurvesGeometry &dst, CurvesGeometry &src)
   MEM_SAFE_FREE(src.curve_offsets);
 
   std::swap(dst.runtime, src.runtime);
-
-  src.update_customdata_pointers();
-  dst.update_customdata_pointers();
 }
 
 CurvesGeometry::CurvesGeometry(CurvesGeometry &&other)
@@ -306,13 +299,11 @@ void CurvesGeometry::update_curve_types()
 
 Span<float3> CurvesGeometry::positions() const
 {
-  return {(const float3 *)this->position, this->point_num};
+  return get_span_attribute<float3>(*this, ATTR_DOMAIN_POINT, ATTR_POSITION);
 }
 MutableSpan<float3> CurvesGeometry::positions_for_write()
 {
-  this->position = (float(*)[3])CustomData_duplicate_referenced_layer_named(
-      &this->point_data, CD_PROP_FLOAT3, ATTR_POSITION.c_str(), this->point_num);
-  return {(float3 *)this->position, this->point_num};
+  return get_mutable_attribute<float3>(*this, ATTR_DOMAIN_POINT, ATTR_POSITION);
 }
 
 Span<int> CurvesGeometry::offsets() const
@@ -961,7 +952,6 @@ void CurvesGeometry::resize(const int points_num, const int curves_num)
     this->curve_offsets = (int *)MEM_reallocN(this->curve_offsets, sizeof(int) * (curves_num + 1));
   }
   this->tag_topology_changed();
-  this->update_customdata_pointers();
 }
 
 void CurvesGeometry::tag_positions_changed()
@@ -1060,10 +1050,11 @@ void CurvesGeometry::transform(const float4x4 &matrix)
 
 static std::optional<bounds::MinMaxResult<float3>> curves_bounds(const CurvesGeometry &curves)
 {
-  Span<float3> positions = curves.positions();
-  if (curves.radius) {
-    Span<float> radii{curves.radius, curves.points_num()};
-    return bounds::min_max_with_radii(positions, radii);
+  const Span<float3> positions = curves.positions();
+  const VArray<float> radii = curves.attributes().lookup_or_default<float>(
+      ATTR_RADIUS, ATTR_DOMAIN_POINT, 0.0f);
+  if (!(radii.is_single() && radii.get_internal_single() == 0.0f)) {
+    return bounds::min_max_with_radii(positions, radii.get_internal_span());
   }
   return bounds::min_max(positions);
 }
@@ -1079,16 +1070,6 @@ bool CurvesGeometry::bounds_min_max(float3 &min, float3 &max) const
   return true;
 }
 
-void CurvesGeometry::update_customdata_pointers()
-{
-  this->position = (float(*)[3])CustomData_get_layer_named(
-      &this->point_data, CD_PROP_FLOAT3, ATTR_POSITION.c_str());
-  this->radius = (float *)CustomData_get_layer_named(
-      &this->point_data, CD_PROP_FLOAT, ATTR_RADIUS.c_str());
-  this->curve_type = (int8_t *)CustomData_get_layer_named(
-      &this->point_data, CD_PROP_INT8, ATTR_CURVE_TYPE.c_str());
-}
-
 static void *ensure_customdata_layer(CustomData &custom_data,
                                      const StringRefNull name,
                                      const eCustomDataType data_type,
@@ -1497,7 +1478,6 @@ void CurvesGeometry::remove_attributes_based_on_types()
   if (!this->has_curve_with_type({CURVE_TYPE_BEZIER, CURVE_TYPE_CATMULL_ROM, CURVE_TYPE_NURBS})) {
     CustomData_free_layer_named(&this->curve_data, ATTR_RESOLUTION.c_str(), curves_num);
   }
-  this->update_customdata_pointers();
 }
 
 /** \} */

source/blender/blenkernel/intern/geometry_component_curves.cc

@@ -358,10 +358,7 @@ static ComponentAttributeProviders create_attribute_providers_for_curve()
         const CurvesGeometry &curves = *static_cast<const CurvesGeometry *>(owner);
         return curves.curves_num();
       },
-      [](void *owner) {
-        CurvesGeometry &curves = *static_cast<CurvesGeometry *>(owner);
-        curves.update_customdata_pointers();
-      }};
+      [](void * /*owner*/) {}};
   static CustomDataAccessInfo point_access = {
       [](void *owner) -> CustomData * {
         CurvesGeometry &curves = *static_cast<CurvesGeometry *>(owner);
@@ -375,10 +372,7 @@ static ComponentAttributeProviders create_attribute_providers_for_curve()
         const CurvesGeometry &curves = *static_cast<const CurvesGeometry *>(owner);
         return curves.points_num();
       },
-      [](void *owner) {
-        CurvesGeometry &curves = *static_cast<CurvesGeometry *>(owner);
-        curves.update_customdata_pointers();
-      }};
+      [](void * /*owner*/) {}};
 
   static BuiltinCustomDataLayerProvider position("position",
                                                  ATTR_DOMAIN_POINT,

source/blender/geometry/intern/resample_curves.cc

@@ -162,8 +162,6 @@ static void gather_point_attributes_to_interpolate(const CurveComponent &src_com
                            result.src_no_interpolation,
                            result.dst_no_interpolation,
                            result.dst_attributes);
-
-  dst_curves.update_customdata_pointers();
 }
 
 static Curves *resample_to_uniform(const CurveComponent &src_component,

source/blender/makesdna/DNA_curves_types.h

@@ -66,23 +66,6 @@ typedef enum NormalMode {
  * curve-processing algorithms for multiple Blender data-block types.
  */
 typedef struct CurvesGeometry {
-  /**
-   * A runtime pointer to the "position" attribute data.
-   * \note This data is owned by #point_data.
-   */
-  float (*position)[3];
-  /**
-   * A runtime pointer to the "radius" attribute data.
-   * \note This data is owned by #point_data.
-   */
-  float *radius;
-
-  /**
-   * The type of each curve. #CurveType.
-   * \note This data is owned by #curve_data.
-   */
-  int8_t *curve_type;
-
   /**
    * The start index of each curve in the point data. The size of each curve can be calculated by
    * subtracting the offset from the next offset. That is valid even for the last curve because

source/blender/makesrna/intern/rna_curves.c

@@ -64,7 +64,24 @@ static int rna_CurvePoint_index_get_const(const PointerRNA *ptr)
 {
   const Curves *curves = rna_curves(ptr);
   const float(*co)[3] = ptr->data;
-  return (int)(co - curves->geometry.position);
+  const float(*positions)[3] = (const float(*)[3])CustomData_get_layer_named(
+      &curves->geometry.point_data, CD_PROP_FLOAT3, "position");
+  return (int)(co - positions);
+}
+
+static int rna_Curves_position_data_length(PointerRNA *ptr)
+{
+  const Curves *curves = rna_curves(ptr);
+  return curves->geometry.point_num;
+}
+
+static void rna_Curves_position_data_begin(CollectionPropertyIterator *iter, PointerRNA *ptr)
+{
+  const Curves *curves = rna_curves(ptr);
+  const float(*positions)[3] = (const float(*)[3])CustomData_get_layer_named(
+      &curves->geometry.point_data, CD_PROP_FLOAT3, "position");
+  rna_iterator_array_begin(
+      iter, (void *)positions, sizeof(float[3]), curves->geometry.point_num, false, NULL);
 }
 
 static int rna_CurvePoint_index_get(PointerRNA *ptr)
@@ -85,21 +102,23 @@ static void rna_CurvePoint_location_set(PointerRNA *ptr, const float value[3])
 static float rna_CurvePoint_radius_get(PointerRNA *ptr)
 {
   const Curves *curves = rna_curves(ptr);
-  if (curves->geometry.radius == NULL) {
+  const float *radii = (const float *)CustomData_get_layer_named(
+      &curves->geometry.point_data, CD_PROP_FLOAT, "radius");
+  if (radii == NULL) {
     return 0.0f;
   }
-  const float(*co)[3] = ptr->data;
-  return curves->geometry.radius[co - curves->geometry.position];
+  return radii[rna_CurvePoint_index_get_const(ptr)];
 }
 
 static void rna_CurvePoint_radius_set(PointerRNA *ptr, float value)
 {
   const Curves *curves = rna_curves(ptr);
-  if (curves->geometry.radius == NULL) {
+  float *radii = (float *)CustomData_get_layer_named(
+      &curves->geometry.point_data, CD_PROP_FLOAT, "radius");
+  if (radii == NULL) {
     return;
   }
-  const float(*co)[3] = ptr->data;
-  curves->geometry.radius[co - curves->geometry.position] = value;
+  radii[rna_CurvePoint_index_get_const(ptr)] = value;
 }
 
 static char *rna_CurvePoint_path(const PointerRNA *ptr)
@@ -123,16 +142,6 @@ static char *rna_CurveSlice_path(const PointerRNA *ptr)
   return BLI_sprintfN("curves[%d]", rna_CurveSlice_index_get_const(ptr));
 }
 
-static void rna_CurveSlice_points_begin(CollectionPropertyIterator *iter, PointerRNA *ptr)
-{
-  Curves *curves = rna_curves(ptr);
-  const int *offset_ptr = (int *)ptr->data;
-  const int offset = *offset_ptr;
-  const int size = *(offset_ptr + 1) - offset;
-  float(*co)[3] = curves->geometry.position + *offset_ptr;
-  rna_iterator_array_begin(iter, co, sizeof(float[3]), size, 0, NULL);
-}
-
 static int rna_CurveSlice_first_point_index_get(PointerRNA *ptr)
 {
   const int *offset_ptr = (int *)ptr->data;
@@ -146,6 +155,17 @@ static int rna_CurveSlice_points_length_get(PointerRNA *ptr)
   return *(offset_ptr + 1) - offset;
 }
 
+static void rna_CurveSlice_points_begin(CollectionPropertyIterator *iter, PointerRNA *ptr)
+{
+  Curves *curves = rna_curves(ptr);
+  const int offset = rna_CurveSlice_first_point_index_get(ptr);
+  const int size = rna_CurveSlice_points_length_get(ptr);
+  float(*positions)[3] = (float(*)[3])CustomData_get_layer_named(
+      &curves->geometry.point_data, CD_PROP_FLOAT3, "position");
+  float(*co)[3] = positions + offset;
+  rna_iterator_array_begin(iter, co, sizeof(float[3]), size, 0, NULL);
+}
+
 static void rna_Curves_update_data(struct Main *UNUSED(bmain),
                                    struct Scene *UNUSED(scene),
                                    PointerRNA *ptr)
@@ -252,20 +272,32 @@ static void rna_def_curves(BlenderRNA *brna)
   RNA_def_property_struct_type(prop, "CurveSlice");
   RNA_def_property_ui_text(prop, "Curves", "All curves in the data-block");
 
-  /* TODO: better solution for (*co)[3] parsing issue. */
-
-  RNA_define_verify_sdna(0);
   prop = RNA_def_property(srna, "points", PROP_COLLECTION, PROP_NONE);
-  RNA_def_property_collection_sdna(prop, NULL, "geometry.position", "geometry.point_num");
   RNA_def_property_struct_type(prop, "CurvePoint");
+  RNA_def_property_collection_funcs(prop,
+                                    "rna_Curves_position_data_begin",
+                                    "rna_iterator_array_next",
+                                    "rna_iterator_array_end",
+                                    "rna_iterator_array_get",
+                                    "rna_Curves_position_data_length",
+                                    NULL,
+                                    NULL,
+                                    NULL);
   RNA_def_property_ui_text(prop, "Points", "Control points of all curves");
-  RNA_define_verify_sdna(1);
 
   /* Direct access to built-in attributes. */
 
   RNA_define_verify_sdna(0);
   prop = RNA_def_property(srna, "position_data", PROP_COLLECTION, PROP_NONE);
-  RNA_def_property_collection_sdna(prop, NULL, "geometry.position", "geometry.point_num");
+  RNA_def_property_collection_funcs(prop,
+                                    "rna_Curves_position_data_begin",
+                                    "rna_iterator_array_next",
+                                    "rna_iterator_array_end",
+                                    "rna_iterator_array_get",
+                                    "rna_Curves_position_data_length",
+                                    NULL,
+                                    NULL,
+                                    NULL);
   RNA_def_property_struct_type(prop, "FloatVectorAttributeValue");
   RNA_def_property_update(prop, 0, "rna_Curves_update_data");
   RNA_define_verify_sdna(1);