blender
/
blender
135
420
Fork 601
Code Issues 6.3k Pull Requests 766 Projects 19 Wiki Activity

Alembic: Support new Curves object type for export #119894

Merged
Jesse Yurkovich merged 10 commits from deadpin/blender:fix-alembicexport into main 2024-04-12 21:27:25 +02:00
Conversation 4 Commits 10 Files Changed 3 +55 -53
1 changed files with 55 additions and 53 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 580d03a055 - Show all commits

108
source/blender/io/alembic/exporter/abc_writer_curves.cc
View File

@ -13,6 +13,7 @@
#include "intern/abc_axis_conversion.h"
#include "BLI_array_utils.hh"
#include "BLI_offset_indices.hh"
#include "DNA_curve_types.h"
#include "DNA_object_types.h"
@ -162,69 +163,70 @@ void ABCCurveWriter::do_write(HierarchyContext &context)
const Span<float> nurbs_weights = curves.nurbs_weights();
const VArray<int8_t> nurbs_orders = curves.nurbs_orders();
const bke::AttributeAccessor curve_attributes = curves.attributes();
deadpin marked this conversation as resolved Outdated
Hans Goudey commented 2024-03-26 13:56:25 +01:00
Outdated
Review
Copy Link

I think this should be lookup_or_default with a 0.01f default. Also, you can use the* operator on the returned attribute reader to just deal with the varray.

