Merge pull request #1441 from danieldresser-ie/splitOrdered

MeshAlgo::MeshSplitter : Preserve vertex order

Changes

@@ -8,6 +8,11 @@ Improvements
 - IECoreHoudini : Updated to support Houdini 20.0 and 20.5.
 - IECoreMaya : Avoid compilation warnings with new gcc.
 
+Fixes
+-----
+
+- MeshAlgo::MeshSplitter/segment : Fixed so that we now preserve vertex order while splitting. This matches the old behvaviour before 1.4.6.0 when segment was optimized. This doesn't affect the correctness of the result, but is a better convention to match user expectations - when combining meshes followed by splitting, it's better if you get back the same vertex ids you started with.
+
 Build
 -----
 

src/IECoreScene/MeshAlgoSplit.cpp

@@ -490,7 +490,8 @@ class Reindexer
 		m_newIndices( m_newIndicesData->writable() ),
 		m_blockSize( blockSize ),
 		m_fromOldIds( ( numOriginalIds - 1 ) / blockSize + 1 ),
-		m_numIdsUsed( 0 )
+		m_numIdsUsed( 0 ),
+		m_indicesComputed( false )
 	{
 		m_newIndices.reserve( numIndices );
 	}
@@ -510,21 +511,21 @@ class Reindexer
 			block = std::make_unique< std::vector<int> >( m_blockSize, -1 );
 		}
 
-		int &r = (*block)[ subIndex ];
-		if( r == -1 )
-		{
-			// Id isn't used yet, we need to set this location in the block, and use it
-			r = m_numIdsUsed;
-			m_numIdsUsed++;
-		}
+		// We initially record that this index is used just by marking it with a 0, against the background of -1.
+		// Once computeIndices is called, the 0 will be replaced with a new index, only counting indices that are
+		// used.
+		(*block)[ subIndex ] = 0;
 
