Alembic: Support new Curves object type for export #119894
No reviewers
Labels
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#119894
Loading…
Reference in New Issue
No description provided.
Delete Branch "deadpin/blender:fix-alembicexport"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Support the new Curves object type in Alembic when exporting curve data.
Make corresponding fixes to importing at the same time.
Summary of changes
Curves
object type during Export including when using the convert to mesh optionAlong the way this required changes on the Import side as well in order
to be complete
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.
With Alembic exported from Blender 4.1
With Alembic exported from this patch
[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.
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;
The style guide mentions that
break
is meant to go inside the brackets@ -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);
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?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?
Yeah a TODO comment is fine IMO
@ -57,3 +82,3 @@
void ABCCurveWriter::do_write(HierarchyContext &context)
{
Curve *curve = static_cast<Curve *>(context.object->data);
Curves *curves;
const
here would be nice I guess@ -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 *)>>(
Nitpicky thing but I think a lambda without a capture
[](Curves *) { ... }
is fine here instead ofstd::function
What did you mean for this one? I do use a stateless lambda just below this as the actual deleter functor.
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!
@ -60,0 +100,4 @@
return;
}
const bke::CurvesGeometry &geometry = curves->geometry.wrap();
Naming would be more typical if
curves
wascurves_id
andgeometry
wascurves
.@ -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(
const
Also we have a super advanced utility for this :D
array_utils::booleans_mix_calc
Huh... I would never have guessed with that name :)
@ -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();
cast to
CurveType
for compiler warnings in the switch if we ever add a new type@ -79,2 +171,3 @@
const size_t current_vert_count = verts.size();
const int totpoint = nurbs->pntsu * nurbs->pntsv;
switch (blender_curve_type) {
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
andvert_counts
arrays need filled in differently too.Yeah, something like that makes more sense I think.
I wouldn't expect
push_back
to be necessary for the loops at all TBH. Justcopy_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 fromCurvesGeometry
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);
@ -105,0 +212,4 @@
verts.push_back(to_yup_V3f(handles_l[start_point_index]));
}
} break;
same break formatting thing here
@ -316,0 +343,4 @@
MutableSpan<float3> positions,
MutableSpan<float3> handles_left,
MutableSpan<float3> handles_right,
const Span<Imath::V3f> alembic_positions)
Typically I've seen the
MutableSpan
arguments come last compared to the non-mutable arguments. It's arbitrary, but worth being consistent about maybe.@ -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",
I think this should be
lookup_or_default
with a0.01f
default. Also, you can use the*
operator on the returned attribute reader to just deal with the varray.@ -60,0 +121,4 @@
}
const VArray<bool> cyclic_values = curves.cyclic();
const bool is_cyclic = cyclic_values.is_empty() ? false : cyclic_values.first();
Cyclic won't be empty here, since it gives you a varray with a single default value when the attribute doesn't exist.
@ -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++) {
Just noticed, I think this can be simpler as
for (const int point : points) {
@ -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);
Seems the radius should be accessed from the current point rather than always the last point in the curve
@blender-bot build
@blender-bot build
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 ):
After I unpack this abc obj to remove these attribute, then render out a new abc file. The new one will not crash maya
@1029910278 That bug should be reported to Maya. They should not be crashing with that attribute set.