Merge pull request #1388 from johnhaddon/usdInvalidPrimvars

USD PrimitiveAlgo : Improve primitive variable validity checks

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)
 ========
 

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

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;
 }

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
 

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<Int64VectorData>( 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;
 }

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<V3fVectorData>( 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<V3fVectorData>( 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<V3fVectorData>( 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<V3fVectorData>( 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<V3fVectorData>( 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 );
 	}
 }
 

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 )

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)],
     }