I think this should be `lookup_or_default` with a `0.01f` default. Also, you can use the`*` operator on the returned attribute reader to just deal with the varray.
const bke::AttributeReader<float> radii = curve_attributes.lookup<float>("radius",
bke::AttrDomain::Point);
const VArray<float> radii = *curve_attributes.lookup_or_default<float>(
"radius", bke::AttrDomain::Point, 0.01f);
vert_counts.resize(curves.curves_num());
const OffsetIndices points_by_curve = curves.points_by_curve();
for (const int i_curve : curves.curves_range()) {
const IndexRange points = points_by_curve[i_curve];
const size_t current_vert_count = verts.size();
if (blender_curve_type == CURVE_TYPE_BEZIER) {
const Span<float3> handles_l = curves.handle_positions_left();
const Span<float3> handles_r = curves.handle_positions_right();
deadpin marked this conversation as resolved Outdated
Hans Goudey commented 2024-03-25 23:17:15 +01:00
Outdated
Review
Copy Link

Could you pull the type switch out of this loop? Then the three types except Bezier will become very simple array copies and that can be reflected in the way the code looks (you could even resize the std::vectors to avoid reallocations)

Could you pull the type switch out of this loop? Then the three types except Bezier will become very simple array copies and that can be reflected in the way the code looks (you could even resize the std::vectors to avoid reallocations)
Jesse Yurkovich commented 2024-03-26 08:41:54 +01:00
Outdated
Review
Copy Link

I think I can. The code is still a bit complicated though, maybe I'm going about it wrong? Here's a snippet of what it would look like to see if I'm on the right track. Notice that the orders and vert_counts arrays need filled in differently too.

  if (blender_curve_type == CURVE_TYPE_BEZIER) {
    ...
  }
  else {
    verts.resize(curves.points_num());    // can probably stick with reserve and push_back but...
    widths.resize(curves.points_num()); // ...this was just a test
    for (const int i_point : curves.points_range()) {
      verts[i_point] = to_yup_V3f(positions[i_point]);
      widths[i_point] = radii.varray[i_point] * 2.0f;
    }

    if (blender_curve_type == CURVE_TYPE_NURBS) {
      weights.resize(curves.points_num());
      std::copy_n(nurbs_weights.data(), weights.size(), weights.data());
    }

    orders.reserve(curves.curves_num());
    vert_counts.reserve(curves.curves_num());
    for (const int i_curve : curves.curves_range()) {
      orders.push_back(nurbs_orders[i_curve]);
      vert_counts.push_back(points_by_curve[i_curve].size());
    }
  }
I think I can. The code is still a bit complicated though, maybe I'm going about it wrong? Here's a snippet of what it would look like to see if I'm on the right track. Notice that the `orders` and `vert_counts` arrays need filled in differently too. ```cpp if (blender_curve_type == CURVE_TYPE_BEZIER) { ... } else { verts.resize(curves.points_num()); // can probably stick with reserve and push_back but... widths.resize(curves.points_num()); // ...this was just a test for (const int i_point : curves.points_range()) { verts[i_point] = to_yup_V3f(positions[i_point]); widths[i_point] = radii.varray[i_point] * 2.0f; } if (blender_curve_type == CURVE_TYPE_NURBS) { weights.resize(curves.points_num()); std::copy_n(nurbs_weights.data(), weights.size(), weights.data()); } orders.reserve(curves.curves_num()); vert_counts.reserve(curves.curves_num()); for (const int i_curve : curves.curves_range()) { orders.push_back(nurbs_orders[i_curve]); vert_counts.push_back(points_by_curve[i_curve].size()); } } ```
Hans Goudey commented 2024-03-26 13:55:00 +01:00
Outdated
Review
Copy Link

Yeah, something like that makes more sense I think.

I wouldn't expect push_back to be necessary for the loops at all TBH. Just copy_n like you use in on place (except for positions and radii which need special handling I suppose).

Also, seems like nurbs_orders could only be read from CurvesGeometry if it is exporting NURBS curves.

And also, vert_counts can be done like this: offset_indices::copy_group_sizes(points_by_curve, points_by_curve.index_range(), vert_counts);

Yeah, something like that makes more sense I think. I wouldn't expect `push_back` to be necessary for the loops at all TBH. Just `copy_n` like you use in on place (except for positions and radii which need special handling I suppose). Also, seems like `nurbs_orders` could only be read from `CurvesGeometry` if it is exporting NURBS curves. And also, `vert_counts` can be done like this: `offset_indices::copy_group_sizes(points_by_curve, points_by_curve.index_range(), vert_counts);`
switch (blender_curve_type) {
case CURVE_TYPE_POLY:
case CURVE_TYPE_CATMULL_ROM:
for (const int i_point : points) {
verts.push_back(to_yup_V3f(positions[i_point]));
widths.push_back(radii.varray[i_point] * 2.0f);
}
break;
for (const int i_curve : curves.curves_range()) {
const IndexRange points = points_by_curve[i_curve];
const size_t current_vert_count = verts.size();
case CURVE_TYPE_BEZIER: {
const Span<float3> handles_l = curves.handle_positions_left();
const Span<float3> handles_r = curves.handle_positions_right();
const int start_point_index = points.first();
const int last_point_index = points.last();
const int start_point_index = points.first();
const int last_point_index = points.last();
/* Vert order in the bezier curve representation is:
* [
* control point 0(+ width), right handle 0, left handle 1,
* control point 1(+ width), right handle 1, left handle 2,
* control point 2(+ width), ...
* ] */
for (int i_point = start_point_index; i_point < last_point_index; i_point++) {
verts.push_back(to_yup_V3f(positions[i_point]));
widths.push_back(radii[last_point_index] * 2.0f);
/* Vert order in the bezier curve representation is:
* [
* control point 0(+ width), right handle 0, left handle 1,
* control point 1(+ width), right handle 1, left handle 2,
* control point 2(+ width), ...
* ] */
for (int i_point = start_point_index; i_point < last_point_index; i_point++) {
verts.push_back(to_yup_V3f(positions[i_point]));
widths.push_back(radii.varray[last_point_index] * 2.0f);
verts.push_back(to_yup_V3f(handles_r[i_point]));
verts.push_back(to_yup_V3f(handles_l[i_point + 1]));
}
/* The last vert in the array doesn't need a right handle because the curve stops
* at that point. */
verts.push_back(to_yup_V3f(positions[last_point_index]));
widths.push_back(radii.varray[last_point_index] * 2.0f);
/* If the curve is cyclic, include the right handle of the last point and the
* left handle of the first point. */
if (is_cyclic) {
verts.push_back(to_yup_V3f(handles_r[last_point_index]));
verts.push_back(to_yup_V3f(handles_l[start_point_index]));
}
break;
verts.push_back(to_yup_V3f(handles_r[i_point]));
deadpin marked this conversation as resolved
Hans Goudey commented 2024-04-02 03:34:38 +02:00
Review
Copy Link

Just noticed, I think this can be simpler as for (const int point : points) {

Just noticed, I think this can be simpler as `for (const int point : points) {`
verts.push_back(to_yup_V3f(handles_l[i_point + 1]));
}
deadpin marked this conversation as resolved
Hans Goudey commented 2024-04-02 03:35:04 +02:00
Review
Copy Link

Seems the radius should be accessed from the current point rather than always the last point in the curve

Seems the radius should be accessed from the current point rather than always the last point in the curve
case CURVE_TYPE_NURBS:
for (const int i_point : points) {
verts.push_back(to_yup_V3f(positions[i_point]));
weights.push_back(nurbs_weights[i_point]);
widths.push_back(radii.varray[i_point] * 2.0f);
}
break;
/* The last vert in the array doesn't need a right handle because the curve stops
* at that point. */
verts.push_back(to_yup_V3f(positions[last_point_index]));
widths.push_back(radii[last_point_index] * 2.0f);
/* If the curve is cyclic, include the right handle of the last point and the
* left handle of the first point. */
if (is_cyclic) {
verts.push_back(to_yup_V3f(handles_r[last_point_index]));
verts.push_back(to_yup_V3f(handles_l[start_point_index]));
}
vert_counts[i_curve] = verts.size() - current_vert_count;
}
}
else {
verts.resize(curves.points_num());
widths.resize(curves.points_num());
for (const int i_point : curves.points_range()) {
verts[i_point] = to_yup_V3f(positions[i_point]);
deadpin marked this conversation as resolved Outdated
Hans Goudey commented 2024-03-25 23:15:32 +01:00
Outdated
Review
Copy Link

same break formatting thing here

same break formatting thing here
widths[i_point] = radii[i_point] * 2.0f;
}
orders.push_back(nurbs_orders[i_curve]);
vert_counts.push_back(verts.size() - current_vert_count);
if (blender_curve_type == CURVE_TYPE_NURBS) {
weights.resize(curves.points_num());
std::copy_n(nurbs_weights.data(), weights.size(), weights.data());
orders.resize(curves.curves_num());
for (const int i_curve : curves.curves_range()) {
orders[i_curve] = nurbs_orders[i_curve];
}
}
offset_indices::copy_group_sizes(points_by_curve, points_by_curve.index_range(), vert_counts);
}
Alembic::AbcGeom::OFloatGeomParam::Sample width_sample;