FIX : Address feedback

Changes.md

@@ -9,6 +9,11 @@ Fixes
   - Fixed "colour tearing", where updates to some image channels became visible before updates to others.
   - Fixed unnecessary texture updates when specific image tiles don't change.
 
+Breaking Changes
+----------------
+
+- Instancer : `encapsulate` now produces a single Capsule at the `.../instances` location, instead of the previous `encapsulateInstanceGroups` plug which produced separate capsules at each `.../instances/<prototypeName>` location. Outputting a single capsule is slightly less flexible for users, but is much faster to render in Arnold ( Arnold is currently unable to deal effectively with large numbers of fully overlapping procedurals ). This change also allowed us to simplify the code slightly, reducing the time spent in Gaffer when rendering an encapsulated Instancer by around 20%.
+
 [^1]: To be omitted from 1.5.0.0 release notes.
 
 1.5.0.0a1 (relative to 1.4.13.0)
@@ -138,7 +143,6 @@ Breaking Changes
 - ImageGadget : Remove non-const variant of `getContext()`.
 - LazyMethod : `deferUntilPlaybackStops` now requires that the Widget has a `scriptNode()` method rather than a `context()` method.
 - Python : Gaffer now disables the user site-packages directory by setting `PYTHONNOUSERSITE=1`. To revert to the previous behaviour, set `PYTHONNOUSERSITE=0` before launching Gaffer.
-- Instancer : `encapsulateInstanceGroups` now produces a single Capsule at the `.../instances` location, instead of producing separate capsules at each `.../instances/<prototypeName>` location. Outputting a single capsule is slightly less flexible for users, but is much faster to render in Arnold ( Arnold is currently unable to deal effectively with large numbers of fully overlapping procedurals ). This change also allowed us to simplify the code slightly, reducing the time spent in Gaffer when rendering an encapsulated Instancer by around 20%.
 
 Build
 -----

include/GafferScene/Instancer.h

@@ -144,8 +144,8 @@ class GAFFERSCENE_API Instancer : public BranchCreator
 		Gaffer::StringPlug *attributePrefixPlug();
 		const Gaffer::StringPlug *attributePrefixPlug() const;
 
-		Gaffer::BoolPlug *encapsulateInstanceGroupsPlug();
-		const Gaffer::BoolPlug *encapsulateInstanceGroupsPlug() const;
+		Gaffer::BoolPlug *encapsulatePlug();
+		const Gaffer::BoolPlug *encapsulatePlug() const;
 
 		Gaffer::BoolPlug *seedEnabledPlug();
 		const Gaffer::BoolPlug *seedEnabledPlug() const;
@@ -246,6 +246,7 @@ class GAFFERSCENE_API Instancer : public BranchCreator
 		void engineHash( const ScenePath &sourcePath, const Gaffer::Context *context, IECore::MurmurHash &h ) const;
 
 		ConstEngineSplitPrototypesDataPtr engineSplitPrototypes( const ScenePath &sourcePath, const Gaffer::Context *context ) const;
