USD PrimitiveAlgo : Fix bugs handling normals

Changes

@@ -1,3 +1,13 @@
+10.4.x.x (relative to 10.4.6.0)
+========
+
+Fixes
+-----
+
+- USD::PrimitiveAlgo :
+  - Fixed reading of primitives containing `primvars:normals`. These are now correctly loaded as a primitive variable called `N`, taking precedence over the UsdGeomPointBased `normals` attribute.
+  - Fixed writing of indexed normals so that the indexing is retained on load. Note that this means that normals are now _always_ written as `primvars:normals` and never via the UsdGeomPointBased `normals` attribute.
+
 10.4.6.0 (relative to 10.4.5.0)
 ========
 

contrib/IECoreUSD/src/IECoreUSD/PrimitiveAlgo.cpp

@@ -97,6 +97,11 @@ void IECoreUSD::PrimitiveAlgo::writePrimitiveVariable( const std::string &name,
 		writePrimitiveVariable( "st", primitiveVariable, primvarsAPI, time );
 		return;
 	}
+	else if( name == "N" && runTimeCast<const V3fVectorData>( primitiveVariable.data ) )
+	{
+		writePrimitiveVariable( "normals", primitiveVariable, primvarsAPI, time );
+		return;
+	}
 
 	const pxr::SdfValueTypeName valueTypeName = DataAlgo::valueTypeName( primitiveVariable.data.get() );
 	pxr::UsdGeomPrimvar usdPrimVar = primvarsAPI.CreatePrimvar( pxr::TfToken( name ), valueTypeName );
@@ -121,11 +126,6 @@ void IECoreUSD::PrimitiveAlgo::writePrimitiveVariable( const std::string &name,
 	{
 		pointBased.CreatePointsAttr().Set( PrimitiveAlgo::toUSDExpanded( value ), time );
 	}
-	else if( name == "N" )
-	{
-		pointBased.CreateNormalsAttr().Set( PrimitiveAlgo::toUSDExpanded( value ), time );
-		pointBased.SetNormalsInterpolation( PrimitiveAlgo::toUSD( value.interpolation ) );
-	}
 	else if( name == "velocity" )
 	{
 		pointBased.CreateVelocitiesAttr().Set( PrimitiveAlgo::toUSDExpanded( value ), time );
@@ -469,6 +469,23 @@ void IECoreUSD::PrimitiveAlgo::readPrimitiveVariables( const pxr::UsdGeomPrimvar
 			primitive->variables.erase( it );
 		}
 	}
+
+	// USD uses "normals" for normals and we use "N".
+
+	it = primitive->variables.find( "normals" );
+	if( it != primitive->variables.end() )
+	{
+		if( auto d = runTimeCast<V3fVectorData>( it->second.data ) )
+		{
+			// Force the interpretation, since some USD files
+			// use `vector3f` rather than `normal3f`. I'm looking
+			// at you, `arnold-usd`.
+			d->setInterpretation( GeometricData::Normal );
+			primitive->variables["N"] = it->second;
+			primitive->variables.erase( it );
+		}
+	}
+
 }
 
 void IECoreUSD::PrimitiveAlgo::readPrimitiveVariables( const pxr::UsdGeomPointBased &pointBased, pxr::UsdTimeCode time, IECoreScene::Primitive *primitive, const Canceller *canceller )
@@ -485,9 +502,14 @@ void IECoreUSD::PrimitiveAlgo::readPrimitiveVariables( const pxr::UsdGeomPointBa
 		}
 
 		Canceller::check( canceller );
-		if( auto n = boost::static_pointer_cast<V3fVectorData>( DataAlgo::fromUSD( pointBased.GetNormalsAttr(), time ) ) )
+		if( !primitive->variables.count( "N" ) )
 		{
-			primitive->variables["N"] = IECoreScene::PrimitiveVariable( PrimitiveAlgo::fromUSD( pointBased.GetNormalsInterpolation() ), 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 );
+			}
 		}
 	}
 

contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py

@@ -1370,7 +1370,9 @@ def testPointBasedPrimvars( self ) :
 
 		# Make sure we redirect Cortex primitive variables to the correct
 		# attributes of UsdGeomPointBased instead of writing them to
-		# arbitrary primvars.
+		# arbitrary primvars. We don't use the `normals` attribute though,
+		# since USD documents `primvars:normals` to take precedence, and
+		# it is the only way we can preserve indexed normals.
 
 		stage = pxr.Usd.Stage.Open( fileName )
 		primvarsAPI = pxr.UsdGeom.PrimvarsAPI( stage.GetPrimAtPath( "/test" ) )
