Merge pull request #5898 from GafferHQ/ocioContextFix

OpenColorIO : Defer expansion of context variables in OCIO vars

Changes.md

@@ -15,6 +15,12 @@ Improvements
   - Added support for dropping an Edit Scope node onto the widget to set it as the current Edit Scope.
   - Added support for middle-dragging from the widget to access the current Edit Scope node.
 - ArnoldAttributes : Added syntax highlighting and auto-complete for set expressions on the `shadowGroup` plug.
+- OpenColorIO : When a script-level OpenColorIO variable contains a Gaffer `${contextVariable}` reference, its evaluation is now deferred to the point of use. This allows it to pick up overrides introduced by nodes such as ContextVariables and Wedge.
+
+Fixes
+-----
+
+- Viewer : Fixed handling of Gaffer `${contextVariable}` references in OpenColorIO variable values. The Viewer now updates the Display Transform appropriately when the value of the context variable changes.
 
 API
 ---

python/GafferImageTest/ColorSpaceTest.py

@@ -247,7 +247,7 @@ def testConfigFromGafferContext( self ) :
 
 		with Gaffer.Context() as c :
 
-			GafferImage.OpenColorIOAlgo.setConfig( c, str( self.openColorIOPath() / "context.ocio" ) )
+			GafferImage.OpenColorIOAlgo.setConfig( c, ( self.openColorIOPath() / "context.ocio" ).as_posix() )
 			GafferImage.OpenColorIOAlgo.addVariable( c, "LUT", "srgb.spi1d" )
 			GafferImage.OpenColorIOAlgo.addVariable( c, "CDL", "cineon.spi1d" )
 

python/GafferImageTest/DisplayTransformTest.py

@@ -256,7 +256,7 @@ def testDisplayAndViewDefaultToConfig( self ) :
 		explicitDisplayTransform["view"].setValue( config.getDefaultView( config.getDefaultDisplay() ) )
 
 		with Gaffer.Context() as context :
-			GafferImage.OpenColorIOAlgo.setConfig( context, str( configPath ) )
+			GafferImage.OpenColorIOAlgo.setConfig( context, configPath.as_posix() )
 			self.assertImagesEqual( defaultDisplayTransform["out"], explicitDisplayTransform["out"] )
 
 if __name__ == "__main__":

python/GafferImageTest/ImageReaderTest.py

@@ -524,7 +524,7 @@ def f( fileName, fileFormat, dataType, metadata, config ) :
 			( "exr", "openexr", "half", "" ),
 			( "dpx", "dpx", "uint12", "" ),
 			( "TIFF", "tiff", "float", "" ),
-			( "tif", "tiff", "uint32", str( self.openColorIOPath() / "context.ocio" ) ),
+			( "tif", "tiff", "uint32", ( self.openColorIOPath() / "context.ocio" ).as_posix() ),
 		] :
 
 			w["fileName"].setValue( self.temporaryDirectory() / "{0}.{1}".format( dataType, ext ) )

python/GafferImageTest/ImageWriterTest.py

@@ -1241,7 +1241,7 @@ def f( fileName, fileFormat, dataType, metadata, config ) :
 			( "exr", "openexr", "half", "" ),
 			( "dpx", "dpx", "uint12", "" ),
 			( "TIFF", "tiff", "float", "" ),
-			( "tif", "tiff", "uint32", str( self.openColorIOPath() / "context.ocio" ) ),
+			( "tif", "tiff", "uint32", ( self.openColorIOPath() / "context.ocio" ).as_posix() ),
 		] :
 
 			w["fileName"].setValue( self.temporaryDirectory() / "{0}.{1}".format( dataType, ext ) )

python/GafferImageTest/OpenColorIOAlgoTest.py

@@ -68,7 +68,7 @@ def testCurrentConfig( self ) :
 
 		with Gaffer.Context() as c :
 