+		void engineSplitPrototypesHash( const ScenePath &sourcePath, const Gaffer::Context *context, IECore::MurmurHash &h ) const;
 
 		struct PrototypeScope : public Gaffer::Context::EditableScope
 		{

python/GafferSceneTest/InstancerTest.py

@@ -70,7 +70,7 @@ def assertEncapsulatedRendersSame( self, instancer ):
 						continue
 					corresponding.setValue( i.getValue() )
 
-		encapInstancer["encapsulateInstanceGroups"].setValue( True )
+		encapInstancer["encapsulate"].setValue( True )
 
 		self.assertScenesRenderSame( instancer["out"], encapInstancer["out"], expandProcedurals = True, ignoreLinks = True )
 
@@ -171,7 +171,7 @@ def test( self ) :
 		encapInstancer["prototypes"].setInput( instanceInput["out"] )
 		encapInstancer["parent"].setValue( "/seeds" )
 		encapInstancer["name"].setValue( "instances" )
-		encapInstancer["encapsulateInstanceGroups"].setValue( True )
+		encapInstancer["encapsulate"].setValue( True )
 
 		# Test an edge case, and make sure while we're at it that we're actually getting an InstancerCapsule
 		# ( Because it's private, it's bound to Python in a sorta weird way that means its typeName() will
@@ -880,7 +880,7 @@ def testAnimation( self ) :
 			with self.assertRaisesRegex( RuntimeError, 'Instancer.out.transform : Instance id "2" is invalid, instancer produces only 2 children. Topology may have changed during shutter.' ):
 				self.assertScenesRenderSame( instancer["out"], instancer["out"], expandProcedurals = True, ignoreLinks = True )
 
-		instancer["encapsulateInstanceGroups"].setValue( True )
+		instancer["encapsulate"].setValue( True )
 
 		with testContext:
 			with self.assertRaisesRegex( RuntimeError, 'Instance id "2" is invalid, instancer produces only 2 children. Topology may have changed during shutter.' ):
@@ -1432,7 +1432,7 @@ def testSets( self ) :
 		encapInstancer["prototypes"].setInput( instances["out"] )
 		encapInstancer["parent"].setValue( "/object" )
 		encapInstancer["prototypeIndex"].setValue( "index" )
-		encapInstancer["encapsulateInstanceGroups"].setValue( True )
+		encapInstancer["encapsulate"].setValue( True )
 
 		unencapFilter = GafferScene.PathFilter()
 		unencapFilter["paths"].setValue( IECore.StringVectorData( [ "/..." ] ) )
@@ -2594,7 +2594,7 @@ def testContextSet( self ):
 		# When encapsulating, we shouldn't pay any time cost for evaluating the set, even with a huge
 		# number of instances
 		plane["divisions"].setValue( imath.V2i( 1000 ) )
-		instancer["encapsulateInstanceGroups"].setValue( True )
+		instancer["encapsulate"].setValue( True )
 		t = time.time()
 		instancer["out"].set( "set" )
 		totalTime = time.time() - t
@@ -2836,7 +2836,7 @@ def testPrototypePropertiesAffectCapsule( self ) :
 
 			with self.subTest( encapsulate = encapsulate ) :
 
-				instancer["encapsulateInstanceGroups"].setValue( encapsulate )
+				instancer["encapsulate"].setValue( encapsulate )
 
 				# Prototype properties used by the capsule should be reflected
 				# in the object hash. For the hash to be updated successfully,
@@ -3115,7 +3115,7 @@ def testChildNamesPerfWithPrototypesAndIds( self ):
 	@GafferTest.TestRunner.PerformanceTestMethod()
 	def testEncapsulatedRenderPerf( self ):
 		nodes = self.initSimpleInstancer( withPrototypes = True, withIds = False )
-		nodes["instancer"]["encapsulateInstanceGroups"].setValue( True )
+		nodes["instancer"]["encapsulate"].setValue( True )
 
 		renderer = GafferScene.Private.IECoreScenePreview.CapturingRenderer( GafferScene.Private.IECoreScenePreview.Renderer.RenderType.Batch )
 

python/GafferSceneUI/InstancerUI.py

@@ -576,20 +576,22 @@ def __init__( self, headings, toolTipOverride = "" ) :
 
 		],
 
