USD: Support armature and shape key export #111931

Merged
Michael Kowalski merged 48 commits from makowalski/blender:usdskel_export_pr into main 2024-01-02 15:51:50 +01:00

New functionality to export armatures and shape keys as USD
skeletons and blend shapes.

NOTE: When exporting skinned meshes or blend shapes, the pre-modified Blender
mesh will be saved, and all modifiers other than the Armature modifier will be ignored.
Please see additional notes in the Limitations section below.

For a description of the UsdSkel API, please see: https://openusd.org/23.05/api/usd_skel_page_front.html

image

  • Added Armature, Only Deform Bones and Shape Key USD export options.
  • Added USDArmatureWriter class.
  • Extended USDMeshWriter to write skinned meshes for binding
    with skeletons and 'neutral' meshes with blend shape targets.
    Specifically, when exporting an armature, a skinned mesh is written
    in its pre-modified rest position. When exporting to blend shapes,
    a mesh with shape keys is saved with its vertices in the shape key
    basis shape position.
  • Added USDHierarchyIterator::process_usd_skel() function to
    finish processing skeleton and blend shape export after the
    writer instances completed writing. This is necessary because
    some of the export operations require processing multiple prims
    at once.
  • Extended USDTransformWriter::do_write() to write transforms
    sparsely, to avoid saving redundant transform values when exporting
    armatures.
  • Added a create_skel_roots() function, called on the stage at the
    end of the export. This function attempts to ensure that skinned
    prims and skeletons are encapsulated under SkelRoot primitives,
    which is required in USD for correct skinning behavior. Please see
    https://openusd.org/23.05/api/class_usd_skel_root.html#details

Challenges

  • When exporting blend shape animations for multiple meshes bound
    to a single skeleton, we need to merge the blend shape time samples
    of the different meshes into a single animation primitive at the end
    of the export. This requires some tricky book keeping, where the weight
    time samples for a given mesh are initially saved by the mesh writer to a
    temporary attribute on the mesh and are later copied to the animation
    primitive as one of the final steps.

Limitations

  • When writing blend shapes and skinned meshes, the pre-modified mesh
    is exported. This is to ensure that the number of blend shape offsets
    matches the number of points, and so that the skinned mesh is saved in
    its rest position.
  • Because the pre-modified mesh must be exported, modifiers in addition
    to Armature modifiers will not be applied. This still allows the round trip
    UsdSkel -> Blender -> UsdSkel, but some additional setup might be required
    to export to UsdSkel when there are multiple modifiers (for example, applying
    mirroring modifiers that precede the armature modifier).
  • Exporting bendy bones or absolute shape keys isn't currently supported.

Testing

Load the attached robot_walk.blend and blendshape_face.blend files, export to USD and load the saved file in USDView. You can also test by re-importing UsdSkel back into Blender and verifying that the armatures and shape keys were preserved in the round trip. I hope to add more tests soon.

The attached rig_simpe.blend contains a mixture of deform and non-dform bones in the rig and can be used to test the Only Defom Bones export option.

New functionality to export armatures and shape keys as USD skeletons and blend shapes. **NOTE:** When exporting skinned meshes or blend shapes, the pre-modified Blender mesh will be saved, and all modifiers other than the Armature modifier will be ignored. Please see additional notes in the Limitations section below. For a description of the `UsdSkel` API, please see: https://openusd.org/23.05/api/usd_skel_page_front.html ![image](/attachments/e534532e-4be4-47ae-8a32-eac3da62860d) - Added `Armature`, `Only Deform Bones` and `Shape Key` USD export options. - Added `USDArmatureWriter` class. - Extended `USDMeshWriter` to write skinned meshes for binding with skeletons and 'neutral' meshes with blend shape targets. Specifically, when exporting an armature, a skinned mesh is written in its pre-modified rest position. When exporting to blend shapes, a mesh with shape keys is saved with its vertices in the shape key `basis` shape position. - Added `USDHierarchyIterator::process_usd_skel()` function to finish processing skeleton and blend shape export after the writer instances completed writing. This is necessary because some of the export operations require processing multiple prims at once. - Extended `USDTransformWriter::do_write()` to write transforms sparsely, to avoid saving redundant transform values when exporting armatures. - Added a `create_skel_roots()` function, called on the stage at the end of the export. This function attempts to ensure that skinned prims and skeletons are encapsulated under `SkelRoot` primitives, which is required in USD for correct skinning behavior. Please see https://openusd.org/23.05/api/class_usd_skel_root.html#details ### Challenges - When exporting blend shape animations for multiple meshes bound to a single skeleton, we need to merge the blend shape time samples of the different meshes into a single animation primitive at the end of the export. This requires some tricky book keeping, where the weight time samples for a given mesh are initially saved by the mesh writer to a temporary attribute on the mesh and are later copied to the animation primitive as one of the final steps. ### Limitations - When writing blend shapes and skinned meshes, the pre-modified mesh is exported. This is to ensure that the number of blend shape offsets matches the number of points, and so that the skinned mesh is saved in its rest position. - Because the pre-modified mesh must be exported, modifiers in addition to Armature modifiers will not be applied. This still allows the round trip UsdSkel -> Blender -> UsdSkel, but some additional setup might be required to export to UsdSkel when there are multiple modifiers (for example, applying mirroring modifiers that precede the armature modifier). - Exporting bendy bones or absolute shape keys isn't currently supported. ### Testing Load the attached `robot_walk.blend` and `blendshape_face.blend` files, export to USD and load the saved file in `USDView`. You can also test by re-importing UsdSkel back into Blender and verifying that the armatures and shape keys were preserved in the round trip. I hope to add more tests soon. The attached `rig_simpe.blend` contains a mixture of deform and non-dform bones in the rig and can be used to test the `Only Defom Bones` export option.
Michael Kowalski added 1 commit 2023-09-04 17:27:46 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
51585227e3
USD: Support armature and shape key export.
New functionality to export armatures and shape keys as USD
skeletons and blend shapes.

- Added Armature and Shape Key USD export options.
- Added USDArmatureWriter class.
- Extended USDMeshWriter to write skinned meshes for binding
with skeletons and 'neutral' meshes with blend shape targets.
- Added ModifierDisabler class to Temporarily disable armature
modifiers on mesh objects, so that meshes can be exported in
their rest position
- Added USDHierarchyIterator::process_usd_skel() function to
finish processing skeleton and blend shape export after the
writer instances completed writing.
Michael Kowalski changed title from USD: Support armature and shape key export. to USD: Support armature and shape key export 2023-09-04 17:31:21 +02:00
Author
Member

@blender-bot build

@blender-bot build
Michael Kowalski added 1 commit 2023-09-04 20:29:19 +02:00
Michael Kowalski added the
Interest
Pipeline, Assets & IO
Interest
USD
labels 2023-09-04 21:36:35 +02:00
Michael Kowalski added this to the USD project 2023-09-04 21:36:44 +02:00
Michael Kowalski added 2 commits 2023-09-06 02:18:54 +02:00
ee1230820c USD export: error merging blend shape weights.
Fixed error generating unique blend shape names when merging
the weights time samples.
Michael Kowalski added 1 commit 2023-09-06 04:55:54 +02:00
c359e397e0 USD: fix USD path in export map.
Now setting the correct USD prim path in the export map, to ensure
the path includes the user-specified root prim.  Also, printing
the USD path in warnings if the prim specified in the export map
can't be found on the stage.
Michael Kowalski added 1 commit 2023-09-06 20:54:27 +02:00
a7735cdc15 USD skel export: remove temp weights primvar.
Removing temporary weights primvar time samples from
blendshape mesh.
Author
Member

FYI, I'm fixing an issue exporting meshes in their rest-poses. I'll update this PR when I've addressed this and committed the fix.

