USD: Skeleton and blend shape import #110912

Merged
Michael Kowalski merged 38 commits from makowalski/blender:usdskel_import into main 2023-08-17 20:11:58 +02:00
4 changed files with 36 additions and 53 deletions
Showing only changes of commit 02a715f0c0 - Show all commits

View File

@ -1130,19 +1130,13 @@ std::string USDMeshReader::get_skeleton_path() const
return "";
}
bool USDMeshReader::get_local_usd_xform(const float time,
pxr::GfMatrix4d *r_xform,
bool *r_is_constant) const
std::optional<XformResult> USDMeshReader::get_local_usd_xform(const float time) const
{
if (!r_xform) {
return false;
}
if (!import_params_.import_skeletons || prim_.IsInstanceProxy()) {
/* Use the standard transform computation, since we are ignoring
* skinning data. Note that applying the UsdSkelBinding API to an
* instance proxy generates a USD error. */
return USDXformReader::get_local_usd_xform(time, r_xform, r_is_constant);
return USDXformReader::get_local_usd_xform(time);
}
if (pxr::UsdSkelBindingAPI skel_api = pxr::UsdSkelBindingAPI::Apply(prim_)) {
@ -1151,11 +1145,7 @@ bool USDMeshReader::get_local_usd_xform(const float time,
if (skel_api.GetGeomBindTransformAttr().Get(&bind_xf)) {
/* Assume that if a bind transform is defined, then the
* transform is constant. */
if (r_is_constant) {
*r_is_constant = true;
}
*r_xform = bind_xf;
return true;
return XformResult(pxr::GfMatrix4f(bind_xf), true);
}
else {
WM_reportf(RPT_WARNING,
@ -1166,7 +1156,7 @@ bool USDMeshReader::get_local_usd_xform(const float time,
}
}
return USDXformReader::get_local_usd_xform(time, r_xform, r_is_constant);
return USDXformReader::get_local_usd_xform(time);
}
} // namespace blender::io::usd

View File

@ -106,11 +106,11 @@ class USDMeshReader : public USDGeomReader {
const double motionSampleTime,
MutableSpan<BlenderT> attribute);
/* Override transform computation to account for the binding
* transformation for skinned meshes. */
bool get_local_usd_xform(const float time,
pxr::GfMatrix4d *r_xform,
bool *r_is_constant) const override;
/**
* Override transform computation to account for the binding
* transformation for skinned meshes.
*/
std::optional<XformResult> get_local_usd_xform(const float time) const override;
makowalski marked this conversation as resolved Outdated

Remove the const from const float time, in the declaration it has no meaning. See the C/C++ style guide for more info.

Remove the `const` from `const float time`, in the declaration it has no meaning. See the C/C++ style guide for more info.
};
} // namespace blender::io::usd

View File