-			GafferImage.OpenColorIOAlgo.setConfig( c, str( self.openColorIOPath() / "context.ocio" ) )
+			GafferImage.OpenColorIOAlgo.setConfig( c, ( self.openColorIOPath() / "context.ocio" ).as_posix() )
 			customConfig = GafferImage.OpenColorIOAlgo.currentConfig()
 			self.assertIsInstance( customConfig, PyOpenColorIO.Config )
 			self.assertEqual( customConfig.getCacheID(), PyOpenColorIO.Config.CreateFromFile( c["ocio:config"] ).getCacheID() )
@@ -89,7 +89,7 @@ def testCurrentConfigAndContext( self ) :
 
 		with Gaffer.Context() as c :
 
-			GafferImage.OpenColorIOAlgo.setConfig( c, str( self.openColorIOPath() / "context.ocio" ) )
+			GafferImage.OpenColorIOAlgo.setConfig( c, ( self.openColorIOPath() / "context.ocio" ).as_posix() )
 			GafferImage.OpenColorIOAlgo.addVariable( c, "LUT", "srgb.spi1d" )
 
 			customConfig, customContext = GafferImage.OpenColorIOAlgo.currentConfigAndContext()

python/GafferImageTest/OpenColorIOConfigPlugTest.py

@@ -117,5 +117,34 @@ def testConfigAppliesDuringExecution( self ) :
 		with open( script["writer"]["fileName"].getValue() ) as f :
 			self.assertEqual( f.readlines(), [ "test.ocio, testValueA" ] )
 
+	def testContextVariableInConfigVariable( self ) :
+
+		script = Gaffer.ScriptNode()
+		script["variables"].addChild( Gaffer.NameValuePlug( "shot", "shot001", defaultEnabled = True, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) )
+
+		plug = GafferImage.OpenColorIOConfigPlug.acquireDefaultConfigPlug( script )
+		plug["config"].setValue( self.openColorIOPath() / "context.ocio" )
+		plug["variables"].addChild( Gaffer.NameValuePlug( "CDL", "${shot}", defaultEnabled = True, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) )
+
+		with script.context() :
+			ocioConfig, ocioContext = GafferImage.OpenColorIOAlgo.currentConfigAndContext()
+			self.assertEqual( ocioContext["CDL"], "shot001" )
+			hashes = { GafferImage.OpenColorIOAlgo.currentConfigAndContextHash() }
+
+		script["variables"][0]["value"].setValue( "shot002" )
+
+		with script.context() :
+			ocioConfig, ocioContext = GafferImage.OpenColorIOAlgo.currentConfigAndContext()
+			self.assertEqual( ocioContext["CDL"], "shot002" )
+			hashes.add( GafferImage.OpenColorIOAlgo.currentConfigAndContextHash() )
+			self.assertEqual( len( hashes ), 2 )
+
+		with Gaffer.Context( script.context() ) as c :
+			c["shot"] = "shot003"
+			ocioConfig, ocioContext = GafferImage.OpenColorIOAlgo.currentConfigAndContext()
+			self.assertEqual( ocioContext["CDL"], "shot003" )
+			hashes.add( GafferImage.OpenColorIOAlgo.currentConfigAndContextHash() )
+			self.assertEqual( len( hashes ), 3 )
+
 if __name__ == "__main__":
 	unittest.main()

src/GafferImage/OpenColorIOAlgo.cpp

