Merge pull request #1661 from ablev-nv/set_from_camera_crash

[usdGeom] avoid crash from UsdGeomCamera::SetFromCamera

(Internal change: 2205282)

pxr/usd/usdGeom/CMakeLists.txt

@@ -186,6 +186,11 @@ pxr_install_test_dir(
     DEST testUsdGeomBBoxCache
 )
 
+pxr_install_test_dir(
+    SRC testenv/testUsdGeomCamera
+    DEST testUsdGeomCamera
+)
+
 pxr_install_test_dir(
     SRC testenv/testUsdGeomConsts
     DEST testUsdGeomConsts

pxr/usd/usdGeom/camera.cpp

@@ -552,7 +552,15 @@ UsdGeomCamera::SetFromCamera(const GfCamera &camera, const UsdTimeCode &time)
 
     const GfMatrix4d camMatrix = camera.GetTransform() * parentToWorldInverse;
 
-    MakeMatrixXform().Set(camMatrix, time);
+    UsdGeomXformOp xformOp = MakeMatrixXform();
+    if (!xformOp) {
+        // The returned xformOp may be invalid if there are xform op
+        // opinions in the composed layer stack stronger than that of
+        // the current edit target.
+        return;
+    }
+    xformOp.Set(camMatrix, time);
+
     GetProjectionAttr().Set(_ProjectionToToken(camera.GetProjection()), time);
     GetHorizontalApertureAttr().Set(camera.GetHorizontalAperture(), time);
     GetVerticalApertureAttr().Set(camera.GetVerticalAperture(), time);

pxr/usd/usdGeom/camera.h

@@ -552,6 +552,25 @@ class UsdGeomCamera : public UsdGeomXformable
     GfCamera GetCamera(const UsdTimeCode &time) const;
 
     /// Write attribute values from \p camera for \p time.
+    /// These attributes will be updated:
+    ///  - projection
+    ///  - horizontalAperture
+    ///  - horizontalApertureOffset
+    ///  - verticalAperture
+    ///  - verticalApertureOffset
+    ///  - focalLength
+    ///  - clippingRange
+    ///  - clippingPlanes
+    ///  - fStop
+    ///  - focalDistance
+    ///  - xformOpOrder and xformOp:transform
+    /// 
+    /// \note This will clear any existing xformOpOrder and replace
+    /// it with a single xformOp:transform entry. The xformOp:transform
+    /// property is created or updated here to match the transform
+    /// on \p camera . This operation will fail if there are stronger xform op
+    /// opinions in the composed layer stack that are stronger than that of
+    /// the current edit target.
     ///
     USDGEOM_API
     void SetFromCamera(const GfCamera &camera, const UsdTimeCode &time);

pxr/usd/usdGeom/testenv/testUsdGeomCamera.py

@@ -22,7 +22,7 @@
 # KIND, either express or implied. See the Apache License for the specific
 # language governing permissions and limitations under the Apache License.
 
-from pxr import Gf, Usd, UsdGeom
+from pxr import Gf, Usd, UsdGeom, Sdf, Tf
 import unittest, math
 
 class TestUsdGeomCamera(unittest.TestCase):
@@ -128,5 +128,33 @@ def test_SetFromCamera(self):
         for a, e in zip(actual, expected):
             self.assertTrue(Gf.IsClose(a, e, 1e-2))
 
+    def test_SetFromCameraWithComposition(self):
+        stage = Usd.Stage.Open("layers_a_b.usda")
+        layerA = Sdf.Layer.FindOrOpen("a.usda")
+        layerB = Sdf.Layer.FindOrOpen("b.usda")
+        stage.SetEditTarget(layerB)
+
+        usdCamera = UsdGeom.Camera.Define(stage, '/camera')
+
+        camera = Gf.Camera()
+        newXform = Gf.Matrix4d().SetTranslate(Gf.Vec3d(100, 200, 300))
+        camera.transform = newXform
+        camera.horizontalAperture = 500.0
+
+        # Verify that trying to SetFromCamera from a weaker edit target does not crash,
+        # and does not modify any existing camera attributes.
+        usdCamera.SetFromCamera(camera, 1.0)
+        self.assertEqual(usdCamera.GetHorizontalApertureAttr().Get(1.0), 1.0)
+
+        # Now use the stronger layer
+        stage.SetEditTarget(layerA)
+
+        # This should succeed
+        usdCamera.SetFromCamera(camera, 1.0)
+
+        self.assertEqual(usdCamera.GetHorizontalApertureAttr().Get(1.0), 500.0)
+        self.assertEqual(usdCamera.ComputeLocalToWorldTransform(1.0), newXform)
+
+
 if __name__ == '__main__':
     unittest.main()

pxr/usd/usdGeom/testenv/testUsdGeomCamera/a.usda

@@ -0,0 +1,13 @@
+#usda 1.0
+(
+    defaultPrim = "camera"
+)
+
+over "camera"
+{
+    matrix4d xformOp:transform = ((1, 0, 0, 0), 
+                                  (0, 1, 0, 0),
+                                  (0, 0, 1, 0),
+                                  (0, 0, 0, 1))
+    uniform token[] xformOpOrder = ["xformOp:transform"]
+}

pxr/usd/usdGeom/testenv/testUsdGeomCamera/b.usda

@@ -0,0 +1,9 @@
+#usda 1.0
+(
+    defaultPrim = "camera"
+)
+
+def Camera "camera"
+{
+    float horizontalAperture = 1.0
+}

pxr/usd/usdGeom/testenv/testUsdGeomCamera/layers_a_b.usda

@@ -0,0 +1,9 @@
+#usda 1.0
+(
+    defaultPrim = "camera"
+    subLayers = [
+        @./a.usda@,
+        @./b.usda@
+    ]
+)
+

pxr/usd/usdGeom/xformable.cpp

@@ -428,6 +428,12 @@ UsdGeomXformOp
 UsdGeomXformable::MakeMatrixXform() const
 {
     ClearXformOpOrder();
+    bool unused = false;
+    if (!GetOrderedXformOps(&unused).empty()) {
+        TF_WARN("Could not clear xformOpOrder for <%s>",
+            GetPrim().GetPath().GetText());
+        return UsdGeomXformOp();
+    }
     return AddTransformOp();
 }