@@ -1382,7 +1384,8 @@ def testPointBasedPrimvars( self ) :
 
 		usdMesh = pxr.UsdGeom.Mesh( stage.GetPrimAtPath( "/test" ) )
 		self.assertTrue( usdMesh.GetPointsAttr().HasAuthoredValue() )
-		self.assertTrue( usdMesh.GetNormalsAttr().HasAuthoredValue() )
+		self.assertFalse( usdMesh.GetNormalsAttr().HasAuthoredValue() )
+		self.assertTrue( primvarsAPI.GetPrimvar( "normals" ) )
 		self.assertTrue( usdMesh.GetVelocitiesAttr().HasAuthoredValue() )
 		if pxr.Usd.GetVersion() >= ( 0, 19, 11 ) :
 			self.assertTrue( usdMesh.GetAccelerationsAttr().HasAuthoredValue() )
@@ -3333,5 +3336,51 @@ def testInvalidPrimitiveVariables( self ) :
 		del goodCube["uv"]
 		self.assertEqual( goodCube, badCube )
 
+	def testNormalsPrimVar( self ) :
+
+		root = IECoreScene.SceneInterface.create( os.path.dirname( __file__ ) + "/data/normalsPrimVar.usda", IECore.IndexedIO.OpenMode.Read )
+		mesh = root.child( "mesh" ).readObject( 0 )
+
+		self.assertNotIn( "normals", mesh )
+		self.assertIn( "N", mesh )
+		self.assertEqual( mesh["N"].interpolation, IECoreScene.PrimitiveVariable.Interpolation.FaceVarying )
+		self.assertIsNotNone( mesh["N"].indices )
+
+	def testNormalsPrimVarBeatsNormalsAttribute( self ) :
+
+		root = IECoreScene.SceneInterface.create( os.path.dirname( __file__ ) + "/data/normalsAttributeAndPrimVar.usda", IECore.IndexedIO.OpenMode.Read )
+		mesh = root.child( "mesh" ).readObject( 0 )
+
+		self.assertNotIn( "normals", mesh )
+		self.assertIn( "N", mesh )
+		self.assertEqual( mesh["N"].data, IECore.V3fVectorData( [ imath.V3f( 0, 0, 1 ) ] * 3, IECore.GeometricData.Interpretation.Normal ) )
+		self.assertEqual( mesh["N"].interpolation, IECoreScene.PrimitiveVariable.Interpolation.FaceVarying )
+		self.assertIsNone( mesh["N"].indices )
+
+	def testRoundTripIndexedNormals( self ) :
+
+		mesh = IECoreScene.MeshPrimitive(
+			IECore.IntVectorData( [ 3, 3 ] ),
+			IECore.IntVectorData( [ 0, 2, 1, 2, 3, 1 ] ),
+			"linear",
+			IECore.V3fVectorData( [
+				imath.V3f( 0, 1, 0 ), imath.V3f( 1, 1, 0 ), imath.V3f( 0, 0, 0 ), imath.V3f( 1, 0, 0 )
+			] )
+		)
+
+		mesh["N"] = IECoreScene.PrimitiveVariable(
+			IECoreScene.PrimitiveVariable.Interpolation.FaceVarying,
+			IECore.V3fVectorData( [ imath.V3f( 0, 0, 1 ) ], IECore.GeometricData.Interpretation.Normal ),
+			IECore.IntVectorData( [ 0, 0, 0, 0, 0, 0 ] )
+		)
+
+		fileName = os.path.join( self.temporaryDirectory(), "indexedNormals.usda" )
+		root = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Write )
+		root.createChild( "mesh" ).writeObject( mesh, 0.0 )
+		del root
+
+		root = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Read )
+		self.assertEqual( root.child( "mesh").readObject( 0.0 ), mesh )
+
 if __name__ == "__main__":
 	unittest.main()

contrib/IECoreUSD/test/IECoreUSD/data/normalsAttributeAndPrimVar.usda

@@ -0,0 +1,16 @@
+#usda 1.0
+
+def Mesh "mesh"
+{
+    int[] faceVertexCounts = [3]
+    int[] faceVertexIndices = [0, 1, 3]
+    point3f[] points = [(0, 0, 0), (0, 1, 0), (1, 0, 0)]
+    normal3f[] primvars:normals = [(0, 0, 1), (0, 0, 1), (0, 0, 1)] (
+        elementSize = 3
+        interpolation = "faceVarying"
+    )
+    normal3f[] normals = [(0, 0, -1), (0, 0, -1), (0, 0, -1)] (
+        interpolation = "vertex"
+    )
+    uniform token subdivisionScheme = "none"
+}