NVIDIA: Defensive check for UsdSkel_SkelAnimationQueryImp to avoid crash

Removing animation prim with visiblity update to UsdSkelRoot may cause a crash.

Closes PixarAnimationStudios#3544

(Internal change: 2369232)

Author: roggiezhang-nv

pxr/usd/usdSkel/animQueryImpl.cpp

@@ -73,6 +73,10 @@ class UsdSkel_SkelAnimationQueryImpl : public UsdSkel_AnimQueryImpl
     bool BlendShapeWeightsMightBeTimeVarying() const override;
 
 private:
+    bool _VerifyAnimation() const {
+        return TF_VERIFY(_anim, "PackedJointAnimation schema object is invalid.");
+    }
+
     template <typename Matrix4>
     bool _ComputeJointLocalTransforms(VtArray<Matrix4>* xforms,
                                       UsdTimeCode time) const;
@@ -170,6 +174,10 @@ UsdSkel_SkelAnimationQueryImpl::GetJointTransformTimeSamples(
     const GfInterval& interval,
     std::vector<double>* times) const
 {
+    if (!_VerifyAnimation()) {
+        return false;
+    }
+    
     return UsdAttribute::GetUnionedTimeSamplesInInterval(
         {_translations.GetAttribute(),
          _rotations.GetAttribute(),
@@ -180,6 +188,10 @@ bool
 UsdSkel_SkelAnimationQueryImpl::GetJointTransformAttributes(
     std::vector<UsdAttribute>* attrs) const
 {
+    if (!_VerifyAnimation()) {
+        return false;
+    }
+
     attrs->push_back(_translations.GetAttribute());
     attrs->push_back(_rotations.GetAttribute());
     attrs->push_back(_scales.GetAttribute());
@@ -190,6 +202,10 @@ UsdSkel_SkelAnimationQueryImpl::GetJointTransformAttributes(
 bool
 UsdSkel_SkelAnimationQueryImpl::JointTransformsMightBeTimeVarying() const
 {
+    if (!_VerifyAnimation()) {
+        return false;
+    }
+    
     return _translations.ValueMightBeTimeVarying() ||
            _rotations.ValueMightBeTimeVarying() ||
            _scales.ValueMightBeTimeVarying();
@@ -201,10 +217,10 @@ UsdSkel_SkelAnimationQueryImpl::ComputeBlendShapeWeights(
     VtFloatArray* weights,
     UsdTimeCode time) const
 {
-    if (TF_VERIFY(_anim, "PackedJointAnimation schema object is invalid.")) {
-        return _blendShapeWeights.Get(weights, time);
+    if (!_VerifyAnimation()) {
+        return false;
     }
-    return false;
+    return _blendShapeWeights.Get(weights, time);
 }
 
 
@@ -213,6 +229,9 @@ UsdSkel_SkelAnimationQueryImpl::GetBlendShapeWeightTimeSamples(
     const GfInterval& interval,
     std::vector<double>* times) const
 {
+    if (!_VerifyAnimation()) {
+        return false;
+    }
     return _blendShapeWeights.GetTimeSamplesInInterval(interval, times);
 }
 
@@ -221,13 +240,19 @@ bool
 UsdSkel_SkelAnimationQueryImpl::GetBlendShapeWeightAttributes(
     std::vector<UsdAttribute>* attrs) const
 {
+    if (!_VerifyAnimation()) {
+        return false;
+    }
     attrs->push_back(_blendShapeWeights.GetAttribute());
     return true;
 }
 
 bool
 UsdSkel_SkelAnimationQueryImpl::BlendShapeWeightsMightBeTimeVarying() const
 {
+    if (!_VerifyAnimation()) {
+        return false;
+    }
     return _blendShapeWeights.ValueMightBeTimeVarying();
 }
 

pxr/usdImaging/usdSkelImaging/CMakeLists.txt

@@ -53,6 +53,7 @@ pxr_build_test(testUsdSkelImagingChanges
     LIBRARIES
         usdImaging
         usdShade
+        usdSkel
         usd
         sdf
         hd
@@ -71,4 +72,6 @@ pxr_install_test_dir(
 pxr_register_test(testUsdSkelImagingChanges
     COMMAND "${CMAKE_INSTALL_PREFIX}/tests/testUsdSkelImagingChanges"
     EXPECTED_RETURN_CODE 0
-)
+    ENV
+      TF_FATAL_VERIFY=0
+)

pxr/usdImaging/usdSkelImaging/testenv/testUsdSkelImagingChanges.cpp

@@ -5,12 +5,17 @@
 // https://openusd.org/license.
 //
 
+#include "pxr/base/tf/errorMark.h"
 #include "pxr/imaging/hd/renderIndex.h"
 #include "pxr/imaging/hd/unitTestNullRenderDelegate.h"
 
 #include "pxr/usd/sdf/path.h"
+#include "pxr/usd/usd/editContext.h"
 #include "pxr/usd/usd/prim.h"
 #include "pxr/usd/usd/stage.h"
+#include "pxr/usd/usdSkel/animation.h"
+#include "pxr/usd/usdSkel/bindingAPI.h"
+#include "pxr/usd/usdSkel/root.h"
 
 #include "pxr/usdImaging/usdImaging/delegate.h"
 
@@ -47,7 +52,8 @@ SwitchBoundMaterialTest()
     // Switch the material for box1
     auto box1Prim = stage->GetPrimAtPath(SdfPath("/Root/Geometry/box1"));
     TF_AXIOM(box1Prim);
-    auto materialBinding = box1Prim.GetRelationship(UsdShadeTokens->materialBinding);
+    auto materialBinding =
+        box1Prim.GetRelationship(UsdShadeTokens->materialBinding);
     TF_AXIOM(materialBinding);
     materialBinding.SetTargets({SdfPath("/Root/Looks/green")});
 
@@ -71,11 +77,68 @@ SwitchBoundMaterialTest()
     TF_AXIOM(dirtyBits != HdChangeTracker::Clean);
 }
 
+static void
+SkelAnimUpdateTest()
+{
+    std::cout << "-------------------------------------------------------\n";
+    std::cout << "SkelAnimUpdateTest\n";
+    std::cout << "-------------------------------------------------------\n";
+
+    const std::string usdPath = "animation.usda";
+    UsdStageRefPtr stage = UsdStage::Open(usdPath);
+    TF_AXIOM(stage);
+    
+    // Bring up Hydra
+    Hd_UnitTestNullRenderDelegate renderDelegate;
+    std::unique_ptr<HdRenderIndex>
+        renderIndex(HdRenderIndex::New(&renderDelegate, HdDriverVector()));
+    auto delegate = std::make_unique<UsdImagingDelegate>(renderIndex.get(),
+                               SdfPath::AbsoluteRootPath());
+    delegate->Populate(stage->GetPseudoRoot());
+    delegate->SetTime(0);
+    delegate->SyncAll(true);
+    
+    UsdEditContext editContext(stage, stage->GetSessionLayer());
+    SdfPath animationPath("/Animation");
+    UsdSkelAnimation skelAnimation =
+        UsdSkelAnimation::Define(stage, animationPath);
+    UsdPrim animationPrim = skelAnimation.GetPrim();
+    TF_AXIOM(animationPrim);
+
+    // Update skeleton binding
+    UsdPrim skeletonPrim = stage->GetPrimAtPath(SdfPath("/Root/Skeleton"));
+    UsdSkelBindingAPI skeletonBindingAPI = UsdSkelBindingAPI(skeletonPrim);
+    skeletonBindingAPI.GetAnimationSourceRel()
+        .SetTargets({animationPath});
+    delegate->ApplyPendingUpdates();
+    delegate->SyncAll(true);
+
+    // Remove animation and update skelRoot's visibility
+    stage->RemovePrim(animationPath);
+    skeletonBindingAPI.GetAnimationSourceRel().ClearTargets(false); 
+    UsdPrim rootPrim = stage->GetPrimAtPath(SdfPath("/Root"));
+    UsdSkelRoot skelRoot = UsdSkelRoot(rootPrim);
+    skelRoot.GetVisibilityAttr().Set(UsdGeomTokens->inherited);
+
+    // Expect errors because animation was removed
+    TfErrorMark errorMark;
+
+    delegate->ApplyPendingUpdates();
+    delegate->SyncAll(true);
+
+    size_t numErrors = 0;
+    errorMark.GetBegin(&numErrors);
+    TF_AXIOM(numErrors == 2);
+
+    errorMark.Clear();
+}
+
 int main()
 {
     TfErrorMark mark;
 
     SwitchBoundMaterialTest();
+    SkelAnimUpdateTest();
 
     if (TF_AXIOM(mark.IsClean())) {
         std::cout << "OK" << std::endl;

pxr/usdImaging/usdSkelImaging/testenv/testUsdSkelImagingChanges/animation.usda

@@ -0,0 +1,46 @@
+#usda 1.0
+(
+    defaultPrim = "Root"
+    endTimeCode = 120
+    metersPerUnit = 0.01
+    startTimeCode = 0
+    timeCodesPerSecond = 24
+    upAxis = "Z"
+)
+
+def SkelRoot "Root" (
+    prepend apiSchemas = ["SkelBindingAPI"]
+)
+{
+    matrix4d xformOp:transform = ( (1, 0, 0, 0), (0, 1, 0, 0), (0, 0, 1, 0), (0, 0, 0, 1) )
+    uniform token[] xformOpOrder = ["xformOp:transform"]
+
+    def Mesh "box" (
+        prepend apiSchemas = ["SkelBindingAPI"]
+    )
+    {
+        int[] faceVertexCounts = [4, 4, 4, 4, 4, 4]
+        int[] faceVertexIndices = [0, 1, 3, 2, 4, 5, 6, 7, 1, 2, 5, 6, 0, 4, 7, 3, 0, 1, 5, 4, 3, 7, 6, 2]
+        token orientation = "rightHanded"
+        point3f[] points = [(-0.6, -0.6, 0), (-0.6, 0.6, 0), (0.6, 0.6, 0), (0.6, -0.6, 0), (-0.6, -0.6, 0.14), (-0.6, 0.6, 0.14), (0.6, 0.6, 0.14), (0.6, -0.6, 0.14)]
+        
+        int[] primvars:skel:jointIndices = [0, 0, 0, 0] (interpolation = "vertex")
+        float[] primvars:skel:jointWeights = [1., 1., 1., 1.] (interpolation = "vertex")
+        prepend rel skel:skeleton = </Root/Skeleton>
+        bool doubleSided = 1
+    }
+
+    def Skeleton "Skeleton" (
+        prepend apiSchemas = ["SkelBindingAPI"]
+    )
+    {
+        uniform token[] joints = ["Root"]
+        uniform matrix4d[] bindTransforms = [
+            ((1., 0., 0., 0.), (0., 1., 0., 0.), (0., 0., 1., 0.), (0.0, 0.0, 0.0, 1.)),    # Root
+        ]
+        uniform matrix4d[] restTransforms = [
+            ((1., 0., 0., 0.), (0., 1., 0., 0.), (0., 0., 1., 0.), (0., 0., 0., 1.)),    # Root
+        ]
+    }
+}
+