@@ -161,18 +161,21 @@ std::vector<std::string> GafferImage::OpenColorIOAlgo::variables( const Gaffer::
 OCIO_NAMESPACE::ConstConfigRcPtr GafferImage::OpenColorIOAlgo::currentConfig()
 {
 	const Gaffer::Context *context = Gaffer::Context::current();
-	return g_configCache.get( getConfig( context ) );
+	return g_configCache.get( context->substitute( getConfig( context ) ) );
 }
 
 IECore::MurmurHash GafferImage::OpenColorIOAlgo::currentConfigHash()
 {
-	return Gaffer::Context::current()->variableHash( g_ocioConfigContextName );
+	const Gaffer::Context *context = Gaffer::Context::current();
+	IECore::MurmurHash result;
+	result.append( context->substitute( getConfig( context ) ) );
+	return result;
 }
 
 GafferImage::OpenColorIOAlgo::ConfigAndContext GafferImage::OpenColorIOAlgo::currentConfigAndContext()
 {
 	const Gaffer::Context *gafferContext = Gaffer::Context::current();
-	OCIO_NAMESPACE::ConstConfigRcPtr config = g_configCache.get( getConfig( gafferContext ) );
+	OCIO_NAMESPACE::ConstConfigRcPtr config = g_configCache.get( gafferContext->substitute( getConfig( gafferContext ) ) );
 
 	OCIO_NAMESPACE::ConstContextRcPtr context = config->getCurrentContext();
 	OCIO_NAMESPACE::ContextRcPtr mutableContext;
@@ -198,9 +201,10 @@ GafferImage::OpenColorIOAlgo::ConfigAndContext GafferImage::OpenColorIOAlgo::cur
 			context = mutableContext;
 		}
 
+		const string value = gafferContext->substitute( gafferContext->get<string>( n ) );
 		mutableContext->setStringVar(
 			n.c_str() + g_ocioStringVarPrefix.size(),
-			gafferContext->get<string>( n ).c_str()
+			value.c_str()
 		);
 	}
 
@@ -219,7 +223,7 @@ IECore::MurmurHash GafferImage::OpenColorIOAlgo::currentConfigAndContextHash()
 	{
 		if( n == g_ocioConfigContextName || boost::starts_with( n.string(), g_ocioStringVarPrefix ) )
 		{
-			result.append( context->variableHash( n ) );
+			result.append( context->substitute( context->get<string>( n ) ) );
 		}
 	}
 	return result;

src/GafferImage/OpenColorIOConfigPlug.cpp

@@ -54,16 +54,6 @@ using namespace GafferImage;
 namespace
 {
 
-string substitutedValue( const StringPlug *plug )
-{
-	string result = plug->getValue();
-	if( auto script = plug->ancestor<ScriptNode>() )
-	{
-		result = script->context()->substitute( result );
-	}
-	return result;
-}
-
 const IECore::InternedString g_defaultConfigPlugName( "openColorIO" );
 
 } // namespace
@@ -180,8 +170,8 @@ void OpenColorIOConfigPlug::plugSet( Gaffer::Plug *plug )
 	auto *scriptNode = parent<ScriptNode>();
 	assert( scriptNode );
 
-	OpenColorIOAlgo::setConfig( scriptNode->context(), substitutedValue( configPlug() ) );
-	OpenColorIOAlgo::setWorkingSpace( scriptNode->context(), substitutedValue( workingSpacePlug() ) );
+	OpenColorIOAlgo::setConfig( scriptNode->context(), configPlug()->getValue() );
+	OpenColorIOAlgo::setWorkingSpace( scriptNode->context(), workingSpacePlug()->getValue() );
 
 	// Add variables
 
@@ -196,15 +186,15 @@ void OpenColorIOConfigPlug::plugSet( Gaffer::Plug *plug )
 			}
 		}
 
-		const string name = substitutedValue( variable->namePlug() );
+		const string name = scriptNode->context()->substitute( variable->namePlug()->getValue() );
 		if( name.empty() )
 		{
 			continue;
 		}
 
 		if( auto stringPlug = variable->valuePlug<StringPlug>() )
 		{
-			OpenColorIOAlgo::addVariable( scriptNode->context(), name, substitutedValue( stringPlug ) );
+			OpenColorIOAlgo::addVariable( scriptNode->context(), name, stringPlug->getValue() );
 			validVariables.insert( name );
 		}
 		else