FIX : Instancer : Fully support 64 bit ids

Changes.md

@@ -7,6 +7,11 @@ Improvements
 - Light Editor : Added `is_sphere` column for Cycles lights.
 - Windows : Gaffer now uses the TBB memory allocator for significantly better performance.
 
+Fixes
+-----
+
+- Instancer : Added support 64 bit ints for ids ( matching what is loaded from USD ).
+
 
 1.5.0.0a3 (relative to 1.5.0.0a2)
 =========
@@ -44,7 +49,6 @@ Fixes
 - Cycles : Fixed issue where scaling unnormalized quad and disk lights would not affect their brightness.
 - SceneReader : Fixed crash reading facevarying normals skinned with UsdSkel. [^1]
 - ShaderView : Fixed crash caused by a SceneCreator returning `None`. [^1]
-- Instancer : Support 64 bit ints for ids ( matching what is loaded from USD ).
 
 Breaking Changes
 ----------------

python/GafferSceneTest/InstancerTest.py

@@ -1566,16 +1566,16 @@ def runTestIds( self, useInt64 ) :
 
 		self.assertEncapsulatedRendersSame( instancer )
 
-	def testNegativeIdsAndIndices( self ) :
+	def testExtremeIdsAndIndices( self ) :
 
-		points = IECoreScene.PointsPrimitive( IECore.V3fVectorData( [ imath.V3f( x, 0, 0 ) for x in range( 0, 2 ) ] ) )
+		points = IECoreScene.PointsPrimitive( IECore.V3fVectorData( [ imath.V3f( x, 0, 0 ) for x in range( 0, 4 ) ] ) )
 		points["id"] = IECoreScene.PrimitiveVariable(
 			IECoreScene.PrimitiveVariable.Interpolation.Vertex,
-			IECore.IntVectorData( [ -10, -5 ] ),
+			IECore.Int64VectorData( [ -10, -5, 8000000000, 8000000001 ] ),
 		)
 		points["index"] = IECoreScene.PrimitiveVariable(
 			IECoreScene.PrimitiveVariable.Interpolation.Vertex,
-			IECore.IntVectorData( [ -1, -2 ] ),
+			IECore.IntVectorData( [ -1, -2, 1, 0 ] ),
 		)
 
 		objectToScene = GafferScene.ObjectToScene()
@@ -1588,24 +1588,57 @@ def testNegativeIdsAndIndices( self ) :
 		instances["children"][0].setInput( cube["out"] )
 		instances["parent"].setValue( "/" )
 
+		allFilter = GafferScene.PathFilter()
+		allFilter["paths"].setValue( IECore.StringVectorData( [ '/*' ] ) )
+
+		customAttributes = GafferScene.CustomAttributes()
+		customAttributes["in"].setInput( instances["out"] )
+		customAttributes["filter"].setInput( allFilter["out"] )
+		customAttributes["attributes"].addChild( Gaffer.NameValuePlug( "intAttr", Gaffer.IntPlug( "value", flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ), True, "member1" ) )
+
+		customAttributes["ReadContextExpression"] = Gaffer.Expression()
+		customAttributes["ReadContextExpression"].setExpression(
+			'parent["attributes"]["member1"]["value"] = context.get( "seed", -1 )'
+		)
+
 		instancer = GafferScene.Instancer()
 		instancer["in"].setInput( objectToScene["out"] )
-		instancer["prototypes"].setInput( instances["out"] )
-		instancer["parent"].setValue( "/object" )
+		instancer["prototypes"].setInput( customAttributes["out"] )
+		instancer["filter"].setInput( allFilter["out"] )
 		instancer["prototypeIndex"].setValue( "index" )
 		instancer["id"].setValue( "id" )
+		instancer["seedEnabled"].setValue( True )
+		instancer["rawSeed"].setValue( True )
 
 		self.assertEqual( instancer["out"].childNames( "/object/instances" ), IECore.InternedStringVectorData( [ "sphere", "cube" ] ) )
-		self.assertEqual( instancer["out"].childNames( "/object/instances/sphere" ), IECore.InternedStringVectorData( [ "-5" ] ) )
-		self.assertEqual( instancer["out"].childNames( "/object/instances/cube" ), IECore.InternedStringVectorData( [ "-10" ] ) )
+		self.assertEqual( instancer["out"].childNames( "/object/instances/sphere" ), IECore.InternedStringVectorData( [ "-5", "8000000001" ] ) )
+		self.assertEqual( instancer["out"].childNames( "/object/instances/cube" ), IECore.InternedStringVectorData( [ "-10", "8000000000" ] ) )
 		self.assertEqual( instancer["out"].childNames( "/object/instances/sphere/-5" ), IECore.InternedStringVectorData() )
 		self.assertEqual( instancer["out"].childNames( "/object/instances/cube/-10" ), IECore.InternedStringVectorData() )