-		m_newIndices.push_back( r );
+		m_newIndices.push_back( id );
+
+		m_indicesComputed = false;
 	}
 
 	// Don't add the index, but just test if it is a part of the reindex. If it is an
 	// id which has already been added, return the new id, otherwise return -1
 	inline int testIndex( int id )
 	{
+		computeIndices();
 		int blockId = id / m_blockSize;
 		int subIndex = id % m_blockSize;
 		auto &block = m_fromOldIds[ blockId ];
@@ -541,6 +542,7 @@ class Reindexer
 	// Get the new indices. Call after calling addIndex for every original index
 	IntVectorDataPtr getNewIndices()
 	{
+		computeIndices();
 		return m_newIndicesData;
 	}
 
@@ -550,6 +552,7 @@ class Reindexer
 	template <typename T >
 	void remapData( const std::vector<T> &in, std::vector<T> &out )
 	{
+		computeIndices();
 		out.resize( m_numIdsUsed );
 		for( unsigned int i = 0; i < m_fromOldIds.size(); i++ )
 		{
@@ -574,6 +577,7 @@ class Reindexer
 	// original id corresponding to each id of the output
 	void getDataRemapping( std::vector<int> &dataRemap )
 	{
+		computeIndices();
 		dataRemap.resize( m_numIdsUsed );
 		for( unsigned int i = 0; i < m_fromOldIds.size(); i++ )
 		{
@@ -595,6 +599,45 @@ class Reindexer
 	}
 
 private:
+
+	void computeIndices()
+	{
+		// Once indices have been added, and before using them, this function is called to
+		// compute the new indices.
+		if( m_indicesComputed )
+		{
+			return;
+		}
+
+		m_indicesComputed = true;
+
+		for( unsigned int blockId = 0; blockId < m_fromOldIds.size(); blockId++ )
+		{
+			auto &block = m_fromOldIds[ blockId ];
+			if( !block )
+			{
+				continue;
+			}
+
+			for( int i = 0; i < m_blockSize; i++ )
+			{
+				if( (*block)[i] != -1 )
+				{
+					(*block)[i] = m_numIdsUsed;
+					m_numIdsUsed++;
+				}
+			}
+		}
+
+		for( int &id : m_newIndices )
+		{
+			int blockId = id / m_blockSize;
+			int subIndex = id % m_blockSize;
+
+			id = (*m_fromOldIds[ blockId ])[subIndex];
+		}
+	}
+
 	// IntVectorData to hold the new indices
 	IntVectorDataPtr m_newIndicesData;
 	std::vector< int > &m_newIndices;
@@ -605,13 +648,16 @@ class Reindexer
 	// Store the mapping from old ids to new ids. The outer vector holds a unique_ptr for each
 	// block of m_blockSize ids in the original id range. These pointers are null if no ids from
 	// that block have been used. Once a block is used, it is allocated with a vector that is set
-	// to -1 for ids which have not been used, and the new corresponding id for ids which have been
-	// used
+	// to -1 for ids which have not been used, and zeros for ids which have been used.  When computeIndices()
+	// is called, all used elements get a new id assigned, relative to just the used ids.
 	std::vector< std::unique_ptr< std::vector< int > > > m_fromOldIds;
 
 	// How many unique ids have appeared in the indices added so far
 	int m_numIdsUsed;
 
+	// Whether we have yet computed the new indices for each used index
+	bool m_indicesComputed;
+
 };
 
 struct ResamplePrimitiveVariableFunctor

test/IECoreScene/MeshAlgoSegmentTest.py

@@ -66,8 +66,8 @@ def testCanSegmentUsingIntegerPrimvar( self ) :
 		p12 = imath.V3f( 1, 2, 0 )
 		p22 = imath.V3f( 2, 2, 0 )
 
-		self.assertEqual( segments[0]["P"].data, IECore.V3fVectorData( [p00, p10, p11, p01, p20, p21], IECore.GeometricData.Interpretation.Point ) )
-		self.assertEqual( segments[1]["P"].data, IECore.V3fVectorData( [p01, p11, p12, p02, p21, p22], IECore.GeometricData.Interpretation.Point ) )
+		self.assertEqual( segments[0]["P"].data, IECore.V3fVectorData( [p00, p10, p20, p01, p11, p21], IECore.GeometricData.Interpretation.Point ) )
+		self.assertEqual( segments[1]["P"].data, IECore.V3fVectorData( [p01, p11, p21, p02, p12, p22], IECore.GeometricData.Interpretation.Point ) )
 
 	def testCanSegmentUsingStringPrimvar( self ) :
 		mesh = IECoreScene.MeshPrimitive.createPlane( imath.Box2f( imath.V2f( 0 ), imath.V2f( 2 ) ), imath.V2i( 2 ) )
@@ -95,8 +95,8 @@ def testCanSegmentUsingStringPrimvar( self ) :
 		p12 = imath.V3f( 1, 2, 0 )
 		p22 = imath.V3f( 2, 2, 0 )
 
-		self.assertEqual( segments[0]["P"].data, IECore.V3fVectorData( [p00, p10, p11, p01, p21, p22, p12], IECore.GeometricData.Interpretation.Point ) )
-		self.assertEqual( segments[1]["P"].data, IECore.V3fVectorData( [p10, p20, p21, p11, p01, p12, p02], IECore.GeometricData.Interpretation.Point ) )
+		self.assertEqual( segments[0]["P"].data, IECore.V3fVectorData( [p00, p10, p01, p11, p21, p12, p22], IECore.GeometricData.Interpretation.Point ) )
+		self.assertEqual( segments[1]["P"].data, IECore.V3fVectorData( [p10, p20, p01, p11, p21, p02, p12], IECore.GeometricData.Interpretation.Point ) )
 
 	def testSegmentsFullyIfNoSegmentValuesGiven( self ) :
 		mesh = IECoreScene.MeshPrimitive.createPlane( imath.Box2f( imath.V2f( 0 ), imath.V2f( 2 ) ), imath.V2i( 2 ) )
@@ -130,8 +130,8 @@ def testSegmentsFullyIfNoSegmentValuesGiven( self ) :
 			s0 = segments[1]
 			s1 = segments[0]
 
-		self.assertEqual( s0["P"].data, IECore.V3fVectorData( [p00, p10, p11, p01, p21, p22, p12], IECore.GeometricData.Interpretation.Point ) )
-		self.assertEqual( s1["P"].data, IECore.V3fVectorData( [p10, p20, p21, p11, p01, p12, p02], IECore.GeometricData.Interpretation.Point ) )
+		self.assertEqual( s0["P"].data, IECore.V3fVectorData( [p00, p10, p01, p11, p21, p12, p22], IECore.GeometricData.Interpretation.Point ) )
+		self.assertEqual( s1["P"].data, IECore.V3fVectorData( [p10, p20, p01, p11, p21, p02, p12], IECore.GeometricData.Interpretation.Point ) )
 
 
 	def testRaisesExceptionIfSegmentKeysNotSameTypeAsPrimvar( self ) :
@@ -184,7 +184,7 @@ def testSegmentSubset( self ) :
 		p22 = imath.V3f( 2, 2, 0 )
 
 		self.assertEqual( len( segments ), 1 )
-		self.assertEqual( segments[0]["P"].data, IECore.V3fVectorData( [p11, p21, p22, p12], IECore.GeometricData.Interpretation.Point ) )
+		self.assertEqual( segments[0]["P"].data, IECore.V3fVectorData( [p11, p21, p12, p22], IECore.GeometricData.Interpretation.Point ) )
 		self.assertEqual( segments[0]["s"].data, IECore.StringVectorData( ["b"] ) )
 
 

test/IECoreScene/MeshAlgoSplitTest.py

@@ -170,16 +170,29 @@ def accumulateInPython2( iterable ):
 			newVerticesPerFace = []
 			newVertIndices = []
 			mapFaceVert = []
-			reverseIndices = []
+
+			usedIndices = set()
 			for i in r:
 				vpf = mesh.verticesPerFace[i]
 				newVerticesPerFace.append( vpf )
 				for j in range( vpf ):
 					origFaceVert = faceIndices[i] + j
 					origVert = mesh.vertexIds[ origFaceVert ]
-					newIndex = reindex.setdefault( origVert, len( reindex ) )
-					if len( reindex ) > len( reverseIndices ):
-						reverseIndices.append( origVert )
+
+					usedIndices.add( origVert )
+
+			usedIndices = sorted( usedIndices )
+
+			for i in range( len( usedIndices ) ):
+				reindex[ usedIndices[i] ] = i
+
+			for i in r:
+				vpf = mesh.verticesPerFace[i]
+				for j in range( vpf ):
+					origFaceVert = faceIndices[i] + j
+					origVert = mesh.vertexIds[ origFaceVert ]
+
+					newIndex = usedIndices.index( origVert )
 					newVertIndices.append( newIndex )
 					mapFaceVert.append( origFaceVert )
 
@@ -226,7 +239,7 @@ def accumulateInPython2( iterable ):
 				if p.interpolation == interp.Constant:
 					d = p.data.copy()
 				elif p.interpolation in [ interp.Vertex, interp.Varying ]:
-					d = type( p.data )( [ pd[i] for i in reverseIndices] )
+					d = type( p.data )( [ pd[i] for i in usedIndices] )
 				elif p.interpolation == interp.FaceVarying:
 					d = type( p.data )( [ pd[i] for i in mapFaceVert ] )
 				elif p.interpolation == interp.Uniform:
@@ -269,8 +282,8 @@ def testCanSplitUsingIntegerPrimvar( self ) :
 		p12 = imath.V3f( 1, 2, 0 )
 		p22 = imath.V3f( 2, 2, 0 )
 
-		self.assertEqual( s0["P"].data, IECore.V3fVectorData( [p00, p10, p11, p01, p20, p21], IECore.GeometricData.Interpretation.Point ) )
-		self.assertEqual( s1["P"].data, IECore.V3fVectorData( [p01, p11, p12, p02, p21, p22], IECore.GeometricData.Interpretation.Point ) )
+		self.assertEqual( s0["P"].data, IECore.V3fVectorData( [p00, p10, p20, p01, p11, p21], IECore.GeometricData.Interpretation.Point ) )
+		self.assertEqual( s1["P"].data, IECore.V3fVectorData( [p01, p11, p21, p02, p12, p22], IECore.GeometricData.Interpretation.Point ) )
 
 	def testSplitsFully( self ) :
 		mesh = IECoreScene.MeshPrimitive.createPlane( imath.Box2f( imath.V2f( 0 ), imath.V2f( 2 ) ), imath.V2i( 2 ) )
@@ -303,8 +316,8 @@ def testSplitsFully( self ) :
 		if s0["s"].data[0] != 'a':
 			s0,s1 = s1,s0
 
-		self.assertEqual( s0["P"].data, IECore.V3fVectorData( [p00, p10, p11, p01, p21, p22, p12], IECore.GeometricData.Interpretation.Point ) )
-		self.assertEqual( s1["P"].data, IECore.V3fVectorData( [p10, p20, p21, p11, p01, p12, p02], IECore.GeometricData.Interpretation.Point ) )
+		self.assertEqual( s0["P"].data, IECore.V3fVectorData( [p00, p10, p01, p11, p21, p12, p22], IECore.GeometricData.Interpretation.Point ) )
+		self.assertEqual( s1["P"].data, IECore.V3fVectorData( [p10, p20, p01, p11, p21, p02, p12], IECore.GeometricData.Interpretation.Point ) )
 
 	def testSplitUsingIndexedPrimitiveVariable( self ) :