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

Support the new Curves object type in Alembic when exporting curve data.
Make corresponding fixes to importing at the same time.

Summary of changes

  • Exporter now supports the Curves object type during Export including when using the convert to mesh option
  • Exporter will now enforce that only 1 combination of curve type / periodicity are in a given object[0].
  • Catmul-rom basis curves are now supported and will be used for Hair data[1].
  • Bezier curves are exported with their left/right handle data[2].
  • Cyclic bezier and nurbs curves should be handled correctly now.

Along the way this required changes on the Import side as well in order
to be complete

  • Importer will now load bezier curve handle data correctly
  • Importer could fail to set the correct cyclic data on the last spline of a multi-curve object

The following shows a Blender scene with various Curve types and the
result after exporting the scene to Alembic and then re-importing it
back in.

Original Blender scene testing Nurbs(left), Hair(center), and Beziers(right)
original-scene.png

With Alembic exported from Blender 4.1

Blender 4.1 import External software import
blender-load-41.png alembic-load-external-41.png

With Alembic exported from this patch

Blender import with Curve to Mesh GN on the nurbs/bezier External software import
blender-load-patch.png alembic-load-external-patch.png

[0] See https://github.com/alembic/alembic/blob/master/lib/Alembic/AbcGeom/OCurves.h#L55. Prior to this change the Alembic exporter would write invalid data to the file in these cases.
[1] Alembics created with the new Curves Hair object with this patch will not be loadable in prior versions of Blender. The importer does not handle the curve data correctly and will crash.
[2] There was a long-standing TODO about how to handle bezier curve data since nothing is documented on Alembic's side. Bezier data wouldn't even round-trip properly inside Blender itself. On a hunch, and because USD was influenced by certain aspects of Alembic which came before it, I decided to try writing out the data the same way as USD. That turned out to work quite well in at least 1 external software so that's what this patch will use for both import and export.

Support the new Curves object type in Alembic when exporting curve data. Make corresponding fixes to importing at the same time. Summary of changes - Exporter now supports the `Curves` object type during Export including when using the convert to mesh option - Exporter will now enforce that only 1 combination of curve type / periodicity are in a given object[0]. - Catmul-rom basis curves are now supported and will be used for Hair data[1]. - Bezier curves are exported with their left/right handle data[2]. - Cyclic bezier and nurbs curves should be handled correctly now. Along the way this required changes on the Import side as well in order to be complete - Importer will now load bezier curve handle data correctly - Importer could fail to set the correct cyclic data on the last spline of a multi-curve object The following shows a Blender scene with various Curve types and the result after exporting the scene to Alembic and then re-importing it back in. | Original Blender scene testing Nurbs(left), Hair(center), and Beziers(right) | | ---- | | ![original-scene.png](/attachments/22007e1c-37e1-4b7c-8878-7c81dba48335) | #### With Alembic exported from Blender 4.1 | Blender 4.1 import | External software import | | ---- | ---- | | ![blender-load-41.png](/attachments/b76b3e21-2d0c-4f7b-8d21-516408f9a415) | ![alembic-load-external-41.png](/attachments/85b3d335-bf23-4dfa-b791-e7a83d5e47be) | #### With Alembic exported from this patch | Blender import with Curve to Mesh GN on the nurbs/bezier | External software import | | ---- | ---- | | ![blender-load-patch.png](/attachments/6d0df2dc-3027-438e-bf37-73c6b6e61201) | ![alembic-load-external-patch.png](/attachments/0f725897-8297-4b9a-8380-d646427ad204) | [0] See https://github.com/alembic/alembic/blob/master/lib/Alembic/AbcGeom/OCurves.h#L55. Prior to this change the Alembic exporter would write invalid data to the file in these cases. [1] Alembics created with the new Curves Hair object with this patch will not be loadable in prior versions of Blender. The importer does not handle the curve data correctly and will crash. [2] There was a long-standing TODO about how to handle bezier curve data since nothing is documented on Alembic's side. Bezier data wouldn't even round-trip properly inside Blender itself. On a hunch, and because USD was influenced by certain aspects of Alembic which came before it, I decided to try writing out the data the same way as USD. That turned out to work quite well in at least 1 external software so that's what this patch will use for both import and export.
Jesse Yurkovich added 4 commits 2024-03-25 23:03:30 +01:00
Hans Goudey requested changes 2024-03-25 23:22:35 +01:00
Hans Goudey left a comment
Member

Very nice! Thanks for working on this!

Very nice! Thanks for working on this!
@ -42,0 +56,4 @@
case OB_CURVES_LEGACY: {
Curve *cu = static_cast<Curve *>(context->object->data);
resolution_u = cu->resolu;
} break;
Member

The style guide mentions that break is meant to go inside the brackets

The style guide mentions that `break` is meant to go inside the brackets
deadpin marked this conversation as resolved
@ -42,3 +67,3 @@
OCompoundProperty user_props = abc_curve_schema_.getUserProperties();
OInt16Property user_prop_resolu(user_props, ABC_CURVE_RESOLUTION_U_PROPNAME);
user_prop_resolu.set(cu->resolu);
user_prop_resolu.set(resolution_u);
Member

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

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

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?
Member

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.
Author
Member

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?
Member

Yeah a TODO comment is fine IMO