+		self.assertEqual( instancer["out"].childNames( "/object/instances/sphere/8000000001" ), IECore.InternedStringVectorData() )
+		self.assertEqual( instancer["out"].childNames( "/object/instances/cube/8000000000" ), IECore.InternedStringVectorData() )
 
 		self.assertEqual( instancer["out"].object( "/object/instances" ), IECore.NullObject.defaultNullObject() )
 		self.assertEqual( instancer["out"].object( "/object/instances/sphere" ), IECore.NullObject.defaultNullObject() )
 		self.assertEqual( instancer["out"].object( "/object/instances/cube" ), IECore.NullObject.defaultNullObject() )
 		self.assertEqual( instancer["out"].object( "/object/instances/sphere/-5" ), sphere["out"].object( "/sphere" ) )
 		self.assertEqual( instancer["out"].object( "/object/instances/cube/-10" ), cube["out"].object( "/cube" ) )
+		self.assertEqual( instancer["out"].object( "/object/instances/sphere/8000000001" ), sphere["out"].object( "/sphere" ) )
+		self.assertEqual( instancer["out"].object( "/object/instances/cube/8000000000" ), cube["out"].object( "/cube" ) )
+
+		self.assertEqual( instancer["out"].attributes( "/object/instances/sphere/-5" )["intAttr"].value, -5 )
+		self.assertEqual( instancer["out"].attributes( "/object/instances/cube/-10" )["intAttr"].value, -10 )
+
+		# We want to fully support int64 typed ids, but for reasons of backwards compatiblity and OSL support,
+		# we're still using int32 for the seed context variable, so these ids get wrapped around even in raw seeds
+		# mode.
+		self.assertEqual( instancer["out"].attributes( "/object/instances/sphere/8000000001" )["intAttr"].value, -589934591 )
+		self.assertEqual( instancer["out"].attributes( "/object/instances/cube/8000000000" )["intAttr"].value, -589934592 )
+
+		self.assertEqual(
+			instancer["variations"].getValue(),
+			IECore.CompoundData( { 'seed' : IECore.IntData( 4 ), '' : IECore.IntData( 4 ) } )
+		)
 
 		self.assertSceneValid( instancer["out"] )
 

src/GafferScene/Instancer.cpp

@@ -303,7 +303,7 @@ struct IdData
 		}
 	}
 
