Merge pull request #5941 from danieldresser-ie/shuffleMatch

ShufflePlug : Fix specific bug with matching names
GafferHQ · Jul 10, 2024 · 6a074fc · 6a074fc
2 parents 77d2310 + ccae215
commit 6a074fc
Show file tree

Hide file tree

Showing 3 changed files with 44 additions and 59 deletions.
diff --git a/Changes.md b/Changes.md
@@ -20,6 +20,7 @@ Fixes
 - UVInspector : Fixed `Unable to find ScriptNode for UVView` warnings.
 - Scene Editors : Fixed update when ScenePlugs are added to or removed from the node being viewed.
 - PrimitiveInspector : Fixed failure to update when the location being viewed ceases to exist, or is recreated.
+- Shuffle : Fixed some special cases where shuffling a channel to itself would fail to have the expected effect.
 
 API
 ---

diff --git a/include/Gaffer/ShufflePlug.inl b/include/Gaffer/ShufflePlug.inl
@@ -117,6 +117,7 @@ T ShufflesPlug::shuffleInternal( const T &sourceContainer, const T *extraSources
 			const std::string &srcName = srcPattern;
 			const typename T::mapped_type *srcValue = nullptr;
 			typename T::const_iterator sIt = sourceContainer.find( srcName );
+
 			if( sIt != sourceContainer.end() )
 			{
 				srcValue = &sIt->second;
@@ -142,21 +143,19 @@ T ShufflesPlug::shuffleInternal( const T &sourceContainer, const T *extraSources
 				if( ! dstPattern.empty() )
 				{
 					const std::string dstName = scope.context()->substitute( dstPattern );
-					if( srcName != dstName )
+					if( dstReplace || ( destinationContainer.find( dstName ) == destinationContainer.end() ) )
 					{
-						if( dstReplace || ( destinationContainer.find( dstName ) == destinationContainer.end() ) )
-						{
-							destinationContainer[ dstName ] = *srcValue;
-							names.insert( dstName );
-						}
+						destinationContainer[ dstName ] = *srcValue;
+						names.insert( dstName );
+					}
 
-						if( srcDelete && ( names.find( srcName ) == names.end() ) )
-						{
-							destinationContainer.erase( srcName );
-						}
+					if( srcDelete && ( names.find( srcName ) == names.end() ) )
+					{
+						destinationContainer.erase( srcName );
 					}
 				}
-			} else if( !ignoreMissingSource )
+			}
+			else if( !ignoreMissingSource )
 			{
 				throw IECore::Exception( fmt::format( "Source \"{}\" does not exist", srcName ) );
 			}
@@ -184,34 +183,31 @@ T ShufflesPlug::shuffleInternal( const T &sourceContainer, const T *extraSources
 					if( ! dstPattern.empty() )
 					{
 						const std::string dstName = scope.context()->substitute( dstPattern );
-						if( srcName != dstName )
+						// NOTE : Check for clashing move destination names within this shuffle.
+						//        Do check regardless of whether shuffle's replace destination
+						//        flag means destination is not actually written.
+
+						if( ! moveNames.insert( dstName ).second )
+						{
+							throw IECore::Exception(
+								fmt::format(
+									"ShufflesPlug::shuffle : Destination plug \"{}\" shuffles from \"{}\" to \"{}\", " \
+									"cannot write from multiple sources to destination \"{}\"",
+									plug->destinationPlug()->relativeName( plug->node() ? plug->node()->parent() : nullptr ),
+									srcPattern, dstPattern, dstName
+								)
+							);
+						}
+
+						if( dstReplace || ( destinationContainer.find( dstName ) == destinationContainer.end() ) )
 						{
-							// NOTE : Check for clashing move destination names within this shuffle.
-							//        Do check regardless of whether shuffle's replace destination
-							//        flag means destination is not actually written.
-
-							if( ! moveNames.insert( dstName ).second )
-							{
-								throw IECore::Exception(
-									fmt::format(
-										"ShufflesPlug::shuffle : Destination plug \"{}\" shuffles from \"{}\" to \"{}\", " \
-										"cannot write from multiple sources to destination \"{}\"",
-										plug->destinationPlug()->relativeName( plug->node() ? plug->node()->parent() : nullptr ),
-										srcPattern, dstPattern, dstName
-									)
-								);
-							}
-
-							if( dstReplace || ( destinationContainer.find( dstName ) == destinationContainer.end() ) )
-							{
-								destinationContainer[ dstName ] = sIt->second;
-								names.insert( dstName );
-							}
-
-							if( srcDelete && ( names.find( srcName ) == names.end() ) )
-							{
-								destinationContainer.erase( srcName );
-							}
+							destinationContainer[ dstName ] = sIt->second;
+							names.insert( dstName );
+						}
+
+						if( srcDelete && ( names.find( srcName ) == names.end() ) )
+						{
+							destinationContainer.erase( srcName );
 						}
 					}
 				}

diff --git a/python/GafferTest/ShufflePlugTest.py b/python/GafferTest/ShufflePlugTest.py
@@ -205,32 +205,15 @@ def testReuseDestination( self ) :
 			} )
 		)
 
-	def testIgnoreIdentity( self ) :
+	def testIdentityWorksAsUsual( self ) :
 
+		# Unlike in an earlier version, shuffling from "bar" to "bar" works as normal ( in this case,
+		# creating a collision because two sources write to the same destination ).
 		p = Gaffer.ShufflesPlug()
 		p.addChild( Gaffer.ShufflePlug( source = "*", destination = "bar" ) )
 
 		source = IECore.CompoundObject( { "foo" : IECore.FloatData( 0.5 ), "bar" : IECore.FloatData( 1.0 ) } )
-		dest = p.shuffle( source )
-		self.assertEqual(
-			dest,
-			IECore.CompoundObject( {
-				"foo" : IECore.FloatData( 0.5 ),
-				"bar" : IECore.FloatData( 0.5 ),
-			} )
-		)
-
-		# replace destination false
-		p[0]["replaceDestination"].setValue( False )
-		source = IECore.CompoundObject( { "foo" : IECore.FloatData( 0.5 ), "bar" : IECore.FloatData( 1.0 ) } )
-		dest = p.shuffle( source )
-		self.assertEqual(
-			dest,
-			IECore.CompoundObject( {
-				"foo" : IECore.FloatData( 0.5 ),
-				"bar" : IECore.FloatData( 1.0 ),
-			} )
-		)
+		self.assertRaisesRegex( RuntimeError, 'cannot write from multiple sources to destination "bar"', p.shuffle, source )
 
 	def testNoReShuffle( self ) :
 
@@ -526,5 +509,10 @@ def testExtraSources( self ) :
 		self.assertEqual( plug.shuffleWithExtraSources( source, extraSources ), IECore.CompoundData( { "bar" : 20 } ) )
 		self.assertEqual( plug.shuffleWithExtraSources( source, extraSources, ignoreMissingSource = False ), IECore.CompoundData( { "bar" : 20 } ) )
 
+		# When using an extra source of `*`, we should be able to create a new value with a fallback by shuffling
+		# from "toto" to "toto"
+		plug[0]["destination"].setValue( "toto" )
+		self.assertEqual( plug.shuffleWithExtraSources( source, extraSources ), IECore.CompoundData( { "toto" : 20 } ) )
+
 if __name__ == "__main__":
 	unittest.main()