USD: Skeleton and blend shape import #110912
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#110912
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "makowalski/blender:usdskel_import"
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?
Added support for
UsdSkel
animation import.This addresses #110076.
For a description of the
UsdSkel
API, please see: https://openusd.org/23.05/api/usd_skel_page_front.htmlUSDSkeletonReader
class which importsUsdSkelSkeleton
primitives as armatures.USDMeshReader
to importUsdSkelBlendShape
as shape keys.USDMeshReader
to import USD skinning data as as deform groups and an armature modifier on the mesh object.USDMeshReader::get_local_usd_xform()
to override the transform computation to account for the binding transformation for skinned meshes.Limitations
UsdSkel
bone hierarchy is displayed as relationship lines between parent and child bones. Maybe if some day there is such a display option in Blender as well, that could help in this case.USD Import Options UI Changes
Skeletal Animation Test Files
https://developer.apple.com/augmented-reality/quick-look/models/drummertoy/toy_drummer_idle.usdz
https://developer.apple.com/augmented-reality/quick-look/models/vintagerobot2k/robot_walk_idle.usdz
https://developer.apple.com/augmented-reality/quick-look/models/biplane/toy_biplane_idle.usdz
https://github.com/usd-wg/assets/tree/main/full_assets/ElephantWithMonochord
https://github.com/usd-wg/assets/blob/main/full_assets/ElephantWithMonochord/SoC-ElephantWithMonochord.usdz
https://github.com/PixarAnimationStudios/OpenUSD/blob/release/pxr/usdImaging/bin/testusdview/testenv/testUsdviewSkinning/arm.usda (also attached as a test suite file)
Blend Shape Animation Test File
See attached
bs_face_anim_example.usd
andusd_blend_shape_test.usda
(test suite file).@blender-bot build
@blender-bot package
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
From a quick look at the code generally looks fine to me, only have a few minor comments below.
Will complete this pass of review tomorrow, but might be good to get also someone from the Animation team to have a look on it, @dr.sybren ? @nathanvegdahl ?
@ -984,0 +1023,4 @@
if (!import_params_.import_skeletons || prim_.IsInstanceProxy()) {
/* Use the standard transform computation, since we are ignoring
* skinning data. Note that applying theUsdSkelBindingAPI to an
theUsdSkelBindingAPI
missing space ;)@ -0,0 +47,4 @@
namespace {
/* Utility: create curve at the given array index. */
FCurve *create_fcurve(int array_index, const char *rna_path)
const char *rna_path
could probably still beconst std::string &rna_path
instead?And then call
BLI_strdup(rna_path.c_str())
below.@ -0,0 +51,4 @@
{
FCurve *fcu = BKE_fcurve_create();
fcu->flag = (FCURVE_VISIBLE | FCURVE_SELECTED);
fcu->rna_path = BLI_strdupn(rna_path, strlen(rna_path));
Could simply call
BLI_strdup()
here, it's doing exactly that.@ -0,0 +59,4 @@
/* Utility: create curve at the given array index and
* adds it as a channel to a group. */
FCurve *create_chan_fcurve(
bAction *act, bActionGroup *grp, int array_index, const char *rna_path, int totvert)
const char *rna_path
could probably still beconst std::string &rna_path
instead? would also makes call to this function less verbose below.@ -0,0 +78,4 @@
bez.f1 = bez.f2 = bez.f3 = SELECT;
bez.h1 = bez.h2 = HD_AUTO;
insert_bezt_fcurve(fcu, &bez, INSERTKEY_NOFLAGS);
BKE_fcurve_handles_recalc(fcu);
This is a fairly expansive process, I don't think it should be called on every 'bezt' insertion.
Would rather call that once on each curve at the end of
import_skeleton_curves()
(andimport_blendshapes()
).@ -0,0 +238,4 @@
/* Set the curve samples. */
for (double frame : samples) {
picky empty line ;)
@ -0,0 +253,4 @@
}
for (int i = 0; i < joint_local_xforms.size(); ++i) {
picky empty line
@ -0,0 +406,4 @@
std::set<pxr::TfToken> shapekey_names;
for (int i = 0; i < targets.size(); ++i) {
picky empty line
@ -0,0 +559,4 @@
std::vector<FCurve *> curves;
for (auto blendshape_name : blendshapes) {
picky empty line
@ -0,0 +685,4 @@
return;
}
/* Check if any bone natrices have negative determinants,
Type:
bone natrices have
@ -0,0 +689,4 @@
* indicating negative scales, possibly due to mirroring
* operations. Such matrices can't be propery converted
* to Blender's axis/roll bone representation (see
* https://developer.blender.org/T82930). If we detect
Updated URL:
https://projects.blender.org/blender/blender/issues/82930
Thanks for the review, @mont29! I've made the requested changes.
@blender-bot package
Package build started. Download here when ready.
Thanks, patch LGTM now.
Am getting a crash though in debug build on linux when trying to import
robot_walk_idle.usdz
and the others, but this is also happening in main, will open a separate bug report for that.Am still on the fence regarding the bones orientation handling, I have the feeling users won't be very happy the have orthogonal bones if the want to edit their pose or animation... @dfelinto @dr.sybren @nathanvegdahl thoughts on this topic?
Also, @dr.sybren and @nathanvegdahl , adding you as reviewers, please resign if you do not have time for this patch. ;)
Generally looks good to me. Just some nits. But full disclosure that I only have a cursory knowledge of USD, and am definitely not familiar with its APIs. So this is mainly a Blender-side review.
@ -0,0 +228,4 @@
pxr::VtMatrix4dArray joint_local_bind_xforms(bind_xforms.size());
for (int i = 0; i < bind_xforms.size(); ++i) {
int parent_id = skel_topology.GetParent(i);
I think this can be const.
@ -0,0 +269,4 @@
}
float re = qrot.GetReal();
pxr::GfVec3f im = qrot.GetImaginary();
I think these can be const...?
@ -0,0 +272,4 @@
pxr::GfVec3f im = qrot.GetImaginary();
for (int j = 0; j < 3; ++j) {
int k = 3 * i + j;
I think this can be const.
@ -0,0 +283,4 @@
}
for (int j = 0; j < 4; ++j) {
int k = 4 * i + j;
I think this can be const.
@ -0,0 +299,4 @@
}
for (int j = 0; j < 3; ++j) {
int k = 3 * i + j;
I think this can be const.
@ -0,0 +560,4 @@
return;
}
size_t num_samples = times.size();
I think this can be const.
@ -0,0 +669,4 @@
}
/* Sanity check: we should have created a bone for each joint. */
size_t num_joints = skel_topology.GetNumJoints();
I think this can be const.
@ -0,0 +703,4 @@
* https://projects.blender.org/blender/blender/issues/82930).
* If we detect such matrices, we will flag an error and won't
* try to import the animation, since the rotations would
* be incorrect in such cases. Unfortunately, the Pixar
This is probably better as future work, but for sampled animation data it should be possible to just convert the animation data to be equivalent for the skeleton we actually end up with. On the other hand, it might just not be worth it, and maybe there are round-tripping considerations I'm not aware of here.
@ -0,0 +755,4 @@
std::vector<std::vector<int>> child_bones(num_joints);
for (size_t i = 0; i < num_joints; ++i) {
int parent_idx = skel_topology.GetParent(i);
I think this can be const.
@ -0,0 +802,4 @@
pxr::GfVec3f parent_head(parent->head);
pxr::GfVec3f parent_tail(parent->tail);
float new_len = (avg_child_head - parent_head).GetLength();
I think this can be const.
@ -0,0 +805,4 @@
float new_len = (avg_child_head - parent_head).GetLength();
/* Be sure not to scale by zero. */
if (new_len > .00001) {
This is probably an unlikely corner case, so maybe not worth the effort. But if you want to take the time:
Rather than a fixed epsilon, it would probably be better to use an epsilon relative to the magnitude of the largest component of
parent_head
. Something like:Fixed epsilons can produce strange/unexpected results when the numbers involved are large. (Relative epsilons can also break with e.g. denormal numbers, but I think that's extremely unlikely in this case.) In this case, with a fixed epsilon, there's a risk of the direction of the bone significantly changing during the re-scaling due to floating point rounding error when the head of the parent is close to its children but far away from the origin.
I made the change you suggested. Thanks for pointing out this issue!
It occurs to me in the edge case when
parent_head
is the origin, the comparison becomesif (new_len > 0)
, but I suspect this is fine.Yeah, I was thinking of that and similar edge cases when I mentioned denormal numbers. But indeed, the bones involved would have to be extremely close to the origin (on the order of 1.0e-34 ish units, I think) for that to start to be an issue, which is extremely unlikely in any practical situation.
For comparison, protons are about 1.0e-15 meters in size, according to wikipedia. So we're talking modeling sub-sub-sub-atomic things in real world units. And then rigging them with armatures, ha ha.
(Edit: the Planck length is about 1.0-e35 meters, apparently. So if people are putting bones that close to both each other and the origin without them being literally equal positions, they must be working on a hell of a project!)
@ -0,0 +815,4 @@
/* Scale terminal bones by the average length scale. */
avg_len_scale /= num_joints;
if (avg_len_scale > .00001) {
Same comment here, regarding fixed epsilons. If you decide this is worth addressing, you'll need to move the check into the loop and compute it per bone.
@ -0,0 +1011,4 @@
for (int j = 0; j < joint_weights_elem_size; ++j) {
int k = offset + j;
float w = joint_weights[k];
if (w < .00001) {
I think k, w, and joint_idx (further below) can all be const.
@ -0,0 +44,4 @@
bool import_anim = true);
/**
* Import the given USD sekeleton as an armature object. Optionally, if the
Typo: sekeleton > skeleton
@ -0,0 +59,4 @@
const pxr::UsdSkelSkeleton &skel,
bool import_anim = true);
/**
* Import skinning data from a source USD prim as deform groups and an armature
I might be misunderstanding the comment, but I think by deform groups you mean vertex groups? That's the Blender terminology.
@mont29 informed me that in fact "deform group" is also used in some areas of Blender's code. So never mind! (Might be good to unify the terminology in the code base at some point, but that's obviously not for this PR!)
If I'm understanding the concern right, then I think it depends somewhat on the primary intended use case for this. If it's interop with USD, then leaving the orientations alone probably makes sense. But if it's purely import, then adapting the orientations to make sense in Blender is probably better. And if it's both, then I have no idea.
In FBX importer, ensuring 'Blender-like' orientation for bones are two options (force connect, and automatic bone orientation), which are OFF by default...
In that case, maybe an option would indeed be good. Although I don't feel strongly about that being part of this PR. It's the kind of thing that can be added later, I think.
Thanks so much for the review! I will implement the requested changes.
Regarding the bone orientation. I wonder if another option could be to provide another display option in Blender more suitable to representing joints. Here is how
toy_drummer_idle.usdz
displays in Omniverse Composer:The joints are represented as spheres with connections between parent and child joints. That way the USD transforms could be kept intact, but they would make more sense visually. I don't know how feasible this would be in Blender. If this seems like a realistic option, we would be happy to work on this on the NVIDIA side, if that makes sense. @CharlesWardlaw for visibility.
@blender-bot build
@nathanvegdahl Thanks, again, for the review! At this point, I made most of the changes you requested, I believe, except for a couple you indicated might be addressed in future tasks.
@mont29 @nathanvegdahl I will certainly investigate implementing bone orientation fixes similar to what is done in the FBX importer. But I do wonder if this might be better to do as a future task, given that the current pull request is already fairly complex. However, I will follow your lead on this.
At this I think the bone orientation can be a future task yes.
BTW, I think fixing #110950 should be high priority here, since it basically makes testing this PR impossible for me (and likely anybody using linux builds?)
Indeed! I believe I have a fix for that crash and will create a pull request for you to try today.
@mont29 FYI, I just merged the latest from main into this pull request, including the fix to the USDZ import crash on linux.
Can confirm all proposed test files now import perfectly on linux as well.
For me this patch is ready for master.
Would be good to add one or two test cases though to our unittests suit for USD ?
@blender-bot build
Looks good to me as well!
This can be noted down as a known limitation for now. This has come up with pretty much every importer that can import armatures. Next Animation & Rigging module meeting will actually discuss putting a solution for this on the agenda. It might not get the highest priority, but as a module we do want to have an official plan for this.
As an intermediary solution, there's also the idea of having bones in pose/object mode just displayed as spheres (#106230), which would at least remove the porcupine look.
Yup, that's pretty much what the spheres mode would be.
Is that warning displayed in the UI? Or only on the console?
When I import this file I have lots of these in the terminal while importing:
If I then switch the viewport to Material mode, Blender crashes. I've posted more info in Blender Chat, as that crash happens at startup in a
main
branch build as well, so it's unrelated to this PR. It does block me from testing this properly though.Yes, I will be adding the test cases. Thanks!
The code is fortunately straight-forward, that's nice! Just one nag about using C-style optional return parameters instead of C++
std::optional<>
.@ -45,6 +45,11 @@ class USDStageReader {
void collect_readers(struct Main *bmain);
/* Complete setting up the armature modifiers that
Docstrings should be Doxygen style, so start with
/**
@ -171,0 +155,4 @@
pxr::GfMatrix4d *r_xform,
bool *r_is_constant) const
{
if (!r_xform) {
r_xform
is not documented as being optional, and this function has no meaning when it isnullptr
. This means that havingr_xform == nullptr
is a programming error, rather than an expected result of some input. UseBLI_assert()
to check such errors, instead of silently ignoring them.It might even be better to circumvent the whole situation, and make this class of errors impossible. This can be done by changing the return type to
std::optional<pxr::GfMatrix4d>
. Ifr_is_constant
is optional, document it as such. Otherwise it might be useful to just return an optional tuple(matrix, is_constant)
.I refactored the function to return an optional tuple, as you recommended. I think this is an elegant solution. Thanks for the suggestion!
@ -53,0 +59,4 @@
* \param r_is_constant: Return value flag, set to false if the transform is
* time varying
*/
virtual bool get_local_usd_xform(const float time,
This should document what the returned boolean means.
@dr.sybren Thank you so much for the comments and testing!
That sphere display option looks great and would be a very nice option to have!
That warning is displayed in the UI.
Yes, this is coming from the new generic mesh attribute import code. Some USD attribute types aren't currently handled, which is to be expected. But maybe the warnings should be made less verbose.
I'm not set up to run linux at the moment to help diagnose, unfortunately. I will try to set myself up for linux building/testing going forward.
I added test cases for skeleton and blend shape import. These require the attached
usd_blend_shape_test.usda
andarm.usda
files to be copied to thetests/usd/
directory.I believe at this point requested changes have been addressed.
@blender-bot build
This was caused by an issue in the color management code, it's been fixed in #111144.
Just a few smaller remarks, these can be addressed when landing the PR.
@ -97,0 +110,4 @@
* Override transform computation to account for the binding
* transformation for skinned meshes.
*/
std::optional<XformResult> get_local_usd_xform(const float time) const override;
Remove the
const
fromconst float time
, in the declaration it has no meaning. See the C/C++ style guide for more info.@ -321,0 +350,4 @@
if (!reader->object()) {
continue;
}
if (const USDMeshReader *mesh_reader = dynamic_cast<const USDMeshReader *>(reader)) {
Swap the condition and
continue
here as well. That wayall the precondition checks use the same logic, and the main path of the for-body can be followed vertically.@ -171,0 +169,4 @@
return std::nullopt;
}
return XformResult(pxr::GfMatrix4f(xform), is_constant);
As
xform
is already apxr::GfMatrix4f
, and that's the expected type for aXformResult
, why is this cast necessary? Might be worth a comment.That's a good point, and I've added a comment to explain this.
xform
is a matrix for doubles (GfMatrix4d
), but Blender expects a matrix of floats, so I explicitly convert to aGfMatrix4f
for the return value.@ -12,6 +12,8 @@
namespace blender::io::usd {
using XformResult = std::tuple<pxr::GfMatrix4f, bool>;
Document what the boolean means.
@ -53,0 +58,4 @@
*
* \param time: Time code for evaluating the transform.
*
* \return: Optional tuple with the following elements:
An explanation fo the type should be done at the type declaration itself, not at one specific use of that type.
@blender-bot build
@blender-bot build