Yeah a TODO comment is fine IMO
deadpin marked this conversation as resolved
@ -57,3 +82,3 @@
void ABCCurveWriter::do_write(HierarchyContext &context)
{
Curve *curve = static_cast<Curve *>(context.object->data);
Curves *curves;
Member

const here would be nice I guess

`const` here would be nice I guess
deadpin marked this conversation as resolved
@ -60,0 +87,4 @@
switch (context.object->type) {
case OB_CURVES_LEGACY: {
const Curve *legacy_curve = static_cast<Curve *>(context.object->data);
converted_curves = std::unique_ptr<Curves, std::function<void(Curves *)>>(
Member

Nitpicky thing but I think a lambda without a capture [](Curves *) { ... } is fine here instead of std::function

Nitpicky thing but I think a lambda without a capture `[](Curves *) { ... }` is fine here instead of `std::function`
Author
Member

What did you mean for this one? I do use a stateless lambda just below this as the actual deleter functor.

What did you mean for this one? I do use a stateless lambda just below this as the actual deleter functor.
Member

Ah I see what you mean. I guess to give it a type this is necessary, I don't run into that much-- my bad!

Ah I see what you mean. I guess to give it a type this is necessary, I don't run into that much-- my bad!
HooglyBoogly marked this conversation as resolved
@ -60,0 +100,4 @@
return;
}
const bke::CurvesGeometry &geometry = curves->geometry.wrap();
Member

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`.
deadpin marked this conversation as resolved
@ -60,0 +118,4 @@
const VArray<bool> cyclic_values = geometry.cyclic();
const bool is_cyclic = cyclic_values[0];
bool all_same_cyclic_type = std::all_of(
Member

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`
Author
Member

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

Huh... I would never have guessed with that name :)
deadpin marked this conversation as resolved
@ -60,0 +131,4 @@
Alembic::AbcGeom::CurveType curve_type = Alembic::AbcGeom::kVariableOrder;
Alembic::AbcGeom::CurvePeriodicity periodicity = is_cyclic ? Alembic::AbcGeom::kPeriodic :
Alembic::AbcGeom::kNonPeriodic;
const int8_t blender_curve_type = geometry.curve_types().first();
Member

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
deadpin marked this conversation as resolved
@ -79,2 +171,3 @@
const size_t current_vert_count = verts.size();
const int totpoint = nurbs->pntsu * nurbs->pntsv;
switch (blender_curve_type) {
Member

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)
Author
Member

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

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);`
deadpin marked this conversation as resolved
@ -105,0 +212,4 @@
verts.push_back(to_yup_V3f(handles_l[start_point_index]));
}
} break;
Member

same break formatting thing here

same break formatting thing here
deadpin marked this conversation as resolved
@ -316,0 +343,4 @@
MutableSpan<float3> positions,
MutableSpan<float3> handles_left,
MutableSpan<float3> handles_right,
const Span<Imath::V3f> alembic_positions)
Member

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.
deadpin marked this conversation as resolved
Jesse Yurkovich added 1 commit 2024-03-26 08:31:13 +01:00
Hans Goudey reviewed 2024-03-26 13:56:43 +01:00
@ -72,0 +162,4 @@
const Span<float> nurbs_weights = curves.nurbs_weights();
const VArray<int8_t> nurbs_orders = curves.nurbs_orders();
const bke::AttributeAccessor curve_attributes = curves.attributes();
const bke::AttributeReader<float> radii = curve_attributes.lookup<float>("radius",
Member

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.
deadpin marked this conversation as resolved
Jesse Yurkovich added 1 commit 2024-03-27 07:27:55 +01:00
Jesse Yurkovich added 1 commit 2024-04-01 21:51:36 +02:00
Hans Goudey approved these changes 2024-04-01 23:46:12 +02:00
@ -60,0 +121,4 @@
}
const VArray<bool> cyclic_values = curves.cyclic();
const bool is_cyclic = cyclic_values.is_empty() ? false : cyclic_values.first();
Member

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.
deadpin marked this conversation as resolved
Hans Goudey reviewed 2024-04-02 03:35:07 +02:00
@ -89,0 +189,4 @@
* 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++) {
Member

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) {`
deadpin marked this conversation as resolved
@ -89,0 +191,4 @@
* ] */
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);
Member

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
deadpin marked this conversation as resolved
Jesse Yurkovich added 1 commit 2024-04-02 06:57:52 +02:00
Jesse Yurkovich added 1 commit 2024-04-05 05:13:20 +02:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
85049316c4
Merge remote-tracking branch 'upstream/main' into fix-alembicexport
Author
Member

@blender-bot build

@blender-bot build
Jesse Yurkovich added 1 commit 2024-04-12 20:35:49 +02:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
92a7ed4057
Merge remote-tracking branch 'upstream/main' into fix-alembicexport
Author
Member

@blender-bot build

@blender-bot build
Jesse Yurkovich merged commit b24ac18130 into main 2024-04-12 21:27:25 +02:00
Jesse Yurkovich deleted branch fix-alembicexport 2024-04-12 21:27:28 +02:00
First-time contributor

I notice that the new exporter's abc file will crash when import to maya
so I check its' attribute in houdini and get something like this( string attribute ):

abc_userProperties = {
	"blender:resolution":[12]
}
abc_userPropertiesMetadata = {
	"blender:resolution":"Int16"
}

After I unpack this abc obj to remove these attribute, then render out a new abc file. The new one will not crash maya

I notice that the new exporter's abc file will crash when import to maya so I check its' attribute in houdini and get something like this( string attribute ): ``` abc_userProperties = { "blender:resolution":[12] } abc_userPropertiesMetadata = { "blender:resolution":"Int16" } ``` After I unpack this abc obj to remove these attribute, then render out a new abc file. The new one will not crash maya
Author
Member

@1029910278 That bug should be reported to Maya. They should not be crashing with that attribute set.

@1029910278 That bug should be reported to Maya. They should not be crashing with that attribute set.
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#119894
No description provided.