USD: Support armature and shape key export #111931
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
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
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#111931
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "makowalski/blender:usdskel_export_pr"
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?
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.htmlArmature
,Only Deform Bones
andShape Key
USD export options.USDArmatureWriter
class.USDMeshWriter
to write skinned meshes for bindingwith 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.USDHierarchyIterator::process_usd_skel()
function tofinish 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.
USDTransformWriter::do_write()
to write transformssparsely, to avoid saving redundant transform values when exporting
armatures.
create_skel_roots()
function, called on the stage at theend 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
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
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.
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).
Testing
Load the attached
robot_walk.blend
andblendshape_face.blend
files, export to USD and load the saved file inUSDView
. 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 theOnly Defom Bones
export option.USD: Support armature and shape key export.to USD: Support armature and shape key export@blender-bot build
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.
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)
For clarity, usually
FunctionRef
is used instead ofstd::function
for a callback like this (where no persistent storage is necessary). It can even be significantly faster (see25917f0165
), though I doubt it matters here.@ -0,0 +22,4 @@
visitor(bone);
for (Bone *child = (Bone *)bone->childbase.first; child; child = child->next) {
Use
LISTBASE_FOREACH
@ -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)
Best if you can return
const ArmatureModifierData *
to avoid retrieving a mutable modifier from a const object. Same below.@ -0,0 +59,4 @@
return BKE_modifier_is_enabled(scene, mod, mode) ? mod : nullptr;
}
namespace blender::io::usd {
Any particular reason the namespace starts here instead of at the top of the file?
@ -0,0 +74,4 @@
}
}
void get_armature_bone_names(const Object *ob_arm, std::vector<std::string> &r_names)
Use
Vector
instead ofstd::vector
@ -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) {
Based on the places the visitor is used, it seems the null check isn't necessary here.
@ -0,0 +147,4 @@
return false;
}
bArmature *arm = (bArmature *)arm_mod->object->data;
Type cast style (https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast)
@ -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)
Use
Set
instead ofstd::set
@ -0,0 +188,4 @@
const Key *get_mesh_shape_key(const Object *obj)
{
if (!obj || !obj->data || obj->type != OB_MESH) {
Looking at the
!obj
check here, seems it's a bit overly defensive. This sort of check should probably happen at a higher level.@ -0,0 +239,4 @@
}
if (kb == basis_key) {
// Skip the basis.
Comment style
@ -0,0 +401,4 @@
return;
}
std::vector<BlendShapeMergeInfo> merge_info;
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, andMap
vsstd::map
elsewhere in this PR).@ -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 *>(
BKE_mesh_copy_for_eval
is a slightly friendlier looking way of doing thing@ -1038,0 +1241,4 @@
const pxr::UsdSkelBindingAPI &skel_api,
const std::vector<std::string> &bone_names)
{
if (!mesh || !skel_api) {
Same comment here about defensive null checks
@ -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;
LISTBASE_FOREACH
@ -1038,0 +1271,4 @@
return;
}
const blender::Span<MDeformVert> dverts = mesh->deform_verts();
Since this code is already in the
blender::
namespace, specifying it here forSpan
is unnecessary.@ -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()) {
This looks like an expensive loop relative to the others. Maybe worth parallelizing with
threading::parallel_for
?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?
@ -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");
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).@ -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
Object.defbase
probably refers to long ago when the names were stored on the object. Nowadays the names are stored on the mesh.@blender-bot build
I've made some updates and I believe this patch if ready for further review. The recent changes include:
@blender-bot package
Package build started. Download here when ready.
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)
blender::FunctionRef
->FunctionRef
@ -0,0 +62,4 @@
static const ArmatureModifierData *get_armature_modifier(const Object *obj,
const Depsgraph *depsgraph)
{
BLI_assert(obj);
Seems like instead of asserting that the object is non-null, the argument could be a reference
const Object &
@ -0,0 +130,4 @@
const char *name,
const Depsgraph *depsgraph)
{
if (!obj || !name) {
Same comment here about the null checks
const Object *obj
->const Object &obj
const char *name
->const StringRefNull name
@ -0,0 +153,4 @@
return mods.size() == 1 && mods.first()->type == eModifierType_Armature;
}
Vector<ModifierData *> get_enabled_modifiers(const Object *obj, const Depsgraph *depsgraph)
Vector<ModifierData *>
->Vector<const ModifierData *>
@ -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;
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@ -0,0 +407,4 @@
Vector<BlendShapeMergeInfo> merge_info;
/* We are merging blend shape names and weighs from multiple
weighs
->weights
@ -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);
Making this mesh
const
would clarify the code below.@ -0,0 +3,4 @@
* SPDX-License-Identifier: GPL-2.0-or-later */
#pragma once
#include <map>
Maybe these should be the corresponding Blender container headers now?
@ -8,2 +8,4 @@
#include "usd_exporter_context.h"
#include "usd_skel_convert.h"
#include <map>
Different headers here too?
@ -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)) {
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?I have removed this change, as it's no longer needed.
@ -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;
std::map
toMap
here too, might as well do this consistently. Let me know if you have questions about the API ofMap
. Overall it should hopefully be more intuitive thanstd::map
though@ -0,0 +5,4 @@
#include "usd_writer_abstract.h"
#include <string>
Unnecessary includes here?
getting these warnings when building the branch:
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:
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)?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:
I do not think that this decision should be made automatically, pointcaches are fully destructive, and potentially very heavy.
@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.
@blender-bot build
@blender-bot build
@blender-bot build
@blender-bot build
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"
@ -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?
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.
@ -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.
The warnings have been fixed.
@blender-bot package
Package build started. Download here when ready.
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.
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 evenonly_deform_bones
?I changed the option name to
only_deform_bones
. Thanks for the suggestion.@ -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.@ -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?
@ -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...@ -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.
@ -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@ -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 aCLOG_INFO
instead?@ -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.
@ -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 ofdeform_map
? This is fairly confusing imho...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.
This is what I get after exporting and re-importing USDC file, using default settings:
.
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?).
@ -0,0 +70,4 @@
bArmature *armature = (bArmature *)ob_arm->data;
for (Bone *bone = (Bone *)armature->bonebase.first; bone; bone = bone->next) {
Can use the
LISTBASE_FOREACH
macro, a bit nicer IMO@ -0,0 +79,4 @@
const bool use_deform,
Vector<std::string> &r_names)
{
Map<const char *, const Bone *> deform_map;
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 usingMap<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.@ -0,0 +112,4 @@
const Object *obj,
const Map<const char *, const Bone *> *deform_map)
{
if (!(skel_anim && obj && obj->pose)) {
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
@ -0,0 +346,4 @@
pxr::VtTokenArray blendshape_names;
LISTBASE_FOREACH (KeyBlock *, kb, &key->block) {
if (!kb) {
LISTBASE_FOREACH
won't give you a null item-- linked list items are always non-null@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.Yes, with animation things work fine indeed.
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 matchingIgnore 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.
@blender-bot build
I believe I've addressed the requested changes. Thanks, again, for the reviews.
@mont29 Regarding the issue you noted:
@blender-bot build
Sorry, forgot to reply.
Great!
Yes, I think this is acceptable. Would be good to create a TODO (or design) task for it now, then.
Some format issues from the buildbot need to be addressed.
@blender-bot build
I believe I've fixed the formatting now. Thanks for pointing this out.
I created two tasks based on the previous discussion:
#116612
#116615
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.
Good catch. Thanks!
@ -830,0 +947,4 @@
free_export_mesh(mesh);
}
}
catch (...) {
Where are exceptions coming from in this path?
These could possibly come from the USD calls in
export_deform_verts()
. In general, this is following the pattern inUSDGenericMeshWriter::do_write()
to ensure the temporary mesh is freed.@blender-bot build