-	int element( int i ) const
+	int64_t element( int i ) const
 	{
 		if( intElements )
 		{
@@ -325,10 +325,10 @@ struct IdData
 // with a grouping pattern that can be changed with seedScramble
 int seedForPoint( int index, const IdData &idData, int numSeeds, int seedScramble )
 {
-	int id = index;
+	int64_t id = index;
 	if( idData.size() )
 	{
-		id = idData.element( id );
+		id = idData.element( index );
 	}
 
 	// numSeeds is set to 0 when we're just passing through the id
@@ -348,7 +348,19 @@ int seedForPoint( int index, const IdData &idData, int numSeeds, int seedScrambl
 
 		IECore::MurmurHash seedHash;
 		seedHash.append( seedScramble );
-		seedHash.append( id );
+
+		if( id <= INT32_MAX && id >= INT_MIN )
+		{
+			// This branch shouldn't be needed, we'd like to just treat ids as 64 bit now ...
+			// but if we just took the branch below, that would changing the seeding of existing
+			// scenes.
+			seedHash.append( (int)id );
+		}
+		else
+		{
+			seedHash.append( id );
+		}
+
 		id = int( ( double( seedHash.h1() ) / double( UINT64_MAX ) ) * double( numSeeds ) );
 		id = id % numSeeds;  // For the rare case h1 / max == 1.0, make sure we stay in range
 	}
@@ -448,7 +460,7 @@ class Instancer::EngineData : public Data
 			{
 				for( size_t i = 0, e = numPoints(); i < e; ++i )
 				{
-					int id = m_ids.element(i);
+					int64_t id = m_ids.element(i);
 					auto ins = m_idsToPointIndices.try_emplace( id, i );
 					if( !ins.second )
 					{
@@ -489,16 +501,16 @@ class Instancer::EngineData : public Data
 			return m_primitive ? m_primitive->variableSize( PrimitiveVariable::Vertex ) : 0;
 		}
 
-		size_t instanceId( size_t pointIndex ) const
+		int64_t instanceId( size_t pointIndex ) const
 		{
 			return m_ids.size() ? m_ids.element( pointIndex ) : pointIndex;
 		}
 
-		size_t pointIndex( size_t i ) const
+		size_t pointIndex( int64_t i ) const
 		{
 			if( !m_ids.size() )
 			{
-				if( i >= numPoints() )
+				if( i >= (int64_t)numPoints() || i < 0 )
 				{
 					throw IECore::Exception( fmt::format( "Instance id \"{}\" is invalid, instancer produces only {} children. Topology may have changed during shutter.", i, numPoints() ) );
 				}
@@ -536,7 +548,7 @@ class Instancer::EngineData : public Data
 				// If there are duplicates in the id list, then some point indices will be omitted - we
 				// need to check each point index to see if it got assigned an id correctly
 
-				int id = m_ids.element( pointIndex );
+				int64_t id = m_ids.element( pointIndex );
 
 				if( m_idsToPointIndices.at(id) != pointIndex )
 				{
@@ -969,7 +981,7 @@ class Instancer::EngineData : public Data
 		const std::vector<Imath::V3f> *m_scales;
 		const std::vector<float> *m_uniformScales;
 
-		using IdsToPointIndices = std::unordered_map <int, size_t>;
+		using IdsToPointIndices = std::unordered_map <int64_t, size_t>;
 		IdsToPointIndices m_idsToPointIndices;
 
 		boost::container::flat_map<InternedString, AttributeCreator> m_attributeCreators;
@@ -1743,7 +1755,7 @@ void Instancer::hash( const Gaffer::ValuePlug *output, const Gaffer::Context *co
 					for( size_t i = r.begin(); i != r.end(); ++i )
 					{
 						const size_t pointIndex = pointIndicesForPrototype[i];
-						size_t instanceId = engine->instanceId( pointIndex );
+						int64_t instanceId = engine->instanceId( pointIndex );
 						engine->setPrototypeContextVariables( pointIndex, scope );
 						IECore::MurmurHash instanceH;
 						instanceH.append( instanceId );
@@ -1993,7 +2005,7 @@ void Instancer::compute( Gaffer::ValuePlug *output, const Gaffer::Context *conte
 					for( size_t i = r.begin(); i != r.end(); ++i )
 					{
 						const size_t pointIndex = pointIndicesForPrototype[i];
-						size_t instanceId = engine->instanceId( pointIndex );
+						int64_t instanceId = engine->instanceId( pointIndex );
 						engine->setPrototypeContextVariables( pointIndex, scope );
 						ConstPathMatcherDataPtr instanceSet = prototypesPlug()->setPlug()->getValue();
 						PathMatcher pointInstanceSet = instanceSet->readable().subTree( *prototypeRoot );
@@ -2456,7 +2468,7 @@ IECore::ConstInternedStringVectorDataPtr Instancer::computeBranchChildNames( con
 		// the ids, not the point indices, and must be sorted. So we need to allocate a
 		// temp buffer of integer ids, before converting to strings.
 
-		std::vector<int> ids;
+		std::vector<int64_t> ids;
 		ids.reserve( pointIndicesForPrototype.size() );
 
 		const EngineData *engineData = esp->engine();
@@ -2472,7 +2484,7 @@ IECore::ConstInternedStringVectorDataPtr Instancer::computeBranchChildNames( con
 		InternedStringVectorDataPtr childNamesData = new InternedStringVectorData;
 		std::vector<InternedString> &childNames = childNamesData->writable();
 		childNames.reserve( ids.size() );
-		for( size_t id : ids )
+		for( int64_t id : ids )
 		{
 			childNames.emplace_back( id );
 		}
@@ -3031,7 +3043,7 @@ void Instancer::InstancerCapsule::render( IECoreScenePreview::Renderer *renderer
 					attribs = proto->m_rendererAttributes.get();
 				}
 
-				int instanceId = engines[0]->instanceId( pointIndex );
+				int64_t instanceId = engines[0]->instanceId( pointIndex );
 
 
 				if( !namePrefixLengths[protoIndex] )
@@ -3052,7 +3064,7 @@ void Instancer::InstancerCapsule::render( IECoreScenePreview::Renderer *renderer
 				// up being named when they use the non-encapsulated hierarchy.
 				std::string &name = names[ protoIndex ];
 				const int prefixLen = namePrefixLengths[ protoIndex ];
-				name.resize( namePrefixLengths[protoIndex] + std::numeric_limits< int >::digits10 + 1 );
+				name.resize( namePrefixLengths[protoIndex] + std::numeric_limits< int64_t >::digits10 + 1 );
 				name.resize( std::to_chars( &name[prefixLen], &(*name.end()), instanceId ).ptr - &name[0] );
 
 				IECoreScenePreview::Renderer::ObjectInterfacePtr objectInterface;