FYI, I'm fixing an issue exporting meshes in their rest-poses. I'll update this PR when I've addressed this and committed the fix.
Michael Kowalski added 4 commits 2023-09-09 00:28:55 +02:00
a8e7db8c9d USD: Use pre-modified mesh for skinned meshes.
Now using the pre-modified mesh to export the mesh
in its rest pose.  This replaces the use of the
ModifierDisabler class, which was causing some
animation data to be dropped.  Using the modifier
disabler as an approach could be revisited in the
future.  Also, moved the mesh creation function for
blend shapes has been moved to a new
get_shape_key_basis_mesh() function.
Michael Kowalski added 1 commit 2023-09-10 22:08:17 +02:00
8bf6d933e0 USD: Skel export fixes.
Skip exporting attributes that correspond to armature modifier
vertex groups.

No longer exporting mesh with UsdSkel bindings if the armature
modifier on the mesh object is disabled.

Move relevant function to the usd_armature_utils files.
Hans Goudey requested changes 2023-09-10 22:51:18 +02:00
Hans Goudey left a comment
Member

This is really not my area of expertise, so I didn't look at the logic much. But I read through the PR anyway and left some code style comments, maybe that's helpful as a first round of review.

This is really not my area of expertise, so I didn't look at the logic much. But I read through the PR anyway and left some code style comments, maybe that's helpful as a first round of review.
@ -0,0 +14,4 @@
#include "WM_api.hh"
/* Recursively invoke the 'visitor' function on the given bone and its children. */
static void visit_bones(const Bone *bone, std::function<void(const Bone *)> visitor)
Member

For clarity, usually FunctionRef is used instead of std::function for a callback like this (where no persistent storage is necessary). It can even be significantly faster (see 25917f0165), though I doubt it matters here.

For clarity, usually `FunctionRef` is used instead of `std::function` for a callback like this (where no persistent storage is necessary). It can even be significantly faster (see 25917f0165b5feea6f04c6df21bb0fd2003b1042), though I doubt it matters here.
makowalski marked this conversation as resolved
@ -0,0 +22,4 @@
visitor(bone);
for (Bone *child = (Bone *)bone->childbase.first; child; child = child->next) {
Member

Use LISTBASE_FOREACH

Use `LISTBASE_FOREACH`
makowalski marked this conversation as resolved
@ -0,0 +29,4 @@
/* Return the armature modifier on the given object. Return null if no
* armature modifier can be found. */
static ArmatureModifierData *get_armature_modifier(const Object *obj)
Member

Best if you can return const ArmatureModifierData * to avoid retrieving a mutable modifier from a const object. Same below.

Best if you can return `const ArmatureModifierData *` to avoid retrieving a mutable modifier from a const object. Same below.
makowalski marked this conversation as resolved
@ -0,0 +59,4 @@
return BKE_modifier_is_enabled(scene, mod, mode) ? mod : nullptr;
}
namespace blender::io::usd {
Member

Any particular reason the namespace starts here instead of at the top of the file?

Any particular reason the namespace starts here instead of at the top of the file?
makowalski marked this conversation as resolved
@ -0,0 +74,4 @@
}
}
void get_armature_bone_names(const Object *ob_arm, std::vector<std::string> &r_names)
Member

Use Vector instead of std::vector