-		"encapsulateInstanceGroups" : [
+		"encapsulate" : [
 
 			"description",
 			"""
-			Converts each group of instances into a capsule, which won't
+			Converts instances into a capsule, which won't
 			be expanded until you Unencapsulate or render. When keeping
 			these locations encapsulated, downstream nodes can't see the
 			instance locations, which prevents editing but improves
 			performance. This option should be preferred to a downstream
 			Encapsulate node because it has the following benefits :
 
 			- Substantially improved performance when the prototypes
-			  define sets.
+			define sets.
 			- Fewer unnecessary updates during interactive rendering.
+			- Faster performance in renderer backends with special
+			instancer capsule support ( ie. Arnold )
 			""",
 			"label", "Instance Groups",
 

src/GafferScene/Instancer.cpp

@@ -342,7 +342,8 @@ class Instancer::EngineData : public Data
 				m_orientations( nullptr ),
 				m_scales( nullptr ),
 				m_uniformScales( nullptr ),
-				m_prototypeContextVariables( prototypeContextVariables )
+				m_prototypeContextVariables( prototypeContextVariables ),
+				m_hasDuplicates( false )
 		{
 			if( !m_primitive )
 			{
@@ -395,7 +396,6 @@ class Instancer::EngineData : public Data
 				}
 			}
 
-			m_hasDuplicates = false;
 			if( m_ids )
 			{
 				for( size_t i = 0, e = numPoints(); i < e; ++i )
@@ -1202,7 +1202,7 @@ Instancer::Instancer( const std::string &name )
 	addChild( new StringPlug( "scale", Plug::In ) );
 	addChild( new StringPlug( "attributes", Plug::In ) );
 	addChild( new StringPlug( "attributePrefix", Plug::In ) );
-	addChild( new BoolPlug( "encapsulateInstanceGroups", Plug::In ) );
+	addChild( new BoolPlug( "encapsulate", Plug::In ) );
 	addChild( new BoolPlug( "seedEnabled", Plug::In ) );
 	addChild( new StringPlug( "seedVariable", Plug::In, "seed" ) );
 	addChild( new IntPlug( "seeds", Plug::In, 10, 1 ) );
@@ -1361,12 +1361,12 @@ const Gaffer::StringPlug *Instancer::attributePrefixPlug() const
 	return getChild<StringPlug>( g_firstPlugIndex + 12 );
 }
 
-Gaffer::BoolPlug *Instancer::encapsulateInstanceGroupsPlug()
+Gaffer::BoolPlug *Instancer::encapsulatePlug()
 {
 	return getChild<BoolPlug>( g_firstPlugIndex + 13 );
 }
 
-const Gaffer::BoolPlug *Instancer::encapsulateInstanceGroupsPlug() const
+const Gaffer::BoolPlug *Instancer::encapsulatePlug() const
 {
 	return getChild<BoolPlug>( g_firstPlugIndex + 13 );
 }
@@ -1532,8 +1532,8 @@ void Instancer::affects( const Plug *input, AffectedPlugsContainer &outputs ) co
 	// For the affects of our output plug, we can mostly rely on BranchCreator's mechanism driven
 	// by affectsBranchObject etc., but for these 3 plugs, we have an overridden hash/compute
 	// which in addition to everything that BranchCreator handles, are also affected by
-	// encapsulateInstanceGroupsPlug()
-	if( input == encapsulateInstanceGroupsPlug() )
+	// encapsulatePlug()
+	if( input == encapsulatePlug() )
 	{
 		outputs.push_back( outPlug()->objectPlug() );
 		outputs.push_back( outPlug()->childNamesPlug() );
@@ -1543,7 +1543,7 @@ void Instancer::affects( const Plug *input, AffectedPlugsContainer &outputs ) co
 	if(
 		input->parent() == prototypesPlug() &&
 		input != prototypesPlug()->globalsPlug() &&
-		!encapsulateInstanceGroupsPlug()->isSetToDefault()
+		!encapsulatePlug()->isSetToDefault()
 	)
 	{
 		outputs.push_back( outPlug()->objectPlug() );
@@ -1605,7 +1605,7 @@ void Instancer::hash( const Gaffer::ValuePlug *output, const Gaffer::Context *co
 		scalePlug()->hash( h );
 		attributesPlug()->hash( h );
 		attributePrefixPlug()->hash( h );
-		encapsulateInstanceGroupsPlug()->hash( h );
+		encapsulatePlug()->hash( h );
 
 		seedEnabledPlug()->hash( h );
 		seedVariablePlug()->hash( h );
@@ -2289,7 +2289,7 @@ IECore::ConstObjectPtr Instancer::computeBranchObject( const ScenePath &sourcePa
 
 void Instancer::hashObject( const ScenePath &path, const Gaffer::Context *context, const ScenePlug *parent, IECore::MurmurHash &h ) const
 {
-	if( parent != capsuleScenePlug() && encapsulateInstanceGroupsPlug()->getValue() )
+	if( parent != capsuleScenePlug() && encapsulatePlug()->getValue() )
 	{
 		// Handling this special case here means an extra call to sourceAndBranchPaths
 		// when we're encapsulating and we're not inside a branch - this is a small
@@ -2322,7 +2322,7 @@ void Instancer::hashObject( const ScenePath &path, const Gaffer::Context *contex
 
 IECore::ConstObjectPtr Instancer::computeObject( const ScenePath &path, const Gaffer::Context *context, const ScenePlug *parent ) const
 {
-	if( parent != capsuleScenePlug() && encapsulateInstanceGroupsPlug()->getValue() )
+	if( parent != capsuleScenePlug() && encapsulatePlug()->getValue() )
 	{
 		ScenePath sourcePath, branchPath;
 		parentAndBranchPaths( path, sourcePath, branchPath );
@@ -2368,7 +2368,7 @@ void Instancer::hashBranchChildNames( const ScenePath &sourcePath, const ScenePa
 	{
 		// "/instances/<prototypeName>"
 		BranchCreator::hashBranchChildNames( sourcePath, branchPath, context, h );
-		engineHash( sourcePath, context, h );
+		engineSplitPrototypesHash( sourcePath, context, h );
 		h.append( branchPath.back() );
 	}
 	else
@@ -2444,7 +2444,7 @@ IECore::ConstInternedStringVectorDataPtr Instancer::computeBranchChildNames( con
 
 void Instancer::hashChildNames( const ScenePath &path, const Gaffer::Context *context, const ScenePlug *parent, IECore::MurmurHash &h ) const
 {
-	if( parent != capsuleScenePlug() && encapsulateInstanceGroupsPlug()->getValue() )
+	if( parent != capsuleScenePlug() && encapsulatePlug()->getValue() )
 	{
 		ScenePath sourcePath, branchPath;
 		parentAndBranchPaths( path, sourcePath, branchPath );
@@ -2460,7 +2460,7 @@ void Instancer::hashChildNames( const ScenePath &path, const Gaffer::Context *co
 
 IECore::ConstInternedStringVectorDataPtr Instancer::computeChildNames( const ScenePath &path, const Gaffer::Context *context, const ScenePlug *parent ) const
 {
-	if( parent != capsuleScenePlug() && encapsulateInstanceGroupsPlug()->getValue() )
+	if( parent != capsuleScenePlug() && encapsulatePlug()->getValue() )
 	{
 		ScenePath sourcePath, branchPath;
 		parentAndBranchPaths( path, sourcePath, branchPath );
@@ -2584,7 +2584,7 @@ IECore::ConstPathMatcherDataPtr Instancer::computeBranchSet( const ScenePath &so
 
 void Instancer::hashSet( const IECore::InternedString &setName, const Gaffer::Context *context, const ScenePlug *parent, IECore::MurmurHash &h ) const
 {
-	if( parent != capsuleScenePlug() && encapsulateInstanceGroupsPlug()->getValue() )
+	if( parent != capsuleScenePlug() && encapsulatePlug()->getValue() )
 	{
 		h = inPlug()->setPlug()->hash();
 		return;
@@ -2595,7 +2595,7 @@ void Instancer::hashSet( const IECore::InternedString &setName, const Gaffer::Co
 
 IECore::ConstPathMatcherDataPtr Instancer::computeSet( const IECore::InternedString &setName, const Gaffer::Context *context, const ScenePlug *parent ) const
 {
-	if( parent != capsuleScenePlug() && encapsulateInstanceGroupsPlug()->getValue() )
+	if( parent != capsuleScenePlug() && encapsulatePlug()->getValue() )
 	{
 		return inPlug()->setPlug()->getValue();
 	}
@@ -2621,6 +2621,12 @@ Instancer::ConstEngineSplitPrototypesDataPtr Instancer::engineSplitPrototypes( c
 	return boost::static_pointer_cast<const EngineSplitPrototypesData>( engineSplitPrototypesPlug()->getValue() );
 }
 
+void Instancer::engineSplitPrototypesHash( const ScenePath &sourcePath, const Gaffer::Context *context, IECore::MurmurHash &h ) const
+{
+	ScenePlug::PathScope scope( context, &sourcePath );
+	engineSplitPrototypesPlug()->hash( h );
+}
+
 const std::type_info &Instancer::instancerCapsuleTypeInfo()
 {
 	return typeid( InstancerCapsule );
@@ -2918,9 +2924,9 @@ void Instancer::InstancerCapsule::render( IECoreScenePreview::Renderer *renderer
 			std::vector< std::string > names( engines[0]->numValidPrototypes() );
 			std::vector< int > namePrefixLengths( engines[0]->numValidPrototypes() );
 
-			for( size_t i = r.begin(); i != r.end(); ++i )
+			for( size_t pointIndex = r.begin(); pointIndex != r.end(); ++pointIndex )
 			{
-				int protoIndex = engines[0]->prototypeIndex( i );
+				int protoIndex = engines[0]->prototypeIndex( pointIndex );
 				if( protoIndex == -1 )
 				{
 					// Invalid prototype
@@ -2948,7 +2954,7 @@ void Instancer::InstancerCapsule::render( IECoreScenePreview::Renderer *renderer
 					// general instead of assuming that frame is the only variable for which offsetMode may be set? )
 					prototypeScope.setFrame( onFrameTime );
 
-					engines[0]->setPrototypeContextVariables( i, prototypeScope );
+					engines[0]->setPrototypeContextVariables( pointIndex, prototypeScope );
 
 					proto = prototypeCache.get( PrototypeCacheGetterKey( protoIndex, prototypeScope.context() ) ).get();
 				}
@@ -2971,7 +2977,7 @@ void Instancer::InstancerCapsule::render( IECoreScenePreview::Renderer *renderer
 					// careful not to modify them though!
 					currentAttributes->members() = proto->m_attributes->members();
 
-					engines[0]->instanceAttributes( i, *currentAttributes );
+					engines[0]->instanceAttributes( pointIndex, *currentAttributes );
 					attribsStorage = renderer->attributes( currentAttributes.get() );
 					attribs = attribsStorage.get();
 				}
@@ -2980,7 +2986,7 @@ void Instancer::InstancerCapsule::render( IECoreScenePreview::Renderer *renderer
 					attribs = proto->m_rendererAttributes.get();
 				}
 
-				int instanceId = engines[0]->instanceId( i );
+				int instanceId = engines[0]->instanceId( pointIndex );
 
 
 				if( !namePrefixLengths[protoIndex] )
@@ -3020,14 +3026,14 @@ void Instancer::InstancerCapsule::render( IECoreScenePreview::Renderer *renderer
 
 				if( sampleTimes.size() == 1 )
 				{
-					objectInterface->transform( proto->m_transforms[0] * engines[0]->instanceTransform( i ) );
+					objectInterface->transform( proto->m_transforms[0] * engines[0]->instanceTransform( pointIndex ) );
 				}
 				else
 				{
-					for( unsigned int j = 0; j < engines.size(); j++ )
+					for( unsigned int i = 0; i < engines.size(); i++ )
 					{
-						int curPointIndex = j == 0 ? i : engines[j]->pointIndex( instanceId );
-						pointTransforms[j] = proto->m_transforms[j] * engines[j]->instanceTransform( curPointIndex );
+						int curPointIndex = i == 0 ? pointIndex : engines[i]->pointIndex( instanceId );
+						pointTransforms[i] = proto->m_transforms[i] * engines[i]->instanceTransform( curPointIndex );
 					}
 
 					objectInterface->transform( pointTransforms, sampleTimes );