@ -67,20 +67,20 @@ void USDXformReader::read_matrix(float r_mat[4][4] /* local matrix */,
const float scale,
bool *r_is_constant)
{
if (r_is_constant) {
*r_is_constant = true;
}
BLI_assert(r_mat);
BLI_assert(r_is_constant);
*r_is_constant = true;
unit_m4(r_mat);
pxr::GfMatrix4d usd_local_xf;
if (!get_local_usd_xform(time, &usd_local_xf, r_is_constant)) {
std::optional<XformResult> xf_result = get_local_usd_xform(time);
if (!xf_result) {
return;
}
/* Convert the result to a float matrix. */
pxr::GfMatrix4f mat4f = pxr::GfMatrix4f(usd_local_xf);
mat4f.Get(r_mat);
std::get<0>(*xf_result).Get(r_mat);
*r_is_constant = std::get<1>(*xf_result);
/* Apply global scaling and rotation only to root objects, parenting
* will propagate it. */
@ -151,34 +151,25 @@ bool USDXformReader::is_root_xform_prim() const
return false;
}
bool USDXformReader::get_local_usd_xform(const float time,
pxr::GfMatrix4d *r_xform,
bool *r_is_constant) const
std::optional<XformResult> USDXformReader::get_local_usd_xform(const float time) const
{
if (!r_xform) {
return false;
}
pxr::UsdGeomXformable xformable;
if (use_parent_xform_) {
xformable = pxr::UsdGeomXformable(prim_.GetParent());
}
else {
xformable = pxr::UsdGeomXformable(prim_);
}
pxr::UsdGeomXformable xformable = use_parent_xform_ ? pxr::UsdGeomXformable(prim_.GetParent()) :
pxr::UsdGeomXformable(prim_);
makowalski marked this conversation as resolved

r_xform is not documented as being optional, and this function has no meaning when it is nullptr. This means that having r_xform == nullptr is a programming error, rather than an expected result of some input. Use BLI_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>. If r_is_constant is optional, document it as such. Otherwise it might be useful to just return an optional tuple (matrix, is_constant).

`r_xform` is not documented as being optional, and this function has no meaning when it is `nullptr`. This means that having `r_xform == nullptr` is a programming error, rather than an expected result of some input. Use `BLI_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>`. If `r_is_constant` is optional, document it as such. Otherwise it might be useful to just return an optional tuple `(matrix, is_constant)`.
Review

I refactored the function to return an optional tuple, as you recommended. I think this is an elegant solution. Thanks for the suggestion!

I refactored the function to return an optional tuple, as you recommended. I think this is an elegant solution. Thanks for the suggestion!
if (!xformable) {
/* This might happen if the prim is a Scope. */
return false;
return std::nullopt;
}
if (r_is_constant) {
*r_is_constant = !xformable.TransformMightBeTimeVarying();
}
bool is_constant = !xformable.TransformMightBeTimeVarying();
bool reset_xform_stack;
return xformable.GetLocalTransformation(r_xform, &reset_xform_stack, time);
pxr::GfMatrix4d xform;
if (!xformable.GetLocalTransformation(&xform, &reset_xform_stack, time)) {
return std::nullopt;
}
return XformResult(pxr::GfMatrix4f(xform), is_constant);
makowalski marked this conversation as resolved Outdated

As xform is already a pxr::GfMatrix4f, and that's the expected type for a XformResult, why is this cast necessary? Might be worth a comment.

As `xform` is already a `pxr::GfMatrix4f`, and that's the expected type for a `XformResult`, 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 a GfMatrix4f for the return value.

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 a `GfMatrix4f` for the return value.
}
} // namespace blender::io::usd

View File

@ -12,6 +12,8 @@
namespace blender::io::usd {
using XformResult = std::tuple<pxr::GfMatrix4f, bool>;
makowalski marked this conversation as resolved Outdated

Document what the boolean means.

Document what the boolean means.
class USDXformReader : public USDPrimReader {
private:
bool use_parent_xform_;
@ -55,13 +57,13 @@ class USDXformReader : public USDPrimReader {
* Return the USD prim's local transformation.
*
* \param time: Time code for evaluating the transform.
* \param r_xform: The transform matrix return value
* \param r_is_constant: Return value flag, set to false if the transform is
* time varying
*
* \return: Optional tuple with the following elements:
makowalski marked this conversation as resolved Outdated

An explanation fo the type should be done at the type declaration itself, not at one specific use of that type.

An explanation fo the type should be done at the type declaration itself, not at one specific use of that type.
* - The transform matrix.
makowalski marked this conversation as resolved Outdated

This should document what the returned boolean means.

This should document what the returned boolean means.
* - A boolean flag indicating whether the matrix
* is constant over time.
*/
virtual bool get_local_usd_xform(const float time,
pxr::GfMatrix4d *r_xform,
bool *r_is_constant) const;
virtual std::optional<XformResult> get_local_usd_xform(const float time) const;
};
} // namespace blender::io::usd