Use `Vector` instead of `std::vector`
makowalski marked this conversation as resolved
@ -0,0 +77,4 @@
void get_armature_bone_names(const Object *ob_arm, std::vector<std::string> &r_names)
{
auto visitor = [&r_names](const Bone *bone) {
if (bone) {
Member

Based on the places the visitor is used, it seems the null check isn't necessary here.

Based on the places the visitor is used, it seems the null check isn't necessary here.
makowalski marked this conversation as resolved
@ -0,0 +147,4 @@
return false;
}
bArmature *arm = (bArmature *)arm_mod->object->data;
Member
Type cast style (https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast)
makowalski marked this conversation as resolved
@ -0,0 +78,4 @@
* Attempt to add the given name to the 'names' set as a unique entry, modifying
* the name with a numerical suffix if necessary, and return the unique name that
* was added to the set. */
std::string add_unique_name(std::set<std::string> &names, const std::string &name)
Member

Use Set instead of std::set

Use `Set` instead of `std::set`
makowalski marked this conversation as resolved
@ -0,0 +188,4 @@
const Key *get_mesh_shape_key(const Object *obj)
{
if (!obj || !obj->data || obj->type != OB_MESH) {
Member

Looking at the !obj check here, seems it's a bit overly defensive. This sort of check should probably happen at a higher level.

Looking at the `!obj` check here, seems it's a bit overly defensive. This sort of check should probably happen at a higher level.
makowalski marked this conversation as resolved
@ -0,0 +239,4 @@
}
if (kb == basis_key) {
// Skip the basis.
Member

Comment style

Comment style
makowalski marked this conversation as resolved
@ -0,0 +401,4 @@
return;
}
std::vector<BlendShapeMergeInfo> merge_info;
Member

Like mentioned above, blender::Vector should typically be the standard choice for a growable array in Blender code, mainly for the inline buffer, but also for consistency.

Obviously if a library requires a std::vector argument that doesn't apply though.

(Same with Set below, and Map vs std::map elsewhere in this PR).

Like mentioned above, `blender::Vector` should typically be the standard choice for a growable array in Blender code, mainly for the inline buffer, but also for consistency. Obviously if a library requires a `std::vector` argument that doesn't apply though. (Same with `Set` below, and `Map` vs `std::map` elsewhere in this PR).
makowalski marked this conversation as resolved
@ -0,0 +534,4 @@
}
/* Make a copy of the mesh so we can update the verts to the basis shape. */
Mesh *temp_mesh = reinterpret_cast<Mesh *>(
Member

BKE_mesh_copy_for_eval is a slightly friendlier looking way of doing thing

`BKE_mesh_copy_for_eval` is a slightly friendlier looking way of doing thing
makowalski marked this conversation as resolved
@ -1038,0 +1241,4 @@
const pxr::UsdSkelBindingAPI &skel_api,
const std::vector<std::string> &bone_names)
{
if (!mesh || !skel_api) {
Member

Same comment here about defensive null checks

Same comment here about defensive null checks
makowalski marked this conversation as resolved
@ -1038,0 +1252,4 @@
std::vector<int> joint_index;
/* Build the index mapping. */
for (const bDeformGroup *def = (const bDeformGroup *)mesh->vertex_group_names.first; def;
Member

LISTBASE_FOREACH

`LISTBASE_FOREACH`
makowalski marked this conversation as resolved
@ -1038,0 +1271,4 @@
return;
}
const blender::Span<MDeformVert> dverts = mesh->deform_verts();
Member

Since this code is already in the blender:: namespace, specifying it here for Span is unnecessary.

Since this code is already in the `blender::` namespace, specifying it here for `Span` is unnecessary.
makowalski marked this conversation as resolved
@ -1038,0 +1295,4 @@
/* Record number of out of bounds vert group indices, for error reporting. */
int num_out_of_bounds = 0;
for (const int i : dverts.index_range()) {
Member

This looks like an expensive loop relative to the others. Maybe worth parallelizing with threading::parallel_for?

This looks like an expensive loop relative to the others. Maybe worth parallelizing with `threading::parallel_for`?
Author
Member

This is definitely something to consider. However, I'm thinking that perhaps such an optimization could be done in a new follow-up PR, to simplify debugging and testing initially. Would that be okay?

This is definitely something to consider. However, I'm thinking that perhaps such an optimization could be done in a new follow-up PR, to simplify debugging and testing initially. Would that be okay?
HooglyBoogly marked this conversation as resolved
@ -1038,0 +1304,4 @@
for (int j = 0; j < element_size; ++j, ++offset) {
if (offset >= joint_indices.size()) {
printf("Programmer error: out of bounds joint indices array offset.\n");
Member

Usually an assert (BLI_assert) is a more useful way to check for this sort of thing, since execution will stop in debug builds (if this really is a programmer error).

Usually an assert (`BLI_assert`) is a more useful way to check for this sort of thing, since execution will stop in debug builds (if this really is a programmer error).
makowalski marked this conversation as resolved
@ -1038,0 +1315,4 @@
int def_nr = static_cast<int>(vert.dw[j].def_nr);
/* This out of bounds check is necessary because MDeformVert.totweight can be
* larger than the number of bDeformGroup structs in Object.defbase. It appears to be
Member

Object.defbase probably refers to long ago when the names were stored on the object. Nowadays the names are stored on the mesh.

`Object.defbase` probably refers to long ago when the names were stored on the object. Nowadays the names are stored on the mesh.
makowalski marked this conversation as resolved
Michael Kowalski added 8 commits 2023-09-14 08:53:03 +02:00
5d1b7edb04 USD: skip skel namespace attrib export.
No longer saving attributes with the "skel:" namespace when
exporting armature and shape keys to UsdSkel.
763c31185b USD: no skel export with multiple modifiers.
Changed the logic so that if a mesh has modifiers in
addition to the armature modifier, the mesh animation is
saved as a point cache.
d69d458b1e USD: verifying modifiers are enabled.
Now exporting skinning data only if armature modifier is
enabled.
e62351d391 USD skel export: addressing review comments.
Updates based on suggestions by Hans.
0759a965f9 USD: Call UsdSkelNormalizeWeights.
Replaced custom code with USD library call.
buildbot/vexp-code-patch-coordinator Build done. Details
ebae8974a6
USD skel: added asserts and simplified code.
Author
Member

@blender-bot build

@blender-bot build
Author
Member

I've made some updates and I believe this patch if ready for further review. The recent changes include:

  • Fixed issue saving the skinned mesh in its rest pose.
  • Ensuring that disabled armature modifiers are ignored.
  • Now disallowing export of skinned meshes and blend shapes if the mesh has modifiers in addition to the armature modifier. Meshes with additional modifiers will be saved as point caches by default. Please see the discussion in the Limitations section.
I've made some updates and I believe this patch if ready for further review. The recent changes include: - Fixed issue saving the skinned mesh in its rest pose. - Ensuring that disabled armature modifiers are ignored. - Now disallowing export of skinned meshes and blend shapes if the mesh has modifiers in addition to the armature modifier. Meshes with additional modifiers will be saved as point caches by default. Please see the discussion in the Limitations section.
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR111931) when ready.
Hans Goudey approved these changes 2023-09-14 16:47:16 +02:00
Hans Goudey left a comment
Member

Someone with more experience with USD and Blender animation should think about the design decisions here. I left another set of code style comments, I doubt I'll have more after this round though. I'm accepting this now, because each of my comments is straightforward, and I don't think I need to look at them again.

If there is an attribute with the same name, remove it is the only larger point I see. I guess that's wrapped up in some existing design discussions.

Someone with more experience with USD and Blender animation should think about the design decisions here. I left another set of code style comments, I doubt I'll have more after this round though. I'm accepting this now, because each of my comments is straightforward, and I don't think I need to look at them again. `If there is an attribute with the same name, remove it` is the only larger point I see. I guess that's wrapped up in some existing design discussions.
@ -0,0 +16,4 @@
namespace blender::io::usd {
/* Recursively invoke the 'visitor' function on the given bone and its children. */
static void visit_bones(const Bone *bone, blender::FunctionRef<void(const Bone *)> visitor)
Member

blender::FunctionRef -> FunctionRef

`blender::FunctionRef` -> `FunctionRef`
makowalski marked this conversation as resolved
@ -0,0 +62,4 @@
static const ArmatureModifierData *get_armature_modifier(const Object *obj,
const Depsgraph *depsgraph)
{
BLI_assert(obj);
Member

Seems like instead of asserting that the object is non-null, the argument could be a reference const Object &

Seems like instead of asserting that the object is non-null, the argument could be a reference `const Object &`
makowalski marked this conversation as resolved
@ -0,0 +130,4 @@
const char *name,
const Depsgraph *depsgraph)
{
if (!obj || !name) {
Member

Same comment here about the null checks

  • const Object *obj -> const Object &obj
  • const char *name -> const StringRefNull name
Same comment here about the null checks - `const Object *obj` -> `const Object &obj` - `const char *name` -> `const StringRefNull name`
makowalski marked this conversation as resolved
@ -0,0 +153,4 @@
return mods.size() == 1 && mods.first()->type == eModifierType_Armature;
}
Vector<ModifierData *> get_enabled_modifiers(const Object *obj, const Depsgraph *depsgraph)
Member

Vector<ModifierData *> -> Vector<const ModifierData *>

`Vector<ModifierData *>` -> `Vector<const ModifierData *>`
makowalski marked this conversation as resolved
@ -0,0 +273,4 @@
/* Subtract the key positions from the
* basis positions to get the offsets. */
sub_v3_v3v3(offsets[i].data(), fp[i], basis_fp[i]);
indices[i] = i;
Member

The indices can be filled outside of this loop with std::iota(indices.begin(), indices.end(), 0); That means this loop is only doing one thing

The indices can be filled outside of this loop with `std::iota(indices.begin(), indices.end(), 0);` That means this loop is only doing one thing
makowalski marked this conversation as resolved
@ -0,0 +407,4 @@
Vector<BlendShapeMergeInfo> merge_info;
/* We are merging blend shape names and weighs from multiple
Member

weighs -> weights

`weighs` -> `weights`
makowalski marked this conversation as resolved
@ -0,0 +521,4 @@
/* If we're exporting blend shapes, we export the unmodified mesh with
* the verts in the basis key positions. */
Mesh *mesh = BKE_object_get_pre_modified_mesh(obj);
Member

Making this mesh const would clarify the code below.

Making this mesh `const` would clarify the code below.
makowalski marked this conversation as resolved
@ -0,0 +3,4 @@
* SPDX-License-Identifier: GPL-2.0-or-later */
#pragma once
#include <map>
Member

Maybe these should be the corresponding Blender container headers now?

Maybe these should be the corresponding Blender container headers now?
makowalski marked this conversation as resolved
@ -8,2 +8,4 @@
#include "usd_exporter_context.h"
#include "usd_skel_convert.h"
#include <map>
Member

Different headers here too?

Different headers here too?
makowalski marked this conversation as resolved
@ -1009,1 +1063,4 @@
if (!BKE_object_defgroup_find_name(mesh_obj, joint_name.c_str())) {
/* If there is an attribute with the same name, remove it. */
bke::AttributeIDRef attr_id(joint_name);
if (attributes.contains(attr_id)) {
Member

If there is an attribute with the same name, remove it I guess this is similar to the other PR handling this, or that one will replace this?

`If there is an attribute with the same name, remove it` I guess this is similar to the other PR handling this, or that one will replace this?
Author
Member

I have removed this change, as it's no longer needed.

I have removed this change, as it's no longer needed.
makowalski marked this conversation as resolved
@ -1038,0 +1174,4 @@
void shape_key_export_chaser(pxr::UsdStageRefPtr stage,
const ObjExportMap &shape_key_mesh_export_map)
{
std::map<pxr::SdfPath, pxr::SdfPathSet> skel_to_mesh;
Member

std::map to Map here too, might as well do this consistently. Let me know if you have questions about the API of Map. Overall it should hopefully be more intuitive than std::map though

`std::map` to `Map` here too, might as well do this consistently. Let me know if you have questions about the API of `Map`. Overall it should hopefully be more intuitive than `std::map` though
makowalski marked this conversation as resolved
@ -0,0 +5,4 @@
#include "usd_writer_abstract.h"
#include <string>
Member

Unnecessary includes here?

Unnecessary includes here?
makowalski marked this conversation as resolved

getting these warnings when building the branch:

[2179/2345] Building CXX object source/blender/io/usd/CMakeFiles/bf_usd.dir/intern/usd_blend_shape_utils.cc.o
/home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:100:6: warning: no previous declaration for ‘void blender::io::usd::ensure_blend_shape_skeleton(pxrInternal_v0_23__pxrReserved__::UsdStageRefPtr, pxrInternal_v0_23__pxrReserved__::UsdPrim&)’ [-Wmissing-declarations]
  100 | void ensure_blend_shape_skeleton(pxr::UsdStageRefPtr stage, pxr::UsdPrim &mesh_prim)
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:191:12: warning: no previous declaration for ‘const Key* blender::io::usd::get_mesh_shape_key(const Object*)’ [-Wmissing-declarations]
  191 | const Key *get_mesh_shape_key(const Object *obj)
      |            ^~~~~~~~~~~~~~~~~~
/home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:204:6: warning: no previous declaration for ‘bool blender::io::usd::is_mesh_with_shape_keys(const Object*)’ [-Wmissing-declarations]
  204 | bool is_mesh_with_shape_keys(const Object *obj)
      |      ^~~~~~~~~~~~~~~~~~~~~~~
/home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:210:6: warning: no previous declaration for ‘void blender::io::usd::create_blend_shapes(pxrInternal_v0_23__pxrReserved__::UsdStageRefPtr, const Object*, const pxrInternal_v0_23__pxrReserved__::UsdPrim&)’ [-Wmissing-declarations]
  210 | void create_blend_shapes(pxr::UsdStageRefPtr stage,
      |      ^~~~~~~~~~~~~~~~~~~
/home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:303:19: warning: no previous declaration for ‘pxrInternal_v0_23__pxrReserved__::VtFloatArray blender::io::usd::get_blendshape_weights(const Key*)’ [-Wmissing-declarations]
  303 | pxr::VtFloatArray get_blendshape_weights(const Key *key)
      |                   ^~~~~~~~~~~~~~~~~~~~~~
/home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:320:6: warning: no previous declaration for ‘bool blender::io::usd::has_animated_mesh_shape_key(const Object*)’ [-Wmissing-declarations]
  320 | bool has_animated_mesh_shape_key(const Object *obj)
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:327:19: warning: no previous declaration for ‘pxrInternal_v0_23__pxrReserved__::VtTokenArray blender::io::usd::get_blend_shape_names(const Key*)’ [-Wmissing-declarations]
  327 | pxr::VtTokenArray get_blend_shape_names(const Key *key)
      |                   ^~~~~~~~~~~~~~~~~~~~~
/home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:354:19: warning: no previous declaration for ‘pxrInternal_v0_23__pxrReserved__::VtTokenArray blender::io::usd::get_blend_shapes_attr_value(const pxrInternal_v0_23__pxrReserved__::UsdPrim&)’ [-Wmissing-declarations]
  354 | pxr::VtTokenArray get_blend_shapes_attr_value(const pxr::UsdPrim &mesh_prim)
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:382:6: warning: no previous declaration for ‘void blender::io::usd::remap_blend_shape_anim(pxrInternal_v0_23__pxrReserved__::UsdStageRefPtr, const pxrInternal_v0_23__pxrReserved__::SdfPath&, const pxrInternal_v0_23__pxrReserved__::SdfPathSet&)’ [-Wmissing-declarations]
  382 | void remap_blend_shape_anim(pxr::UsdStageRefPtr stage,
      |      ^~~~~~~~~~~~~~~~~~~~~~
/home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:516:7: warning: no previous declaration for ‘Mesh* blender::io::usd::get_shape_key_basis_mesh(Object*)’ [-Wmissing-declarations]
  516 | Mesh *get_shape_key_basis_mesh(Object *obj)
      |       ^~~~~~~~~~~~~~~~~~~~~~~~
[2200/2345] Building CXX object source/blender/io/usd/CMakeFiles/bf_usd.dir/intern/usd_skel_root_utils.cc.o
/home/guest/blender/src/source/blender/io/usd/intern/usd_skel_root_utils.cc: In function ‘void blender::io::usd::create_skel_roots(pxrInternal_v0_23__pxrReserved__::UsdStageRefPtr, const USDExportParams&)’:
/home/guest/blender/src/source/blender/io/usd/intern/usd_skel_root_utils.cc:115:18: warning: format ‘%s’ expects a matching ‘char*’ argument [-Wformat=]
  115 |                  "%s: Couldn't find a common Xform ancestor for skinned prim %s "
      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  116 |                  "and skeleton %s to convert to a USD SkelRoot. "
      |                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  117 |                  "This can be addressed by setting a root primitive in the export options.\n",
      |                  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[2207/2345] Building CXX object source/blender/io/usd/CMakeFiles/bf_usd.dir/intern/usd_writer_mesh.cc.o
/home/guest/blender/src/source/blender/io/usd/intern/usd_writer_mesh.cc: In member function ‘void blender::io::usd::USDMeshWriter::add_shape_key_weights_sample(const Object*)’:
/home/guest/blender/src/source/blender/io/usd/intern/usd_writer_mesh.cc:969:16: warning: format ‘%s’ expects a matching ‘char*’ argument [-Wformat=]
  969 |                "%s: couldn't create primvar %s on prim %s\n",
      |                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[2343/2345] Linking CXX executable bin/blender
getting these warnings when building the branch: ```lines=10 [2179/2345] Building CXX object source/blender/io/usd/CMakeFiles/bf_usd.dir/intern/usd_blend_shape_utils.cc.o /home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:100:6: warning: no previous declaration for ‘void blender::io::usd::ensure_blend_shape_skeleton(pxrInternal_v0_23__pxrReserved__::UsdStageRefPtr, pxrInternal_v0_23__pxrReserved__::UsdPrim&)’ [-Wmissing-declarations] 100 | void ensure_blend_shape_skeleton(pxr::UsdStageRefPtr stage, pxr::UsdPrim &mesh_prim) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:191:12: warning: no previous declaration for ‘const Key* blender::io::usd::get_mesh_shape_key(const Object*)’ [-Wmissing-declarations] 191 | const Key *get_mesh_shape_key(const Object *obj) | ^~~~~~~~~~~~~~~~~~ /home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:204:6: warning: no previous declaration for ‘bool blender::io::usd::is_mesh_with_shape_keys(const Object*)’ [-Wmissing-declarations] 204 | bool is_mesh_with_shape_keys(const Object *obj) | ^~~~~~~~~~~~~~~~~~~~~~~ /home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:210:6: warning: no previous declaration for ‘void blender::io::usd::create_blend_shapes(pxrInternal_v0_23__pxrReserved__::UsdStageRefPtr, const Object*, const pxrInternal_v0_23__pxrReserved__::UsdPrim&)’ [-Wmissing-declarations] 210 | void create_blend_shapes(pxr::UsdStageRefPtr stage, | ^~~~~~~~~~~~~~~~~~~ /home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:303:19: warning: no previous declaration for ‘pxrInternal_v0_23__pxrReserved__::VtFloatArray blender::io::usd::get_blendshape_weights(const Key*)’ [-Wmissing-declarations] 303 | pxr::VtFloatArray get_blendshape_weights(const Key *key) | ^~~~~~~~~~~~~~~~~~~~~~ /home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:320:6: warning: no previous declaration for ‘bool blender::io::usd::has_animated_mesh_shape_key(const Object*)’ [-Wmissing-declarations] 320 | bool has_animated_mesh_shape_key(const Object *obj) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:327:19: warning: no previous declaration for ‘pxrInternal_v0_23__pxrReserved__::VtTokenArray blender::io::usd::get_blend_shape_names(const Key*)’ [-Wmissing-declarations] 327 | pxr::VtTokenArray get_blend_shape_names(const Key *key) | ^~~~~~~~~~~~~~~~~~~~~ /home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:354:19: warning: no previous declaration for ‘pxrInternal_v0_23__pxrReserved__::VtTokenArray blender::io::usd::get_blend_shapes_attr_value(const pxrInternal_v0_23__pxrReserved__::UsdPrim&)’ [-Wmissing-declarations] 354 | pxr::VtTokenArray get_blend_shapes_attr_value(const pxr::UsdPrim &mesh_prim) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ /home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:382:6: warning: no previous declaration for ‘void blender::io::usd::remap_blend_shape_anim(pxrInternal_v0_23__pxrReserved__::UsdStageRefPtr, const pxrInternal_v0_23__pxrReserved__::SdfPath&, const pxrInternal_v0_23__pxrReserved__::SdfPathSet&)’ [-Wmissing-declarations] 382 | void remap_blend_shape_anim(pxr::UsdStageRefPtr stage, | ^~~~~~~~~~~~~~~~~~~~~~ /home/guest/blender/src/source/blender/io/usd/intern/usd_blend_shape_utils.cc:516:7: warning: no previous declaration for ‘Mesh* blender::io::usd::get_shape_key_basis_mesh(Object*)’ [-Wmissing-declarations] 516 | Mesh *get_shape_key_basis_mesh(Object *obj) | ^~~~~~~~~~~~~~~~~~~~~~~~ [2200/2345] Building CXX object source/blender/io/usd/CMakeFiles/bf_usd.dir/intern/usd_skel_root_utils.cc.o /home/guest/blender/src/source/blender/io/usd/intern/usd_skel_root_utils.cc: In function ‘void blender::io::usd::create_skel_roots(pxrInternal_v0_23__pxrReserved__::UsdStageRefPtr, const USDExportParams&)’: /home/guest/blender/src/source/blender/io/usd/intern/usd_skel_root_utils.cc:115:18: warning: format ‘%s’ expects a matching ‘char*’ argument [-Wformat=] 115 | "%s: Couldn't find a common Xform ancestor for skinned prim %s " | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 116 | "and skeleton %s to convert to a USD SkelRoot. " | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 117 | "This can be addressed by setting a root primitive in the export options.\n", | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [2207/2345] Building CXX object source/blender/io/usd/CMakeFiles/bf_usd.dir/intern/usd_writer_mesh.cc.o /home/guest/blender/src/source/blender/io/usd/intern/usd_writer_mesh.cc: In member function ‘void blender::io::usd::USDMeshWriter::add_shape_key_weights_sample(const Object*)’: /home/guest/blender/src/source/blender/io/usd/intern/usd_writer_mesh.cc:969:16: warning: format ‘%s’ expects a matching ‘char*’ argument [-Wformat=] 969 | "%s: couldn't create primvar %s on prim %s\n", | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [2343/2345] Linking CXX executable bin/blender ```

Also, trying to use this code in a heavy Blender Studio production file (from the Pets project), got two crashes in a debug build, both only indirectly related to USD code itself:

  • It looks like depsgraph update is ending up calling BKE_view_layer_synced_ensure() on main (orig) data, from within a job thread. Reported as #112534. In the mean time, it may be a good idea to move depsgraph building for USD exporter to the main thread (instead of doing it in the job one)?
  • The infamous WM_report/WM_timer issue is back at it again (see #105369). This needs to be re-opened/re-investigated, it's still crashing in several ways (which is not really unexpected, given how unthreadsafe it is!). See #112537
Also, trying to use this code in a heavy Blender Studio production file (from the Pets project), got two crashes in a debug build, both only indirectly related to USD code itself: * It looks like depsgraph update is ending up calling `BKE_view_layer_synced_ensure()` on main (orig) data, from within a job thread. Reported as #112534. In the mean time, it may be a good idea to move depsgraph building for USD exporter to the main thread (instead of doing it in the job one)? * The infamous WM_report/WM_timer issue is back at it again (see #105369). This needs to be re-opened/re-investigated, it's still crashing in several ways (which is not really unexpected, given how unthreadsafe it is!). See #112537

Would also like to remind here about what we discussed during the last module meeting regarding of handling of 'raw' mesh data vs. fully evaluated one:

  • By default rigged objects should be exported as usdSkel in USD.
    • This implies that if the object has a complex modifier stack, this will be discarded, and they only get the 'raw' mesh exported.
  • Users should have the opt-in option to instead export their animated character as a baked point cache, if they need to keep all modifiers' stack effects in end result.

I do not think that this decision should be made automatically, pointcaches are fully destructive, and potentially very heavy.

Would also like to remind here about what we discussed during the last [module meeting](https://devtalk.blender.org/t/2023-09-14-pipeline-assets-i-o-meeting/30919) regarding of handling of 'raw' mesh data vs. fully evaluated one: * By default rigged objects should be exported as usdSkel in USD. * This implies that if the object has a complex modifier stack, this will be discarded, and they only get the 'raw' mesh exported. * Users should have the opt-in option to instead export their animated character as a baked point cache, if they need to keep all modifiers' stack effects in end result. I do not think that this decision should be made automatically, pointcaches are fully destructive, and potentially very heavy.
Author
Member

Also, trying to use this code in a heavy Blender Studio production file (from the Pets project)

@mont29 Is this file available for testing, by any chance? I understand that it might not be publicly available, so no worries if it isn't.

> Also, trying to use this code in a heavy Blender Studio production file (from the Pets project) @mont29 Is this file available for testing, by any chance? I understand that it might not be publicly available, so no worries if it isn't.

@makowalski these files are not secret anymore, but not sure if I can give you easily access to them... There is one publicly available, cannot say if it's reproducible with this one too (am on holidays rn ;) ).

Note again that the issues am describing are not directly related to USD, there are more revealed by it. I would not consider them a blocker for this patch.

@makowalski these files are not secret anymore, but not sure if I can give you easily access to them... There is one [publicly available](https://studio.blender.org/films/wing-it/?asset=6953), cannot say if it's reproducible with this one too (am on holidays rn ;) ). Note again that the issues am describing are not directly related to USD, there are more revealed by it. I would not consider them a blocker for this patch.
Michael Kowalski added 2 commits 2023-12-01 21:36:17 +01:00
1d42747452 Merge branch 'main' into usdskel_export_pr
This commit will not compile as it has unresolved
merge conflicts.
c2a8378675 USD: fix merge conflicts.
Fixed merge conflicts and build errors introduced
by previous commit which merged from main.
Michael Kowalski added 1 commit 2023-12-02 02:35:13 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
d2ba23fd27
Updates based on review comments.
Author
Member

@blender-bot build

@blender-bot build
Michael Kowalski added 2 commits 2023-12-02 03:36:22 +01:00
Author
Member

@blender-bot build

@blender-bot build
Michael Kowalski added 1 commit 2023-12-02 04:07:32 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
62c9a7f6f0
Fixed mac build error.
Author
Member

@blender-bot build

@blender-bot build
Michael Kowalski added 3 commits 2023-12-04 04:22:39 +01:00
8380cd7bf7 USD: allow skel export with multiple modifiers.
Per review feedback from Bastien, no longer enforcing the restriction
that UsdSkel animation export is supported only for meshes that
have a single armature modifier. Previously, meshes with multiple
modifiers were always exported as point caches.
Bastien Montagne requested review from Jesse Yurkovich 2023-12-04 16:45:26 +01:00
Michael Kowalski added 2 commits 2023-12-04 22:46:43 +01:00
47a032d477 USD: add SkelBindingAPI to blend shape skeleton
This fixes an issue where the exported blend shapes were not
animating in USDView.
c54864b46f USD: Add option to export only deform bones for armatures
This patch adds an Only Deform Bones flag to the exporter so that
when exporting UsdSkel assets, non-deforming helper bones are
skipped.  If deform bones live as leaf bones along a  chain of
non-deforming bones, their non-deforming parents are also exported
in order to preserve hierarchy relationships and animation.

Finally, this patch reorders the joint weight binding orders for
UsdGeomMesh exports to match the order and count of joints
written on the UsdSkel side per clip.

This commit was primarily authored by Charles Wardlaw with edits
by Michael Kowalski.
Michael Kowalski added 1 commit 2023-12-05 01:32:07 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
6f6a3b1db2
USD: Now using blender::Map for the deform bone map.
Author
Member

@blender-bot build

@blender-bot build
Jesse Yurkovich reviewed 2023-12-05 10:14:12 +01:00
Jesse Yurkovich left a comment
Member

I've looked over most of this but honestly I'm not going to be the best to check the armature_utils and blend_shape_utils logic. Beyond that just a few minor comments.

I've looked over most of this but honestly I'm not going to be the best to check the armature_utils and blend_shape_utils logic. Beyond that just a few minor comments.
@ -108,0 +132,4 @@
usd_export_context_.export_params.export_shapekeys) &&
attribute_id.name().rfind("skel:") == 0)
{
/* If we're exporting armatures ot shape keys to UsdSkel, we skip any

spelling "ot" -> "or"

spelling "ot" -> "or"
makowalski marked this conversation as resolved
@ -714,0 +902,4 @@
Mesh *USDMeshWriter::get_export_mesh(Object *object_eval, bool &r_needsfree)
{
if (write_blend_shapes_) {

Generally, what happens if there's both shape keys and armatures on the object?

Generally, what happens if there's both shape keys and armatures on the object?
Author
Member

If there are both shape keys and an armature deformer on the object, the mesh is saved in the base shape key pose, which also becomes the rest pose for the skeletal binding.

If there are both shape keys and an armature deformer on the object, the mesh is saved in the base shape key pose, which also becomes the rest pose for the skeletal binding.
deadpin marked this conversation as resolved
@ -47,6 +47,9 @@ struct USDExportParams {
bool export_normals = true;
bool export_mesh_colors = true;
bool export_materials = true;
bool export_armatures;

Set the new fields to their defaults.

Set the new fields to their defaults.
makowalski marked this conversation as resolved
Michael Kowalski added 2 commits 2023-12-05 19:07:42 +01:00
513049875f USD: revert deform group name collision handling
Reverted code to avoid deform group name collisions,
as this is no longer required.
5b7511a6f8 USD: added "Rigging" section to export options
Also disabling deform-only option when not exporting
armatures.
Michael Kowalski added 1 commit 2023-12-05 20:32:48 +01:00
Author
Member

getting these warnings when building the branch:

The warnings have been fixed.

> getting these warnings when building the branch: The warnings have been fixed.
Michael Kowalski added 1 commit 2023-12-06 00:03:20 +01:00
buildbot/vexp-code-patch-coordinator Build done. Details
c0d2bc76dc
Merge branch 'main' into usdskel_export_pr
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR111931) when ready.
Author
Member

I believe I've addressed the requested changes. I've also included in this pull request a new Only Deform Bones USD export option implemented by @CharlesWardlaw.

This pull request is now ready for further review.

I believe I've addressed the requested changes. I've also included in this pull request a new `Only Deform Bones` USD export option implemented by @CharlesWardlaw. This pull request is now ready for further review.
Bastien Montagne requested changes 2023-12-19 18:27:48 +01:00
Bastien Montagne left a comment
Owner

NOTE: This is a code-level only review.

Generally looks fine, besides some details noted below, that should be trivial to address.


Non-blocking comments:

I noticed this PR is still using the 'C-way' of passing or returning optional data (i.e. using pointers, and checking if they are null). I think many of these cases could use std::optional instead? But this does not have to be addressed in this PR, could also be a later cleanup/refactor.

NOTE: This is a code-level only review. Generally looks fine, besides some details noted below, that should be trivial to address. ---------------------------- Non-blocking comments: I noticed this PR is still using the 'C-way' of passing or returning optional data (i.e. using pointers, and checking if they are null). I think many of these cases could use `std::optional` instead? But this does not have to be addressed in this PR, could also be a later cleanup/refactor.
@ -338,0 +359,4 @@
"skinned meshes");
RNA_def_boolean(ot->srna,
"use_deform",

Think this was already discussed, but could be renamed to do_deform_bones_only, or even only_deform_bones?

Think this was already discussed, but could be renamed to `do_deform_bones_only`, or even `only_deform_bones`?
Author
Member

I changed the option name to only_deform_bones. Thanks for the suggestion.

I changed the option name to `only_deform_bones`. Thanks for the suggestion.
makowalski marked this conversation as resolved
@ -0,0 +175,4 @@
}
const bool deform = !(bone->flag & BONE_NO_DEFORM);
if (deform && deform_map) {

This && deform_map is not needed, the function already returns early in case it is null.

This `&& deform_map` is not needed, the function already returns early in case it is null.
makowalski marked this conversation as resolved
@ -0,0 +184,4 @@
/* Get deform parents */
for (const auto &item : deform_map->items()) {
if (item.value) {

Would rather assert here, think an entry in the mapping without a valid value would be a bug?

Would rather assert here, think an entry in the mapping without a valid value would be a bug?
makowalski marked this conversation as resolved
@ -0,0 +186,4 @@
for (const auto &item : deform_map->items()) {
if (item.value) {
const Bone *parent = item.value->parent;
while (parent) {

Could be a for loop instead...

Could be a `for` loop instead...
makowalski marked this conversation as resolved
@ -0,0 +108,4 @@
pxr::UsdSkelBindingAPI skel_api = pxr::UsdSkelBindingAPI::Apply(mesh_prim);
if (!skel_api) {
CLOG_WARN(&LOG,

There is no need to explicitly print the function name in the message, the CLOG_ macros do it automatically.

Same for all other cases below.

There is no need to explicitly print the function name in the message, the `CLOG_` macros do it automatically. Same for all other cases below.
makowalski marked this conversation as resolved
@ -0,0 +513,4 @@
pxr::VtFloatArray src_weights;
if (info.src_weights_attr.Get(&src_weights, time)) {
if (!info.anim_map.Remap(src_weights, &dst_weights)) {
std::cerr << "WARNING: Failed remapping blend shape weights." << std::endl;

Should be a CLOG_WARN or so

Should be a `CLOG_WARN` or so
makowalski marked this conversation as resolved
@ -0,0 +109,4 @@
if (pxr::UsdGeomXform xf = get_xform_ancestor(prim, skel.GetPrim())) {
/* We found a common Xform ancestor, so we set its type to UsdSkelRoot. */
BKE_reportf(reports,

Why BKE_report? This looks like it should be a CLOG_INFO instead?

Why `BKE_report`? This looks like it should be a `CLOG_INFO` instead?
makowalski marked this conversation as resolved
@ -0,0 +113,4 @@
LISTBASE_FOREACH (const bPoseChannel *, pchan, &pose->chanbase) {
if (!pchan->bone) {
printf("WARNING: pchan %s is missing bone.\n", pchan->name);

Should use CLOG_WARN instead.

Actually, have you ever run into such a case? I think this could be an assert instead.

Should use `CLOG_WARN` instead. Actually, have you ever run into such a case? I think this could be an assert instead.
makowalski marked this conversation as resolved
@ -0,0 +173,4 @@
return;
}
Map<const char *, const Bone *> *use_deform = usd_export_context_.export_params.use_deform ?

Why naming the mapping use_deform, instead of deform_map? This is fairly confusing imho...

Why naming the mapping `use_deform`, instead of `deform_map`? This is fairly confusing imho...
makowalski marked this conversation as resolved

After (very!) quick testing, I have a feeling I am missing something.

This is the source .blend file: usd_io_armature_test_basic.blend, and the exported USDC: usd_io_armature_test_basic.usdc

It has a cube deformed by a single bone armature, and that bone is posed.

image

This is what I get after exporting and re-importing USDC file, using default settings:

image.

As you can see, the pose is lost (also tried with enabling export of animation, but same result). Further more, the imported bone is shortened compared to the original one (this may be a limitation of the USD representation of skeletons?).

After (very!) quick testing, I have a feeling I am missing something. This is the source .blend file: [usd_io_armature_test_basic.blend](/attachments/e89e81fb-fc06-497a-8783-142706503db4), and the exported USDC: [usd_io_armature_test_basic.usdc](/attachments/8becf768-51fe-4272-8d1d-1338077aabc2) It has a cube deformed by a single bone armature, and that bone is posed. ![image](/attachments/8266beaf-6ad7-4213-aeb1-9d1e82eec80a) This is what I get after exporting and re-importing USDC file, using default settings: ![image](/attachments/a242025a-9fa2-4a08-b7aa-38f9fa070aee). As you can see, the pose is lost (also tried with enabling export of animation, but same result). Further more, the imported bone is shortened compared to the original one (this may be a limitation of the USD representation of skeletons?).
Hans Goudey reviewed 2023-12-19 18:42:13 +01:00
@ -0,0 +70,4 @@
bArmature *armature = (bArmature *)ob_arm->data;
for (Bone *bone = (Bone *)armature->bonebase.first; bone; bone = bone->next) {
Member

Can use the LISTBASE_FOREACH macro, a bit nicer IMO

Can use the `LISTBASE_FOREACH` macro, a bit nicer IMO
makowalski marked this conversation as resolved
@ -0,0 +79,4 @@
const bool use_deform,
Vector<std::string> &r_names)
{
Map<const char *, const Bone *> deform_map;
Member

I'm fairly sure this Map is hashing and testing equality of the const char * pointers rather than the actual strings. If the strings are owned outside of the map, I'd suggest using Map<StringRef, const Bone *>. That will ensure the hashing and comparison uses the string, and might be faster too, since it won't have to test string length all the time.

I'm fairly sure this Map is hashing and testing equality of the `const char *` pointers rather than the actual strings. If the strings are owned outside of the map, I'd suggest using `Map<StringRef, const Bone *>`. That will ensure the hashing and comparison uses the string, and might be faster too, since it won't have to test string length all the time.
makowalski marked this conversation as resolved
@ -0,0 +112,4 @@
const Object *obj,
const Map<const char *, const Bone *> *deform_map)
{
if (!(skel_anim && obj && obj->pose)) {
Member

These null checks seem overly defensive to me. I'd expect object to not be null here, and skel_anim is a reference, so that's more or less checked at compile time. The consensus I've seen is that checking for null at this level is more likely to hide bugs or confuse different abstraction levels.

Good to see you're using references a fair amount in this file :) Doing that a bit more might help clarify things

These null checks seem overly defensive to me. I'd expect object to not be null here, and skel_anim is a reference, so that's more or less checked at compile time. The consensus I've seen is that checking for null at this level is more likely to hide bugs or confuse different abstraction levels. Good to see you're using references a fair amount in this file :) Doing that a bit more might help clarify things
makowalski marked this conversation as resolved
@ -0,0 +346,4 @@
pxr::VtTokenArray blendshape_names;
LISTBASE_FOREACH (KeyBlock *, kb, &key->block) {
if (!kb) {
Member

LISTBASE_FOREACH won't give you a null item-- linked list items are always non-null

`LISTBASE_FOREACH` won't give you a null item-- linked list items are always non-null
makowalski marked this conversation as resolved
Author
Member

As you can see, the pose is lost (also tried with enabling export of animation, but same result). Further more, the imported bone is shortened compared to the original one (this may be a limitation of the USD representation of skeletons?).

@mont29 Thank you so much for testing and for pointing this out!

The first issue (lost pose) happens when there in no pose animation that varies over time. This is partly due to an issue in the skeleton import code, which can be addressed in a separate PR. In any case, I've also identified a related issue in the skeleton export code in the current PR, and will provide a fix in the next day or so. Incidentally, if I animate the bone transform over time and enable exporting animation, the animation loads correctly from the USD back into Blender when I try it. I believe it's only the case where there is no animation or only an animation with a single default sample that the issue arises, and I will address this.

The second issue (shortened bone) is indeed due to a limitation of UsdSkel. USD skeletons are composed of joints, not bones, and the joints don't encode length. When importing the skeleton back into Blender, the import code tries to guess what the bone length should be based on distance from parent bone to child bone. In the case where we have a leaf joint, the guess is based on additional heuristics, such as the average of the estimated lengths for non-leaf bones computed for the entire skeleton. In the case where there is a single bone in the skeleton, the bone is assigned a default length of 1.0. Such heuristics can perhaps be improved on import to give better results.

> As you can see, the pose is lost (also tried with enabling export of animation, but same result). Further more, the imported bone is shortened compared to the original one (this may be a limitation of the USD representation of skeletons?). @mont29 Thank you so much for testing and for pointing this out! The first issue (lost pose) happens when there in no pose animation that varies over time. This is partly due to an issue in the skeleton *import* code, which can be addressed in a separate PR. In any case, I've also identified a related issue in the skeleton export code in the current PR, and will provide a fix in the next day or so. Incidentally, if I animate the bone transform over time and enable exporting animation, the animation loads correctly from the USD back into Blender when I try it. I believe it's only the case where there is no animation or only an animation with a single default sample that the issue arises, and I will address this. The second issue (shortened bone) is indeed due to a limitation of `UsdSkel`. USD skeletons are composed of joints, not bones, and the joints don't encode length. When importing the skeleton back into Blender, the import code tries to guess what the bone length should be based on distance from parent bone to child bone. In the case where we have a leaf joint, the guess is based on additional heuristics, such as the average of the estimated lengths for non-leaf bones computed for the entire skeleton. In the case where there is a single bone in the skeleton, the bone is assigned a default length of 1.0. Such heuristics can perhaps be improved on import to give better results.

The first issue (lost pose) happens when there in no pose animation that varies over time. This is partly due to an issue in the skeleton import code, which can be addressed in a separate PR. In any case, I've also identified a related issue in the skeleton export code in the current PR, and will provide a fix in the next day or so. Incidentally, if I animate the bone transform over time and enable exporting animation, the animation loads correctly from the USD back into Blender when I try it. I believe it's only the case where there is no animation or only an animation with a single default sample that the issue arises, and I will address this.

Yes, with animation things work fine indeed.

The second issue (shortened bone) is indeed due to a limitation of UsdSkel. USD skeletons are composed of joints, not bones, and the joints don't encode length. When importing the skeleton back into Blender, the import code tries to guess what the bone length should be based on distance from parent bone to child bone. In the case where we have a leaf joint, the guess is based on additional heuristics, such as the average of the estimated lengths for non-leaf bones computed for the entire skeleton. In the case where there is a single bone in the skeleton, the bone is assigned a default length of 1.0. Such heuristics can perhaps be improved on import to give better results.

This is similar to the FBX handling of skeletons... In FBX exporter there is the Add Leaf Bones option (enabled by default), which adds an extra joint at the tip of each Blender leaf bone. The importer has a matching Ignore Leaf Bones (although disabled by default), to remove these on import. These extra leaves are always fully non-deforming joints. Could this be considered as an USD-valid solution as well? Or would that go against USD standard and/or common expectations from other DCCs?

> The first issue (lost pose) happens when there in no pose animation that varies over time. This is partly due to an issue in the skeleton *import* code, which can be addressed in a separate PR. In any case, I've also identified a related issue in the skeleton export code in the current PR, and will provide a fix in the next day or so. Incidentally, if I animate the bone transform over time and enable exporting animation, the animation loads correctly from the USD back into Blender when I try it. I believe it's only the case where there is no animation or only an animation with a single default sample that the issue arises, and I will address this. Yes, with animation things work fine indeed. > The second issue (shortened bone) is indeed due to a limitation of `UsdSkel`. USD skeletons are composed of joints, not bones, and the joints don't encode length. When importing the skeleton back into Blender, the import code tries to guess what the bone length should be based on distance from parent bone to child bone. In the case where we have a leaf joint, the guess is based on additional heuristics, such as the average of the estimated lengths for non-leaf bones computed for the entire skeleton. In the case where there is a single bone in the skeleton, the bone is assigned a default length of 1.0. Such heuristics can perhaps be improved on import to give better results. This is similar to the FBX handling of skeletons... In FBX exporter there is the `Add Leaf Bones` option (enabled by default), which adds an extra joint at the tip of each Blender leaf bone. The importer has a matching `Ignore Leaf Bones` (although disabled by default), to remove these on import. These extra leaves are always fully non-deforming joints. Could this be considered as an USD-valid solution as well? Or would that go against USD standard and/or common expectations from other DCCs?
Author
Member

This is similar to the FBX handling of skeletons... In FBX exporter there is the Add Leaf Bones option (enabled by default), which adds an extra joint at the tip of each Blender leaf bone. The importer has a matching Ignore Leaf Bones (although disabled by default), to remove these on import. These extra leaves are always fully non-deforming joints. Could this be considered as an USD-valid solution as well? Or would that go against USD standard and/or common expectations from other DCCs?

I think the Add Leaf Bones option may be valid for USD as well, and I'll investigate this. Thanks for the suggestion.

Another option might be to record the original Blender bone lengths in a custom property on the USD skeleton primitive that the importer could look up when creating bones.

> This is similar to the FBX handling of skeletons... In FBX exporter there is the `Add Leaf Bones` option (enabled by default), which adds an extra joint at the tip of each Blender leaf bone. The importer has a matching `Ignore Leaf Bones` (although disabled by default), to remove these on import. These extra leaves are always fully non-deforming joints. Could this be considered as an USD-valid solution as well? Or would that go against USD standard and/or common expectations from other DCCs? I think the `Add Leaf Bones` option may be valid for USD as well, and I'll investigate this. Thanks for the suggestion. Another option might be to record the original Blender bone lengths in a custom property on the USD skeleton primitive that the importer could look up when creating bones.
Michael Kowalski added 1 commit 2023-12-21 17:37:50 +01:00
000db4e542 USD: improve non-animating skeleton export.
No longer creating a USD animation primitive if exporting
animation is turned off.

Now setting the USD rest transforms to the Blender
parent-relative pose matrices.
Michael Kowalski added 4 commits 2023-12-22 06:25:13 +01:00
Michael Kowalski added 3 commits 2023-12-22 08:44:39 +01:00
eaea815b50 Merge branch 'main' into usdskel_export_pr.
This commit won't compile due to merge conflicts.
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
54105b2298
USD: adopting option name only_deform_bones.
Author
Member

@blender-bot build

@blender-bot build
Author
Member

I believe I've addressed the requested changes. Thanks, again, for the reviews.

@mont29 Regarding the issue you noted:

  • The problem with the lost pose when re-importing the skeleton is actually entirely an import issue at this point and I will address this in a separate pull request. I made some improvements to how the rest transforms are saved and confirmed that the pose is being exported correctly and loads as expected in USDView, Houdini and Omniverse.
  • I was thinking the issue with the mismatching lengths can be addressed in a separate task as well, as this will require further discussion. But please let me know if this is acceptable.
I believe I've addressed the requested changes. Thanks, again, for the reviews. @mont29 Regarding the issue you noted: - The problem with the lost pose when re-importing the skeleton is actually entirely an import issue at this point and I will address this in a separate pull request. I made some improvements to how the rest transforms are saved and confirmed that the pose is being exported correctly and loads as expected in USDView, Houdini and Omniverse. - I was thinking the issue with the mismatching lengths can be addressed in a separate task as well, as this will require further discussion. But please let me know if this is acceptable.
Michael Kowalski added 1 commit 2023-12-22 15:15:36 +01: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-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
aa0fa05265
USD: fix compile warning for linux/mac.
Author
Member

@blender-bot build

@blender-bot build

Sorry, forgot to reply.

@mont29 Regarding the issue you noted:

  • The problem with the lost pose when re-importing the skeleton is actually entirely an import issue at this point and I will address this in a separate pull request. I made some improvements to how the rest transforms are saved and confirmed that the pose is being exported correctly and loads as expected in USDView, Houdini and Omniverse.

Great!

  • I was thinking the issue with the mismatching lengths can be addressed in a separate task as well, as this will require further discussion. But please let me know if this is acceptable.

Yes, I think this is acceptable. Would be good to create a TODO (or design) task for it now, then.

Sorry, forgot to reply. > @mont29 Regarding the issue you noted: > > - The problem with the lost pose when re-importing the skeleton is actually entirely an import issue at this point and I will address this in a separate pull request. I made some improvements to how the rest transforms are saved and confirmed that the pose is being exported correctly and loads as expected in USDView, Houdini and Omniverse. Great! > - I was thinking the issue with the mismatching lengths can be addressed in a separate task as well, as this will require further discussion. But please let me know if this is acceptable. Yes, I think this is acceptable. Would be good to create a TODO (or design) task for it now, then.
Bastien Montagne requested changes 2023-12-28 13:19:12 +01:00
Bastien Montagne left a comment
Owner

Some format issues from the buildbot need to be addressed.

Some format issues from the buildbot need to be addressed.
Michael Kowalski added 2 commits 2023-12-28 21:24:04 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-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-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
c2c64a23f5
USD: format fixes.
Author
Member

@blender-bot build

@blender-bot build
Author
Member

Some format issues from the buildbot need to be addressed.

I believe I've fixed the formatting now. Thanks for pointing this out.

> Some format issues from the buildbot need to be addressed. I believe I've fixed the formatting now. Thanks for pointing this out.
Author
Member

Would be good to create a TODO (or design) task for it now, then.

I created two tasks based on the previous discussion:

#116612
#116615

> Would be good to create a TODO (or design) task for it now, then. I created two tasks based on the previous discussion: https://projects.blender.org/blender/blender/issues/116612 https://projects.blender.org/blender/blender/issues/116615
Bastien Montagne approved these changes 2023-12-29 16:08:49 +01:00
Jesse Yurkovich approved these changes 2023-12-31 01:05:44 +01:00
Jesse Yurkovich left a comment
Member

Looks ok. Just 2 minor comments that don't require another review unless you'd like to discuss any.

Looks ok. Just 2 minor comments that don't require another review unless you'd like to discuss any.
@ -51,0 +61,4 @@
static const pxr::TfToken Anim("Anim", pxr::TfToken::Immortal);
static const pxr::TfToken joint1("joint1", pxr::TfToken::Immortal);
static const pxr::TfToken Skel("Skel", pxr::TfToken::Immortal);
} // namespace usdtokens

Looks like these usdtokens aren't needed? The ones defined for blend shapes are used though.

Looks like these usdtokens aren't needed? The ones defined for blend shapes are used though.
Author
Member

Good catch. Thanks!

Good catch. Thanks!
makowalski marked this conversation as resolved
@ -830,0 +947,4 @@
free_export_mesh(mesh);
}
}
catch (...) {

Where are exceptions coming from in this path?

Where are exceptions coming from in this path?
Author
Member

These could possibly come from the USD calls in export_deform_verts(). In general, this is following the pattern in USDGenericMeshWriter::do_write() to ensure the temporary mesh is freed.

These could possibly come from the USD calls in `export_deform_verts()`. In general, this is following the pattern in `USDGenericMeshWriter::do_write()` to ensure the temporary mesh is freed.
makowalski marked this conversation as resolved
Michael Kowalski added 2 commits 2024-01-02 15:01:45 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
d745d6abb5
USD: removed unused USD token definitions.
Author
Member

@blender-bot build

@blender-bot build
Michael Kowalski merged commit c6d61e8586 into main 2024-01-02 15:51:50 +01:00
Michael Kowalski deleted branch usdskel_export_pr 2024-01-02 15:51:53 +01:00
Sign in to join this conversation.
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
5 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#111931
No description provided.