Merge pull request #1413 from danieldresser-ie/meshBoundary

johnhaddon · web-flow · commit ff821fb6c1cb · 2024-04-19T10:24:26.000+01:00
MeshPrimitive : Add subdivision options
diff --git a/Changes b/Changes
@@ -1,6 +1,15 @@
 10.5.x.x (relative to 10.5.6.2)
 ========
 
+Features
+--------
+
+- MeshPrimitive : Added interpolateBoundary, faceVaryingLinearInterpolation and triangleSubdivisionRule properties for controlling the shape of the subdivision limit surface.
+
+Fixes
+-----
+
+- MeshPrimitive : Removed `interpolation` from topologyHash. The topologyHash should only include things which affect the layout of primitive variables.
 
 
 10.5.6.2 (relative to 10.5.6.1)
diff --git a/SConstruct b/SConstruct
@@ -96,7 +96,7 @@ o.Add(
 o.Add(
 	"CXXFLAGS",
 	"The extra flags to pass to the C++ compiler during compilation.",
-	[ "-pipe", "-Wall", "-Wextra" ] if Environment()["PLATFORM"] != "win32" else [ "/permissive-", "/D_USE_MATH_DEFINES", "/Zc:externC-", "/DBOOST_ALL_NO_LIB" ],
+	[ "-pipe", "-Wall", "-Wextra", "-Wsuggest-override" ] if Environment()["PLATFORM"] != "win32" else [ "/permissive-", "/D_USE_MATH_DEFINES", "/Zc:externC-", "/DBOOST_ALL_NO_LIB" ],
 )
 
 o.Add(
diff --git a/contrib/IECoreUSD/src/IECoreUSD/MeshAlgo.cpp b/contrib/IECoreUSD/src/IECoreUSD/MeshAlgo.cpp
@@ -76,8 +76,30 @@ IECore::ObjectPtr readMesh( pxr::UsdGeomMesh &mesh, pxr::UsdTimeCode time, const
 
 	if( subdivScheme == pxr::UsdGeomTokens->catmullClark )
 	{
-		newMesh->setInterpolation( "catmullClark" );
+		newMesh->setInterpolation( IECoreScene::MeshPrimitive::interpolationCatmullClark.string() );
 	}
+	else if( subdivScheme == pxr::UsdGeomTokens->loop )
+	{
+		newMesh->setInterpolation( IECoreScene::MeshPrimitive::interpolationLoop.string() );
+	}
+	else
+	{
+		// For "none", we currently use the default value of "linear". It would probably be preferrable if
+		// we used the name "none", since this is different from "bilinear", which would indicate that
+		// subdivision is being requested, but without altering the shape of the limit surface.
+	}
+
+	pxr::TfToken interpolateBoundary;
+	mesh.GetInterpolateBoundaryAttr().Get( &interpolateBoundary, time );
+	newMesh->setInterpolateBoundary( interpolateBoundary.GetString() );
+
+	pxr::TfToken faceVaryingLinearInterpolation;
+	mesh.GetFaceVaryingLinearInterpolationAttr().Get( &faceVaryingLinearInterpolation, time );
+	newMesh->setFaceVaryingLinearInterpolation( faceVaryingLinearInterpolation.GetString() );
+
+	pxr::TfToken triangleSubdivisionRule;
+	mesh.GetTriangleSubdivisionRuleAttr().Get( &triangleSubdivisionRule, time );
+	newMesh->setTriangleSubdivisionRule( triangleSubdivisionRule.GetString() );
 
 	// Corners
 
@@ -170,15 +192,23 @@ bool writeMesh( const IECoreScene::MeshPrimitive *mesh, const pxr::UsdStagePtr &
 
 	// Interpolation
 
-	if( mesh->interpolation() == std::string( "catmullClark" ) )
+	if( mesh->interpolation() == IECoreScene::MeshPrimitive::interpolationCatmullClark.string() )
 	{
 		usdMesh.CreateSubdivisionSchemeAttr().Set( pxr::UsdGeomTokens->catmullClark );
 	}
+	else if( mesh->interpolation() == IECoreScene::MeshPrimitive::interpolationLoop.string() )
+	{
+		usdMesh.CreateSubdivisionSchemeAttr().Set( pxr::UsdGeomTokens->loop );
+	}
 	else
 	{
 		usdMesh.CreateSubdivisionSchemeAttr().Set( pxr::UsdGeomTokens->none );
 	}
 
+	usdMesh.CreateInterpolateBoundaryAttr().Set( pxr::TfToken( mesh->getInterpolateBoundary().string() ), time );
+	usdMesh.CreateFaceVaryingLinearInterpolationAttr().Set( pxr::TfToken( mesh->getFaceVaryingLinearInterpolation().string() ), time );
+	usdMesh.CreateTriangleSubdivisionRuleAttr().Set( pxr::TfToken( mesh->getTriangleSubdivisionRule().string() ), time );
+
 	// Corners
 
 	if( mesh->cornerIds()->readable().size() )
diff --git a/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py b/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py
@@ -843,6 +843,96 @@ def testCanWriteSubD( self ):
 
 		self.assertEqual(readChild.readObject( 0.0 ).interpolation, "catmullClark")
 
+	def testSubdOptions( self ) :
+
+		fileName = os.path.join( self.temporaryDirectory(), "test.usda" )
+		resaveFileName = os.path.join( self.temporaryDirectory(), "resave.usda" )
+
+		# We need a list of all the values from USD we should support. There probably should be a
+		# more direct way to get this, but I have already wasted far, far too much time trying to
+		# understand which USD API to use.
+		dummyStage = pxr.Usd.Stage.CreateInMemory()
+		dummyMesh = pxr.UsdGeom.Mesh.Define( dummyStage, "/mesh" )
+		allowedSubScheme = dummyMesh.GetSubdivisionSchemeAttr().GetMetadata( "allowedTokens" )
+		allowedIB = dummyMesh.GetInterpolateBoundaryAttr().GetMetadata( "allowedTokens" )
+		allowedFVLI = dummyMesh.GetFaceVaryingLinearInterpolationAttr().GetMetadata( "allowedTokens" )
+		allowedTS = dummyMesh.GetTriangleSubdivisionRuleAttr().GetMetadata( "allowedTokens" )
+
+		del dummyMesh
+		del dummyStage
+
+		for property, allowed in [
+			( "subdivisionScheme", allowedSubScheme ),
+			( "interpolateBoundary", allowedIB ),
+			( "faceVaryingLinearInterpolation", allowedFVLI ),
+			( "triangleSubdivisionRule", allowedTS ),
+			
+		]:
+			for value in allowed:
+
+				if property == "subdivisionScheme" and value == "bilinear":
+					# We know we don't support this
+					continue
+
+				stage = pxr.Usd.Stage.CreateNew( fileName )
+				mesh = pxr.UsdGeom.Mesh.Define( stage, "/mesh" )
+				if property == "subdivisionScheme":
+					mesh.CreateSubdivisionSchemeAttr().Set( value )
+				else:
+					mesh.CreateSubdivisionSchemeAttr().Set( "catmullClark" )
+
+				if property == "interpolateBoundary":
+					mesh.CreateInterpolateBoundaryAttr().Set( value, 0.0 )
+
+				if property == "faceVaryingLinearInterpolation":
+					mesh.CreateFaceVaryingLinearInterpolationAttr().Set( value, 0.0 )
+
+				if property == "triangleSubdivisionRule":
+					mesh.CreateTriangleSubdivisionRuleAttr().Set( value, 0.0 )
+				
+				stage.GetRootLayer().Save()
+				del stage
+
+				root = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Read )
+
+				cortexMesh = root.child( "mesh" ).readObject( 0.0 )
+				if property == "subdivisionScheme":
+					if value == "none":
+						self.assertEqual( cortexMesh.interpolation, "linear" )
+					else:
+						self.assertEqual( cortexMesh.interpolation, value )
+				elif property == "interpolateBoundary":
+					self.assertEqual( cortexMesh.getInterpolateBoundary(), value )
+				elif property == "faceVaryingLinearInterpolation":
+					self.assertEqual( cortexMesh.getFaceVaryingLinearInterpolation(), value )
+				elif property == "triangleSubdivisionRule":
+					self.assertEqual( cortexMesh.getTriangleSubdivisionRule(), value )
+
+				sceneWrite = IECoreScene.SceneInterface.create( resaveFileName, IECore.IndexedIO.OpenMode.Write )
+				root = sceneWrite.createChild( "root" )
+				child = root.createChild( "mesh" )
+
+				child.writeObject ( cortexMesh, 0.0 )
+
+				del child
+				del root
+				del sceneWrite
+
+				rereadFile = pxr.Usd.Stage.Open( resaveFileName )
+				rereadMesh = pxr.UsdGeom.Mesh.Get( rereadFile, "/root/mesh" )
+
+				if property == "subdivisionScheme":
+					self.assertEqual( rereadMesh.GetSubdivisionSchemeAttr().Get( 0.0 ), value )
+				elif property == "interpolateBoundary":
+					self.assertEqual( rereadMesh.GetInterpolateBoundaryAttr().Get( 0.0 ), value )
+				elif property == "faceVaryingLinearInterpolation":
+					self.assertEqual( rereadMesh.GetFaceVaryingLinearInterpolationAttr().Get( 0.0 ), value )
+				elif property == "triangleSubdivisionRule":
+					self.assertEqual( rereadMesh.GetTriangleSubdivisionRuleAttr().Get( 0.0 ), value )
+
+				del rereadMesh
+				del rereadFile
+
 	def testCanWriteAnimatedPrimitiveVariable ( self ):
 
 		fileName = os.path.join( self.temporaryDirectory(), "usd_animated_primvar.usda" )
diff --git a/include/IECore/EuclideanToSphericalTransform.h b/include/IECore/EuclideanToSphericalTransform.h
@@ -62,7 +62,7 @@ class EuclideanToSphericalTransform : public SpaceTransform< F, T >
 		EuclideanToSphericalTransform();
 
 		/// Perform the conversion.
-		virtual T transform( const F &f );
+		T transform( const F &f ) override;
 
 		/// Returns an instance of a class able to perform the inverse conversion
 		InverseType inverse() const;
diff --git a/include/IECore/SphericalToEuclideanTransform.h b/include/IECore/SphericalToEuclideanTransform.h
@@ -62,7 +62,7 @@ class SphericalToEuclideanTransform : public SpaceTransform< F, T >
 		SphericalToEuclideanTransform();
 
 		/// Perform the conversion. The x component should be in the range [0,2*M_PI] and the second [0,M_PI]
-		virtual T transform( const F &f );
+		T transform( const F &f ) override;
 
 		/// Returns an instance of a class able to perform the inverse conversion
 		InverseType inverse() const;
diff --git a/include/IECoreScene/MeshPrimitive.h b/include/IECoreScene/MeshPrimitive.h
@@ -38,6 +38,7 @@
 #include "IECoreScene/Export.h"
 #include "IECoreScene/Primitive.h"
 
+#include "IECore/InternedString.h"
 #include "IECore/VectorTypedData.h"
 
 namespace IECoreScene
@@ -61,12 +62,30 @@ class IECORESCENE_API MeshPrimitive : public Primitive
 
 		IE_CORE_DECLAREEXTENSIONOBJECT( MeshPrimitive, MeshPrimitiveTypeId, Primitive );
 
+		//! @name Define supported interpolations
+		/// \todo : In the future, we hope to use InternedStrings whenever we get/set
+		/// interpolations.
+		/// \todo : The meaning of "linear" has ended up being somewhat misaligned to
+		/// what we actually want. The ideal would probably be if "linear" was instead
+		/// named "none" - indicating that no subdivision is requested, and there was
+		/// a new value "bilinear", which indicated that the limit surface is simple
+		/// polygons, but subdivision is still being requested.
+		/////////////////////////////////////////////////////////////////////////////
+		//@{
+		static const IECore::InternedString interpolationLinear;
+		static const IECore::InternedString interpolationCatmullClark;
+		static const IECore::InternedString interpolationLoop;
+		//@}
+
 		/// Construct a MeshPrimitive with no faces.
 		MeshPrimitive();
 		/// Construct a MeshPrimitive. The number of faces specified by verticesPerFace->readable()->size().
 		/// Copies of the IntVectorData objects are taken rather than references to the initial data.
 		MeshPrimitive( IECore::ConstIntVectorDataPtr verticesPerFace, IECore::ConstIntVectorDataPtr vertexIds,
-			const std::string &interpolation = "linear", IECore::V3fVectorDataPtr p = nullptr );
+			const std::string &interpolation = interpolationLinear.string(), IECore::V3fVectorDataPtr p = nullptr );
+
+		/// Destructor
+		~MeshPrimitive() override;
 
 		//! @name Topology access
 		/// These functions allow access to get and set topology after construction.
@@ -102,6 +121,35 @@ class IECORESCENE_API MeshPrimitive : public Primitive
 		void removeCreases();
 		//@}
 
+		//! @name Subdivision options
+		/// These parameters control various details that affect the shape of the limit surface
+		/////////////////////////////////////////////////////////////////////////////
+		//@{
+
+		const IECore::InternedString &getInterpolateBoundary() const;
+		void setInterpolateBoundary( const IECore::InternedString &interpolateBoundary );
+
+		static const IECore::InternedString interpolateBoundaryNone;
+		static const IECore::InternedString interpolateBoundaryEdgeOnly;
+		static const IECore::InternedString interpolateBoundaryEdgeAndCorner;
+
+		const IECore::InternedString &getFaceVaryingLinearInterpolation() const;
+		void setFaceVaryingLinearInterpolation( const IECore::InternedString &faceVaryingLinearInterpolation );
+
+		static const IECore::InternedString faceVaryingLinearInterpolationNone;
+		static const IECore::InternedString faceVaryingLinearInterpolationCornersOnly;
+		static const IECore::InternedString faceVaryingLinearInterpolationCornersPlus1;
+		static const IECore::InternedString faceVaryingLinearInterpolationCornersPlus2;
+		static const IECore::InternedString faceVaryingLinearInterpolationBoundaries;
+		static const IECore::InternedString faceVaryingLinearInterpolationAll;
+
+		const IECore::InternedString &getTriangleSubdivisionRule() const;
+		void setTriangleSubdivisionRule( const IECore::InternedString &triangleSubdivisionRule );
+
+		static const IECore::InternedString triangleSubdivisionRuleCatmullClark;
+		static const IECore::InternedString triangleSubdivisionRuleSmooth;
+		//@}
+
 		size_t variableSize( PrimitiveVariable::Interpolation interpolation ) const override;
 
 		/// Render the mesh
diff --git a/src/IECorePython/StringAlgoBinding.cpp b/src/IECorePython/StringAlgoBinding.cpp
@@ -73,12 +73,12 @@ list matchPatternPath( const std::string &path, char separator )
 struct VariableProviderWrapper : StringAlgo::VariableProvider, wrapper<StringAlgo::VariableProvider>
 {
 
-	virtual int frame() const
+	int frame() const override
 	{
 		return this->get_override( "frame" )();
 	}
 
-	virtual const std::string &variable( const boost::string_view &name, bool &recurse ) const
+	const std::string &variable( const boost::string_view &name, bool &recurse ) const override
 	{
 		object result = this->get_override( "variable" )( std::string( name ) );
 		extract<tuple> tupleExtractor( result );
diff --git a/src/IECoreScene/MeshPrimitive.cpp b/src/IECoreScene/MeshPrimitive.cpp
diff --git a/src/IECoreScene/bindings/MeshPrimitiveBinding.cpp b/src/IECoreScene/bindings/MeshPrimitiveBinding.cpp
diff --git a/test/IECoreScene/MeshPrimitive.py b/test/IECoreScene/MeshPrimitive.py

Original file line number	Diff line number	Diff line change
`@@ -96,7 +96,7 @@ o.Add(`
`96`	`96`	`o.Add(`
`97`	`97`	`"CXXFLAGS",`
`98`	`98`	`"The extra flags to pass to the C++ compiler during compilation.",`
`99`		`- [ "-pipe", "-Wall", "-Wextra" ] if Environment()["PLATFORM"] != "win32" else [ "/permissive-", "/D_USE_MATH_DEFINES", "/Zc:externC-", "/DBOOST_ALL_NO_LIB" ],`
	`99`	`+ [ "-pipe", "-Wall", "-Wextra", "-Wsuggest-override" ] if Environment()["PLATFORM"] != "win32" else [ "/permissive-", "/D_USE_MATH_DEFINES", "/Zc:externC-", "/DBOOST_ALL_NO_LIB" ],`
`100`	`100`	`)`
`101`	`101`
`102`	`102`	`o.Add(`
Original file line number	Diff line number	Diff line change
`@@ -73,12 +73,12 @@ list matchPatternPath( const std::string &path, char separator )`
`73`	`73`	`struct VariableProviderWrapper : StringAlgo::VariableProvider, wrapper<StringAlgo::VariableProvider>`
`74`	`74`	`{`
`75`	`75`
`76`		`- virtual int frame() const`
	`76`	`+ int frame() const override`
`77`	`77`	`{`
`78`	`78`	`return this->get_override( "frame" )();`
`79`	`79`	`}`
`80`	`80`
`81`		`- virtual const std::string &variable( const boost::string_view &name, bool &recurse ) const`
	`81`	`+ const std::string &variable( const boost::string_view &name, bool &recurse ) const override`
`82`	`82`	`{`
`83`	`83`	`object result = this->get_override( "variable" )( std::string( name ) );`
`84`	`84`	`extract<tuple> tupleExtractor( result );`