Page MenuHome

Alembic: new exporter based on the USD exporter structure
Changes PlannedPublic

Authored by Sybren A. Stüvel (sybren) on Fri, May 8, 11:26 AM.

Details

Summary

This patch leverages the AbstractHierarchyIterator to restructure the Alembic exporter. The produced Alembic files have not changed much (details below), as the Alembic writing code has simply been moved from the old exporter to the new. How the export hierarchy is handled changed a lot, though, and also the way in which transforms are computed. As a result, T71395 is fixed.

Differences between the old and new exporter, in terms of the produced Alembic file:

  • The old exporter always exported a mesh object to {object.name}/{object.name}Shape. Now it exports to {object.name}/{mesh.name} instead.
  • Duplicated objects now have a unique numerical suffix.
  • Transforms are always exported as inheriting, because Blender doesn't have a concept of parenting without inheriting the transform.

The AbstractHierarchyIterator (AHI) has changed as well, to support all the features of the Alembic exporter:

  • The Alembic exporter allows different sampling rates for transforms and for shapes. This is necessary, for example, to have control over motion blur: when only using rigid objects, it's enough to export sub-frame samples of transforms. The AHI now has a concept of "export sets" where writers can be grouped into sets and used selectively.
  • The Alembic exporter allows exporting a flattened hierarchy, i.e. with every object a direct child of the top node. The AHI has been adjusted to allow a subclass to exert such control over the exported hierarchy.

Also extracted axis conversion functionality from abc_utils.{h,cc} into abc_axis_conversion.{h,cc}. My longer-term goal is to remove abc_utils.{h,cc} completely, and move its functionality into files with more specific names.

Compared to the old Alembic exporter, Subdivision modifiers are now disabled in a cleaner, more efficient way (they are disabled when exporting with the "Apply Subdivisions" option is unchecked). Previously the exporter would move to a new frame, disable the modifier, evaluate the object, and enable the modifier again. This is now done before exporting starts, and modifiers are only restored when exporting ends.

Some issues with the old Alembic exporter that have NOT been fixed in this patch:

  • Exporting NURBS patches and curves (see T49114 for example).
  • Exporting flattened hierarchy in combination with dupli-objects. This seems to be broken in the old Alembic exporter as well, but hasn't been reported.

There are some changes that I plan to do after this patch is accepted. Most notably, the AbstractHierarchyIterator code is now still part of the USD exporter module (hence also the USD namespace). Since it is meant to be used by more exporters, it should be moved into a generic module, for example io/abstract.

Diff Detail

Repository
rB Blender
Branch
temp-alembic-exporter-D
Build Status
Buildable 7952
Build 7952: arc lint + arc unit

Event Timeline

Sybren A. Stüvel (sybren) requested review of this revision.Fri, May 8, 11:26 AM
Sergey Sharybin (sergey) requested changes to this revision.Fri, May 8, 11:47 AM

Just very quick pass. Is hard to review it. I suggest to split the refactor from actual logic changes.

  • You can easily move those axis conversion to a separate file now, do all the changes in other files. You wouldn't even need a review for that since it's no funcitonal changes.
  • I do see changes in the abstract classes. Those should also be able to happen against the current master without involving a huge amount of the new code.

All the above wouldn't cause functional changes, making it easier to review and look into the code if something unforeseen will happen.

After all the abstract code is refactored, the Alembic part will also become just a simple exercise of porting code to a new basement.

Also, when picking C++ language constructions ask yourself: is it really readable? Can it be done in a more explicit and readable way?

source/blender/io/alembic/intern/abc_axis_conversion.h
27–31

Why not BLI_INLINE ?

source/blender/io/usd/intern/abstract_hierarchy_iterator.h
234–237

Can't say I am happy with this new semantic. You use std::pair<AbstractHierarchyWriter *, bool> here in the function prototype, where it is hard to tell what is this about, but then you use std::tie(writer, created)when you actually use this function.

Seems too much verbosity, without immediate clear reasoning for it.

First thing you can do is to explain in the comment when and why this created is needed. You can also explain that it actually means that when created is false and writer is not nullptr it means that writer is actually usable, is just was creates some time ago.

Second thing you can do is to wrap this into the named struct:

struct WriterInfo {
  AbstractHierarchyWriter* writer;

  // Is set to truth when ensure_writer() did not find existing writer and did create a new one.
  // Is set to false when writer has been re-used or when allocation of the new one has failed.
  bool newly_created;
}

P.S. You might want to have some helper methods there like constructor, maybe cast to bool so you can do if (!writer_info) { return; } and things like that. If you do that, suggest making it a class then.

tests/gtests/alembic/abc_export_test.cc
110

You are mixing doubles and floats.

This revision now requires changes to proceed.Fri, May 8, 11:47 AM

Just to state the obvious, the refactor can also be done in multiple commits. For example, refactor ensure_writer as one change, add determine_graph_index_object as another change.
Some of the stuff will be trivial and will not require review. Some of the stuff would still need to go through review, but will go much faster.

Just very quick pass. Is hard to review it. I suggest to split the refactor from actual logic changes.

👍

  • You can easily move those axis conversion to a separate file now, do all the changes in other files. You wouldn't even need a review for that since it's no funcitonal changes.
  • I do see changes in the abstract classes. Those should also be able to happen against the current master without involving a huge amount of the new code.

I have a bunch of commits in master now for all the little cleanups & safe changes.

Other changes will be offered in separate diffs (D7669, D7670, D7672). Once those are all accepted, I'll update this diff with the remaining changes. Please don't bother with this diff until then ;-)

Sybren A. Stüvel (sybren) planned changes to this revision.Fri, May 8, 5:49 PM