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
3 changed files with 105 additions and 65 deletions
Showing only changes of commit 6582eab14c - Show all commits

View File

@ -191,6 +191,7 @@ ABCAbstractWriter *ABCHierarchyIterator::create_data_writer_for_object_type(
case OB_CAMERA:
return new ABCCameraWriter(writer_args);
case OB_CURVES_LEGACY:
case OB_CURVES:
if (params_.curves_as_mesh) {
return new ABCCurveMeshWriter(writer_args);
}

View File

@ -6,13 +6,18 @@
* \ingroup balembic
*/
#include <functional>
#include <memory>
#include "abc_writer_curves.h"
#include "intern/abc_axis_conversion.h"
#include "DNA_curve_types.h"
#include "DNA_object_types.h"
#include "BKE_curve.hh"
#include "BKE_curve_legacy_convert.hh"
#include "BKE_curves.hh"
#include "BKE_lib_id.hh"
#include "BKE_mesh.hh"
#include "BKE_object.hh"
@ -27,7 +32,7 @@ using Alembic::AbcGeom::ON3fGeomParam;
using Alembic::AbcGeom::OV2fGeomParam;
namespace blender::io::alembic {
#pragma optimize("", off)
const std::string ABC_CURVE_RESOLUTION_U_PROPNAME("blender:resolution");
ABCCurveWriter::ABCCurveWriter(const ABCWriterConstructorArgs &args) : ABCAbstractWriter(args) {}
@ -56,7 +61,29 @@ Alembic::Abc::OCompoundProperty ABCCurveWriter::abc_prop_for_custom_props()
void ABCCurveWriter::do_write(HierarchyContext &context)
{
Curve *curve = static_cast<Curve *>(context.object->data);
Curves *curves;
std::unique_ptr<Curves, std::function<void(Curves *)>> converted_curves;
switch (context.object->type) {
case OB_CURVES_LEGACY: {
const Curve *legacy_curve = static_cast<Curve *>(context.object->data);
deadpin marked this conversation as resolved Outdated

I guess Alembic requires the resolution to be just a single value?

I guess Alembic requires the resolution to be just a single value?

This is actually a blender-specific property we attach. It's used so our reader code can then pull this out if present. I suppose it was better for roundtripping in the past. For the new Curves object should we just not bother you think? When this is not present, or <= 0, the reader will simply not call curves.resolution_for_write().fill(...) which I think is fine?

This is actually a blender-specific property we attach. It's used so our reader code can then pull this out if present. I suppose it was better for roundtripping in the past. For the new Curves object should we just not bother you think? When this is not present, or <= 0, the reader will simply not call `curves.resolution_for_write().fill(...)` which I think is fine?

Hmm, well the new curves type has a resolution stored per curve (so did the old one actually, with Nurb::resolu, so ideally that would be written and read as well. But maybe that could wait until all generic attributes are supported here too.

Hmm, well the new curves type has a resolution stored per curve (so did the old one actually, with `Nurb::resolu`, so ideally that would be written and read as well. But maybe that could wait until all generic attributes are supported here too.

I see where you're coming from now. Would a TODO comment suffice for now to do some followup later since this is no better or worse than before?

I see where you're coming from now. Would a TODO comment suffice for now to do some followup later since this is no better or worse than before?

Yeah a TODO comment is fine IMO

Yeah a TODO comment is fine IMO
converted_curves = std::unique_ptr<Curves, std::function<void(Curves *)>>(
bke::curve_legacy_to_curves(*legacy_curve), [](Curves *c) { BKE_id_free(nullptr, c); });
curves = converted_curves.get();
break;
}
case OB_CURVES:
curves = static_cast<Curves *>(context.object->data);
break;
default:
BLI_assert_unreachable();
return;
}
const bke::CurvesGeometry &geometry = curves->geometry.wrap();
if (geometry.points_num() == 0) {
deadpin marked this conversation as resolved Outdated

const here would be nice I guess

`const` here would be nice I guess
return;
}
std::vector<Imath::V3f> verts;
std::vector<int32_t> vert_counts;
@ -70,79 +97,87 @@ void ABCCurveWriter::do_write(HierarchyContext &context)
Alembic::AbcGeom::CurveType curve_type = Alembic::AbcGeom::kVariableOrder;
Alembic::AbcGeom::CurvePeriodicity periodicity = Alembic::AbcGeom::kNonPeriodic;
Nurb *nurbs = static_cast<Nurb *>(curve->nurb.first);
for (; nurbs; nurbs = nurbs->next) {
const size_t current_point_count = verts.size();
if (nurbs->bp) {
curve_basis = Alembic::AbcGeom::kNoBasis;
curve_type = Alembic::AbcGeom::kVariableOrder;
const Span<float3> positions = geometry.positions();
const Span<float> nurbs_weights = geometry.nurbs_weights();
const VArray<int8_t> nurbs_orders = geometry.nurbs_orders();
const VArray<bool> is_cyclic = geometry.cyclic();
deadpin marked this conversation as resolved
Review

Naming would be more typical if curves was curves_id and geometry was curves.

Naming would be more typical if `curves` was `curves_id` and `geometry` was `curves`.
const bke::AttributeAccessor curve_attributes = geometry.attributes();
const bke::AttributeReader<float> radii = curve_attributes.lookup<float>("radius",
bke::AttrDomain::Point);
const int totpoint = nurbs->pntsu * nurbs->pntsv;
const OffsetIndices points_by_curve = geometry.points_by_curve();
for (const int i_curve : geometry.curves_range()) {
const IndexRange points = points_by_curve[i_curve];
const int8_t blender_curve_type = geometry.curve_types()[i_curve];
const BPoint *point = nurbs->bp;
switch (blender_curve_type) {
case CURVE_TYPE_POLY:
curve_basis = Alembic::AbcGeom::kNoBasis;
curve_type = Alembic::AbcGeom::kVariableOrder;
for (int i = 0; i < totpoint; i++, point++) {
copy_yup_from_zup(temp_vert.getValue(), point->vec);
verts.push_back(temp_vert);
weights.push_back(point->vec[3]);
widths.push_back(point->radius);
}
}
else if (nurbs->bezt) {
curve_basis = Alembic::AbcGeom::kBezierBasis;
curve_type = Alembic::AbcGeom::kCubic;
for (const int i_point : points) {
copy_yup_from_zup(temp_vert.getValue(), positions[i_point]);
verts.push_back(temp_vert);
weights.push_back(1.0f);
deadpin marked this conversation as resolved
Review

const

Also we have a super advanced utility for this :D array_utils::booleans_mix_calc

`const` Also we have a super advanced utility for this :D `array_utils::booleans_mix_calc`
Review

Huh... I would never have guessed with that name :)

Huh... I would never have guessed with that name :)
widths.push_back(radii.varray[i_point] * 2.0f);
}
deadpin marked this conversation as resolved Outdated

Cyclic won't be empty here, since it gives you a varray with a single default value when the attribute doesn't exist.

Cyclic won't be empty here, since it gives you a varray with a single default value when the attribute doesn't exist.
const int totpoint = nurbs->pntsu;
break;
const BezTriple *bezier = nurbs->bezt;
case CURVE_TYPE_CATMULL_ROM:
curve_basis = Alembic::AbcGeom::kCatmullromBasis;
curve_type = Alembic::AbcGeom::kVariableOrder;
/* TODO(kevin): store info about handles, Alembic doesn't have this. */
for (int i = 0; i < totpoint; i++, bezier++) {
copy_yup_from_zup(temp_vert.getValue(), bezier->vec[1]);
verts.push_back(temp_vert);
widths.push_back(bezier->radius);
}
for (const int i_point : points) {
copy_yup_from_zup(temp_vert.getValue(), positions[i_point]);
verts.push_back(temp_vert);
weights.push_back(1.0f);
deadpin marked this conversation as resolved Outdated

cast to CurveType for compiler warnings in the switch if we ever add a new type

cast to `CurveType` for compiler warnings in the switch if we ever add a new type
widths.push_back(radii.varray[i_point] * 2.0f);
}
break;
case CURVE_TYPE_BEZIER:
curve_basis = Alembic::AbcGeom::kBezierBasis;
curve_type = Alembic::AbcGeom::kCubic;
/* TODO: Does Alembic support left/right handle data at all? */
for (const int i_point : points) {
copy_yup_from_zup(temp_vert.getValue(), positions[i_point]);
verts.push_back(temp_vert);
weights.push_back(1.0f);
widths.push_back(radii.varray[i_point] * 2.0f);
}
break;
case CURVE_TYPE_NURBS:
curve_basis = Alembic::AbcGeom::kBsplineBasis;
curve_type = Alembic::AbcGeom::kVariableOrder;
for (const int i_point : points) {
copy_yup_from_zup(temp_vert.getValue(), positions[i_point]);
verts.push_back(temp_vert);
weights.push_back(nurbs_weights[i_point]);
widths.push_back(radii.varray[i_point] * 2.0f);
}
break;
deadpin marked this conversation as resolved Outdated

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.
}
if ((nurbs->flagu & CU_NURB_ENDPOINT) != 0) {
periodicity = Alembic::AbcGeom::kNonPeriodic;
}
else if ((nurbs->flagu & CU_NURB_CYCLIC) != 0) {
int size_adjust = 0;
if (is_cyclic[i_curve]) {
/* Duplicate the first N vertices to indicate periodic construction. */
periodicity = Alembic::AbcGeom::kPeriodic;
/* Duplicate the start points to indicate that the curve is actually
* cyclic since other software need those.
*/
for (int i = 0; i < nurbs->orderu; i++) {
verts.push_back(verts[i]);
size_adjust = nurbs_orders[i_curve];
deadpin marked this conversation as resolved Outdated

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)

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()); } } ```

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);`
for (const int i_point : points.take_front(size_adjust)) {
verts.push_back(verts[i_point]);
}
}
if (nurbs->knotsu != nullptr) {
const size_t num_knots = KNOTSU(nurbs);
/* Add an extra knot at the beginning and end of the array since most apps
* require/expect them. */
knots.resize(num_knots + 2);
for (int i = 0; i < num_knots; i++) {
knots[i + 1] = nurbs->knotsu[i];
}
if ((nurbs->flagu & CU_NURB_CYCLIC) != 0) {
knots[0] = nurbs->knotsu[0];
knots[num_knots - 1] = nurbs->knotsu[num_knots - 1];
}
else {
knots[0] = (2.0f * nurbs->knotsu[0] - nurbs->knotsu[1]);
knots[num_knots - 1] = (2.0f * nurbs->knotsu[num_knots - 1] -
nurbs->knotsu[num_knots - 2]);
}
}
orders.push_back(uint8_t(nurbs->orderu));
vert_counts.push_back(verts.size() - current_point_count);
orders.push_back(nurbs_orders[i_curve]);
vert_counts.push_back(points.size() + size_adjust);
}
Alembic::AbcGeom::OFloatGeomParam::Sample width_sample;

View File

@ -40,6 +40,7 @@ using Alembic::AbcGeom::ISampleSelector;
using Alembic::AbcGeom::kWrapExisting;
namespace blender::io::alembic {
#pragma optimize("", off)
static int16_t get_curve_resolution(const ICurvesSchema &schema,
const Alembic::Abc::ISampleSelector &sample_sel)
{
@ -338,7 +339,10 @@ void AbcCurveReader::read_curves_sample(Curves *curves_id,
curves.fill_curve_types(data.curve_type);
if (data.curve_type != CURVE_TYPE_POLY) {
curves.resolution_for_write().fill(get_curve_resolution(schema, sample_sel));
int16_t curve_resolution = get_curve_resolution(schema, sample_sel);
if (curve_resolution > 0) {
curves.resolution_for_write().fill(curve_resolution);
}
}
deadpin marked this conversation as resolved Outdated

Typically I've seen the MutableSpan arguments come last compared to the non-mutable arguments. It's arbitrary, but worth being consistent about maybe.

Typically I've seen the `MutableSpan` arguments come last compared to the non-mutable arguments. It's arbitrary, but worth being consistent about maybe.
MutableSpan<float3> curves_positions = curves.positions_for_write();
@ -364,7 +368,7 @@ void AbcCurveReader::read_curves_sample(Curves *curves_id,
for (const int i_curve : curves.curves_range()) {
int position_offset = data.offset_in_alembic[i_curve];
for (const int i_point : curves.points_by_curve()[i_curve]) {
radii.span[i_point] = (*data.radii)[position_offset++];
radii.span[i_point] = (*data.radii)[position_offset++] / 2.0f;
}
}