diff --git a/Changes b/Changes index 0262cd8aa4..1f205f6beb 100644 --- a/Changes +++ b/Changes @@ -1,14 +1,27 @@ 10.5.x.x (relative to 10.5.1.0) ======== -Breaking Changes ----------------- -- IECoreMaya : Maya meshes converted from Cortex data no longer have normals set explicitly and instead relies on Maya to calculate them. +Improvements +------------ + +- USD PrimitiveAlgo : Added `readPrimitiveVariable()` utility method for reading from regular `UsdAttributes`. Fixes ---- +----- + +- USDScene : Fixed handling of invalid values on the following attributes : + - PointBased : `positions`, `normals`, `velocities`, `accelerations`. + - Curves : `widths`. + - PointInstancer : `ids`, `protoIndices`, `orientations`, `scales`, `velocities`, `accelerations`, `angularVelocities`. + - Points : `ids`, `widths`. + Invalid values are now ignored with a warning, instead of loading as invalid primitive variables. - ToMayaMeshConverter : No longer locks normals set on the Mesh from the scc. +Breaking Changes +---------------- + +- IECoreMaya : Maya meshes converted from Cortex data no longer have normals set explicitly and instead relies on Maya to calculate them. + 10.5.1.0 (relative to 10.5.0.0) ======== diff --git a/contrib/IECoreUSD/include/IECoreUSD/PrimitiveAlgo.h b/contrib/IECoreUSD/include/IECoreUSD/PrimitiveAlgo.h index f0cb73d5ad..1ef5ab0830 100644 --- a/contrib/IECoreUSD/include/IECoreUSD/PrimitiveAlgo.h +++ b/contrib/IECoreUSD/include/IECoreUSD/PrimitiveAlgo.h @@ -75,6 +75,8 @@ IECOREUSD_API pxr::TfToken toUSD( IECoreScene::PrimitiveVariable::Interpolation IECOREUSD_API void readPrimitiveVariables( const pxr::UsdGeomPrimvarsAPI &primvarsAPI, pxr::UsdTimeCode timeCode, IECoreScene::Primitive *primitive, const IECore::Canceller *canceller = nullptr ); /// As above, but also reads "P", "N" etc from `pointBased`. IECOREUSD_API void readPrimitiveVariables( const pxr::UsdGeomPointBased &pointBased, pxr::UsdTimeCode timeCode, IECoreScene::Primitive *primitive, const IECore::Canceller *canceller = nullptr ); +/// Reads the value for `attribute`, adding it as a primitive variable with the specified `name` and `interpolation`. +IECOREUSD_API void readPrimitiveVariable( const pxr::UsdAttribute &attribute, pxr::UsdTimeCode timeCode, IECoreScene::Primitive *primitive, const std::string &name, IECoreScene::PrimitiveVariable::Interpolation interpolation = IECoreScene::PrimitiveVariable::Vertex ); /// Returns true if any of the primitive variables might be animated. IECOREUSD_API bool primitiveVariablesMightBeTimeVarying( const pxr::UsdGeomPrimvarsAPI &primvarsAPI ); /// Returns true if any of the primitive variables might be animated, including the diff --git a/contrib/IECoreUSD/src/IECoreUSD/CurvesAlgo.cpp b/contrib/IECoreUSD/src/IECoreUSD/CurvesAlgo.cpp index 7343d20234..866695dcdf 100644 --- a/contrib/IECoreUSD/src/IECoreUSD/CurvesAlgo.cpp +++ b/contrib/IECoreUSD/src/IECoreUSD/CurvesAlgo.cpp @@ -109,12 +109,9 @@ IECore::ObjectPtr readCurves( pxr::UsdGeomBasisCurves &curves, pxr::UsdTimeCode PrimitiveAlgo::readPrimitiveVariables( curves, time, newCurves.get(), canceller ); Canceller::check( canceller ); - PrimitiveVariable::Interpolation widthInterpolation = PrimitiveAlgo::fromUSD( curves.GetWidthsInterpolation() ); - DataPtr widthData = DataAlgo::fromUSD( curves.GetWidthsAttr(), time, /* arrayAccepted = */ widthInterpolation != PrimitiveVariable::Constant ); - if( widthData ) - { - newCurves->variables["width"] = PrimitiveVariable( widthInterpolation, widthData ); - } + PrimitiveAlgo::readPrimitiveVariable( + curves.GetWidthsAttr(), time, newCurves.get(), "width", PrimitiveAlgo::fromUSD( curves.GetWidthsInterpolation() ) + ); return newCurves; } diff --git a/contrib/IECoreUSD/src/IECoreUSD/PointInstancerAlgo.cpp b/contrib/IECoreUSD/src/IECoreUSD/PointInstancerAlgo.cpp index 322995f8c9..ea2096e94f 100644 --- a/contrib/IECoreUSD/src/IECoreUSD/PointInstancerAlgo.cpp +++ b/contrib/IECoreUSD/src/IECoreUSD/PointInstancerAlgo.cpp @@ -65,47 +65,26 @@ IECore::ObjectPtr readPointInstancer( pxr::UsdGeomPointInstancer &pointInstancer // Per point attributes - if( auto protoIndicesData = DataAlgo::fromUSD( pointInstancer.GetProtoIndicesAttr(), time ) ) - { - Canceller::check( canceller ); - newPoints->variables["prototypeIndex"] = IECoreScene::PrimitiveVariable( IECoreScene::PrimitiveVariable::Vertex, protoIndicesData ); - } + Canceller::check( canceller ); + PrimitiveAlgo::readPrimitiveVariable( pointInstancer.GetProtoIndicesAttr(), time, newPoints.get(), "prototypeIndex" ); - if( auto idsData = DataAlgo::fromUSD( pointInstancer.GetIdsAttr(), time ) ) - { - Canceller::check( canceller ); - newPoints->variables["instanceId"] = IECoreScene::PrimitiveVariable( IECoreScene::PrimitiveVariable::Vertex, idsData ); - } + Canceller::check( canceller ); + PrimitiveAlgo::readPrimitiveVariable( pointInstancer.GetIdsAttr(), time, newPoints.get(), "instanceId" ); - if( auto orientationData = DataAlgo::fromUSD( pointInstancer.GetOrientationsAttr(), time ) ) - { - Canceller::check( canceller ); - newPoints->variables["orientation"] = IECoreScene::PrimitiveVariable( IECoreScene::PrimitiveVariable::Vertex, orientationData ); - } + Canceller::check( canceller ); + PrimitiveAlgo::readPrimitiveVariable( pointInstancer.GetOrientationsAttr(), time, newPoints.get(), "orientation" ); - if( auto scaleData = DataAlgo::fromUSD( pointInstancer.GetScalesAttr(), time ) ) - { - Canceller::check( canceller ); - newPoints->variables["scale"] = IECoreScene::PrimitiveVariable( IECoreScene::PrimitiveVariable::Vertex, scaleData ); - } + Canceller::check( canceller ); + PrimitiveAlgo::readPrimitiveVariable( pointInstancer.GetScalesAttr(), time, newPoints.get(), "scale" ); - if( auto velocityData = DataAlgo::fromUSD( pointInstancer.GetVelocitiesAttr(), time ) ) - { - Canceller::check( canceller ); - newPoints->variables["velocity"] = IECoreScene::PrimitiveVariable( IECoreScene::PrimitiveVariable::Vertex, velocityData ); - } + Canceller::check( canceller ); + PrimitiveAlgo::readPrimitiveVariable( pointInstancer.GetVelocitiesAttr(), time, newPoints.get(), "velocity" ); - if( auto accelerationData = DataAlgo::fromUSD( pointInstancer.GetAccelerationsAttr(), time ) ) - { - Canceller::check( canceller ); - newPoints->variables["acceleration"] = IECoreScene::PrimitiveVariable( IECoreScene::PrimitiveVariable::Vertex, accelerationData ); - } + Canceller::check( canceller ); + PrimitiveAlgo::readPrimitiveVariable( pointInstancer.GetAccelerationsAttr(), time, newPoints.get(), "acceleration" ); - if( auto angularVelocityData = DataAlgo::fromUSD( pointInstancer.GetAngularVelocitiesAttr(), time ) ) - { - Canceller::check( canceller ); - newPoints->variables["angularVelocity"] = IECoreScene::PrimitiveVariable( IECoreScene::PrimitiveVariable::Vertex, angularVelocityData ); - } + Canceller::check( canceller ); + PrimitiveAlgo::readPrimitiveVariable( pointInstancer.GetAngularVelocitiesAttr(), time, newPoints.get(), "angularVelocity" ); // Prototype paths diff --git a/contrib/IECoreUSD/src/IECoreUSD/PointsAlgo.cpp b/contrib/IECoreUSD/src/IECoreUSD/PointsAlgo.cpp index acae1d54eb..2512f8dc84 100644 --- a/contrib/IECoreUSD/src/IECoreUSD/PointsAlgo.cpp +++ b/contrib/IECoreUSD/src/IECoreUSD/PointsAlgo.cpp @@ -61,18 +61,12 @@ IECore::ObjectPtr readPoints( pxr::UsdGeomPoints &points, pxr::UsdTimeCode time, PrimitiveAlgo::readPrimitiveVariables( points, time, newPoints.get(), canceller ); Canceller::check( canceller ); - if( auto i = boost::static_pointer_cast( DataAlgo::fromUSD( points.GetIdsAttr(), time ) ) ) - { - newPoints->variables["id"] = IECoreScene::PrimitiveVariable( IECoreScene::PrimitiveVariable::Vertex, i ); - } + PrimitiveAlgo::readPrimitiveVariable( points.GetIdsAttr(), time, newPoints.get(), "id" ); - PrimitiveVariable::Interpolation widthInterpolation = PrimitiveAlgo::fromUSD( points.GetWidthsInterpolation() ); Canceller::check( canceller ); - DataPtr widthData = DataAlgo::fromUSD( points.GetWidthsAttr(), time, /* arrayAccepted = */ widthInterpolation != PrimitiveVariable::Constant ); - if( widthData ) - { - newPoints->variables["width"] = PrimitiveVariable( widthInterpolation, widthData ); - } + PrimitiveAlgo::readPrimitiveVariable( + points.GetWidthsAttr(), time, newPoints.get(), "width", PrimitiveAlgo::fromUSD( points.GetWidthsInterpolation() ) + ); return newPoints; } diff --git a/contrib/IECoreUSD/src/IECoreUSD/PrimitiveAlgo.cpp b/contrib/IECoreUSD/src/IECoreUSD/PrimitiveAlgo.cpp index 53accaf467..9add88c8eb 100644 --- a/contrib/IECoreUSD/src/IECoreUSD/PrimitiveAlgo.cpp +++ b/contrib/IECoreUSD/src/IECoreUSD/PrimitiveAlgo.cpp @@ -209,6 +209,17 @@ pxr::TfToken IECoreUSD::PrimitiveAlgo::toUSD( IECoreScene::PrimitiveVariable::In namespace { +void addPrimitiveVariableIfValid( IECoreScene::Primitive *primitive, const std::string &name, const IECoreScene::PrimitiveVariable &primitiveVariable, const UsdAttribute &source ) +{ + if( !primitive->isPrimitiveVariableValid( primitiveVariable ) ) + { + IECore::msg( IECore::MessageHandler::Level::Warning, "IECoreUSD::PrimitiveAlgo", boost::format( "Ignoring invalid primitive variable \"%1%\"" ) % source.GetPath().GetAsString() ); + return; + } + + primitive->variables[name] = primitiveVariable; +} + void readPrimitiveVariable( const pxr::UsdGeomPrimvar &primVar, pxr::UsdTimeCode time, const std::string &name, IECoreScene::Primitive *primitive, bool constantAcceptsArray ) { IECoreScene::PrimitiveVariable::Interpolation interpolation = IECoreUSD::PrimitiveAlgo::fromUSD( primVar.GetInterpolation() ); @@ -242,14 +253,9 @@ void readPrimitiveVariable( const pxr::UsdGeomPrimvar &primVar, pxr::UsdTimeCode indices = DataAlgo::fromUSD( srcIndices ); } - const IECoreScene::PrimitiveVariable primitiveVariable( interpolation, data, indices ); - if( !primitive->isPrimitiveVariableValid( primitiveVariable ) ) - { - IECore::msg( IECore::MessageHandler::Level::Warning, "IECoreUSD::PrimitiveAlgo", boost::format( "Skipping invalid UsdGeomPrimvar \"%1%\"" ) % primVar.GetAttr().GetPath().GetAsString() ); - return; - } - - primitive->variables[name] = primitiveVariable; + addPrimitiveVariableIfValid( + primitive, name, IECoreScene::PrimitiveVariable( interpolation, data, indices ), primVar + ); } pxr::UsdSkelCache *skelCache() @@ -387,7 +393,10 @@ bool readPrimitiveVariables( const pxr::UsdSkelRoot &skelRoot, const pxr::UsdGeo Canceller::check( canceller ); p->setInterpretation( GeometricData::Point ); - primitive->variables["P"] = IECoreScene::PrimitiveVariable( IECoreScene::PrimitiveVariable::Vertex, p ); + addPrimitiveVariableIfValid( + primitive, "P", IECoreScene::PrimitiveVariable( IECoreScene::PrimitiveVariable::Vertex, p ), + pointBased.GetPointsAttr() + ); // we'll consider normals optional and return true regardless of whether normals were skinned successfully pxr::VtVec3fArray normals; @@ -397,7 +406,10 @@ bool readPrimitiveVariables( const pxr::UsdSkelRoot &skelRoot, const pxr::UsdGeo if( auto n = boost::static_pointer_cast( DataAlgo::fromUSD( normals ) ) ) { n->setInterpretation( GeometricData::Normal ); - primitive->variables["N"] = IECoreScene::PrimitiveVariable( PrimitiveAlgo::fromUSD( pointBased.GetNormalsInterpolation() ), n ); + addPrimitiveVariableIfValid( + primitive, "N", IECoreScene::PrimitiveVariable( PrimitiveAlgo::fromUSD( pointBased.GetNormalsInterpolation() ), n ), + pointBased.GetNormalsAttr() + ); } } @@ -449,7 +461,7 @@ void IECoreUSD::PrimitiveAlgo::readPrimitiveVariables( const pxr::UsdGeomPrimvar name = "Cs"; constantAcceptsArray = false; } - readPrimitiveVariable( primVar, time, name, primitive, constantAcceptsArray ); + ::readPrimitiveVariable( primVar, time, name, primitive, constantAcceptsArray ); } // USD uses "st" for the primary texture coordinates and we use "uv", @@ -496,32 +508,29 @@ void IECoreUSD::PrimitiveAlgo::readPrimitiveVariables( const pxr::UsdGeomPointBa if( !skelRoot || !::readPrimitiveVariables( skelRoot, pointBased, time, primitive, canceller ) ) { Canceller::check( canceller ); - if( auto p = boost::static_pointer_cast( DataAlgo::fromUSD( pointBased.GetPointsAttr(), time ) ) ) - { - primitive->variables["P"] = IECoreScene::PrimitiveVariable( IECoreScene::PrimitiveVariable::Vertex, p ); - } + readPrimitiveVariable( pointBased.GetPointsAttr(), time, primitive, "P" ); Canceller::check( canceller ); if( !primitive->variables.count( "N" ) ) { // Only load `PointBased::GetNormalsAttr()` if we didn't already load `primvars:normals`. // From the USD API docs : "If normals and primvars:normals are both specified, the latter has precedence." - if( auto n = boost::static_pointer_cast( DataAlgo::fromUSD( pointBased.GetNormalsAttr(), time ) ) ) - { - primitive->variables["N"] = IECoreScene::PrimitiveVariable( PrimitiveAlgo::fromUSD( pointBased.GetNormalsInterpolation() ), n ); - } + readPrimitiveVariable( pointBased.GetNormalsAttr(), time, primitive, "N", PrimitiveAlgo::fromUSD( pointBased.GetNormalsInterpolation() ) ); } } Canceller::check( canceller ); - if( auto v = boost::static_pointer_cast( DataAlgo::fromUSD( pointBased.GetVelocitiesAttr(), time ) ) ) - { - primitive->variables["velocity"] = IECoreScene::PrimitiveVariable( IECoreScene::PrimitiveVariable::Vertex, v ); - } + readPrimitiveVariable( pointBased.GetVelocitiesAttr(), time, primitive, "velocity" ); + + Canceller::check( canceller ); + readPrimitiveVariable( pointBased.GetAccelerationsAttr(), time, primitive, "acceleration" ); +} - if( auto a = boost::static_pointer_cast( DataAlgo::fromUSD( pointBased.GetAccelerationsAttr(), time ) ) ) +void IECoreUSD::PrimitiveAlgo::readPrimitiveVariable( const pxr::UsdAttribute &attribute, pxr::UsdTimeCode timeCode, IECoreScene::Primitive *primitive, const std::string &name, IECoreScene::PrimitiveVariable::Interpolation interpolation ) +{ + if( auto d = DataAlgo::fromUSD( attribute, timeCode, /* arrayAccepted = */ interpolation != PrimitiveVariable::Constant ) ) { - primitive->variables["acceleration"] = IECoreScene::PrimitiveVariable( IECoreScene::PrimitiveVariable::Vertex, a ); + addPrimitiveVariableIfValid( primitive, name, IECoreScene::PrimitiveVariable( interpolation, d ), attribute ); } } diff --git a/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py b/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py index 42115b83ff..6eb6b95fa3 100644 --- a/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py +++ b/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py @@ -736,10 +736,11 @@ def testCanWriteCurves( self ) : for i in range( 16 ) : for j in range( 16 ) : vertsPerCurveData.append( 4 ); - for k in range( 3 ) : + for k in range( 4 ) : positionData.append( imath.V3f( [i, j, k] ) ) curvesPrimitive = IECoreScene.CurvesPrimitive( IECore.IntVectorData( vertsPerCurveData ), IECore.CubicBasisf.linear(), False, IECore.V3fVectorData( positionData ) ) + self.assertTrue( curvesPrimitive.arePrimitiveVariablesValid() ) root.writeObject( curvesPrimitive, 0.0 ) @@ -3544,7 +3545,7 @@ def testInvalidPrimitiveVariables( self ) : self.assertEqual( len( mh.messages ), 1 ) self.assertEqual( mh.messages[0].level, IECore.Msg.Level.Warning ) - self.assertEqual( mh.messages[0].message, "Skipping invalid UsdGeomPrimvar \"/pCube1.primvars:st\"" ) + self.assertEqual( mh.messages[0].message, "Ignoring invalid primitive variable \"/pCube1.primvars:st\"" ) self.assertNotIn( "uv", badCube ) self.assertTrue( badCube.arePrimitiveVariablesValid() ) @@ -3553,6 +3554,82 @@ def testInvalidPrimitiveVariables( self ) : del goodCube["uv"] self.assertEqual( goodCube, badCube ) + def testInvalidPointBasedAttributes( self ) : + + fileName = os.path.join( self.temporaryDirectory(), "pointBased.usda" ) + stage = pxr.Usd.Stage.CreateNew( fileName ) + + curves = pxr.UsdGeom.BasisCurves.Define( stage, "/curves" ) + curves.CreateCurveVertexCountsAttr().Set( [ 4 ] ) + curves.CreatePointsAttr().Set( [ ( 1, 1, 1 ), ( 2, 2, 2 ) ] ) + curves.CreateVelocitiesAttr().Set( [ ( 1, 1, 1 ) ] ) + curves.CreateAccelerationsAttr().Set( [ ( 0, 0, 0 ) ] ) + curves.CreateNormalsAttr().Set( [ ( 1, 0, 0 ) ] ) + + stage.GetRootLayer().Save() + + root = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Read ) + with IECore.CapturingMessageHandler() as mh : + curves = root.child( "curves" ).readObject( 0 ) + + # No valid primitive variables. + self.assertEqual( curves.keys(), [] ) + # And a warning message for every one we tried to load. + messages = { m.message for m in mh.messages } + for attribute in [ + "/curves.points", + "/curves.velocities", + "/curves.accelerations", + "/curves.normals", + ] : + self.assertIn( + f"Ignoring invalid primitive variable \"{attribute}\"", + messages + ) + + def testInvalidCurvesAttributes( self ) : + + fileName = os.path.join( self.temporaryDirectory(), "curves.usda" ) + stage = pxr.Usd.Stage.CreateNew( fileName ) + + curves = pxr.UsdGeom.BasisCurves.Define( stage, "/curves" ) + curves.CreateCurveVertexCountsAttr().Set( [ 4 ] ) + curves.CreateWidthsAttr().Set( [ 1, 2 ] ) + + stage.GetRootLayer().Save() + + root = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Read ) + with IECore.CapturingMessageHandler() as mh : + curves = root.child( "curves" ).readObject( 0 ) + + self.assertEqual( len( mh.messages ), 1 ) + self.assertEqual( + mh.messages[0].message, f"Ignoring invalid primitive variable \"/curves.widths\"", + ) + self.assertNotIn( "width", curves ) + + def testInvalidPointsAttributes( self ) : + + fileName = os.path.join( self.temporaryDirectory(), "points.usda" ) + stage = pxr.Usd.Stage.CreateNew( fileName ) + + points = pxr.UsdGeom.Points.Define( stage, "/points" ) + points.CreatePointsAttr().Set( [ ( 1, 1, 1 ), ( 2, 2, 2 ), ( 3, 3, 3 ) ] ) + points.CreateWidthsAttr().Set( [ 1, 2 ] ) + points.CreateIdsAttr().Set( [ 1, 2 ] ) + + stage.GetRootLayer().Save() + + root = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Read ) + with IECore.CapturingMessageHandler() as mh : + points = root.child( "points" ).readObject( 0 ) + + self.assertEqual( len( mh.messages ), 2 ) + self.assertEqual( mh.messages[0].message, f"Ignoring invalid primitive variable \"/points.ids\"" ) + self.assertEqual( mh.messages[1].message, f"Ignoring invalid primitive variable \"/points.widths\"" ) + self.assertNotIn( "id", points ) + self.assertNotIn( "width", points ) + def testNormalsPrimVar( self ) : root = IECoreScene.SceneInterface.create( os.path.dirname( __file__ ) + "/data/normalsPrimVar.usda", IECore.IndexedIO.OpenMode.Read ) diff --git a/contrib/IECoreUSD/test/IECoreUSD/data/curves.usda b/contrib/IECoreUSD/test/IECoreUSD/data/curves.usda index fd82a0818a..bb289bb5af 100644 --- a/contrib/IECoreUSD/test/IECoreUSD/data/curves.usda +++ b/contrib/IECoreUSD/test/IECoreUSD/data/curves.usda @@ -3,6 +3,7 @@ def BasisCurves "borderLines" { float3[] extent = [(-5, -5, -0), (5, 5, 0)] + int[] curveVertexCounts = [ 4 ] point3f[] points.timeSamples = { 1.0000000298023224: [(-5, -5, 0), (5, -5, 0), (5, 5, 0), (-5, 5, 0)], }