From 976d28b916fa956c6e2fde26deaed66999bf5043 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Thu, 13 Jun 2024 11:33:05 +0100 Subject: [PATCH] OpenColorIO : Defer expansion of context variables in OCIO vars Previously, the OpenColorIOConfigPlug was baking in values from the current context at the point the OCIO plugs were edited on the ScriptNode. This meant that later changes to the context variable were not reflected in the OCIO variable. Among other things, this meant that the Viewer LUT would fail to update when changing shot in certain multi-shot workflows. Moving the variable expansion to `OpenColorIOAlgo::currentContextAndConfig()` fixes this problem, but also enables more advanced workflows. For instance, a Wedge or ContactSheet node can now set the variable being referenced, and the OCIO config used by OpenColorIOTransform nodes will reflect that change dynamically as the graph executes. There is one knock-on effect from this, as demonstrated in the test changes. On Windows, we must now use `/` separators in the context variables where before we could get away with `\`. We think it's unlikely that anyone is creating contexts manually like this in Windows - much more likely is usage of OpenColorIOContext and OpenColorIOConfigPlug where `/` has always been a requirement. So we think the very small risk of disruption is worth it for the benefits provided. --- Changes.md | 6 ++++ python/GafferImageTest/ColorSpaceTest.py | 2 +- .../GafferImageTest/DisplayTransformTest.py | 2 +- python/GafferImageTest/ImageReaderTest.py | 2 +- python/GafferImageTest/ImageWriterTest.py | 2 +- python/GafferImageTest/OpenColorIOAlgoTest.py | 4 +-- .../OpenColorIOConfigPlugTest.py | 29 +++++++++++++++++++ src/GafferImage/OpenColorIOAlgo.cpp | 14 +++++---- src/GafferImage/OpenColorIOConfigPlug.cpp | 18 +++--------- 9 files changed, 54 insertions(+), 25 deletions(-) diff --git a/Changes.md b/Changes.md index 2b212ce7463..3b032512d26 100644 --- a/Changes.md +++ b/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 --- diff --git a/python/GafferImageTest/ColorSpaceTest.py b/python/GafferImageTest/ColorSpaceTest.py index 6591d8d1a51..9da4453f0b9 100644 --- a/python/GafferImageTest/ColorSpaceTest.py +++ b/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" ) diff --git a/python/GafferImageTest/DisplayTransformTest.py b/python/GafferImageTest/DisplayTransformTest.py index f16c24f0502..59077b7df08 100644 --- a/python/GafferImageTest/DisplayTransformTest.py +++ b/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__": diff --git a/python/GafferImageTest/ImageReaderTest.py b/python/GafferImageTest/ImageReaderTest.py index 4f38b9a9c5d..07f86b2f469 100644 --- a/python/GafferImageTest/ImageReaderTest.py +++ b/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 ) ) diff --git a/python/GafferImageTest/ImageWriterTest.py b/python/GafferImageTest/ImageWriterTest.py index 8d79be16357..e7230a54205 100644 --- a/python/GafferImageTest/ImageWriterTest.py +++ b/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 ) ) diff --git a/python/GafferImageTest/OpenColorIOAlgoTest.py b/python/GafferImageTest/OpenColorIOAlgoTest.py index a7e08320144..4a3125aae3b 100644 --- a/python/GafferImageTest/OpenColorIOAlgoTest.py +++ b/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() diff --git a/python/GafferImageTest/OpenColorIOConfigPlugTest.py b/python/GafferImageTest/OpenColorIOConfigPlugTest.py index c33f471a343..daca3186da2 100644 --- a/python/GafferImageTest/OpenColorIOConfigPlugTest.py +++ b/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() diff --git a/src/GafferImage/OpenColorIOAlgo.cpp b/src/GafferImage/OpenColorIOAlgo.cpp index f1f0b10d281..be694aefa96 100644 --- a/src/GafferImage/OpenColorIOAlgo.cpp +++ b/src/GafferImage/OpenColorIOAlgo.cpp @@ -161,18 +161,21 @@ std::vector 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( n ) ); mutableContext->setStringVar( n.c_str() + g_ocioStringVarPrefix.size(), - gafferContext->get( 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( n ) ) ); } } return result; diff --git a/src/GafferImage/OpenColorIOConfigPlug.cpp b/src/GafferImage/OpenColorIOConfigPlug.cpp index 8f62e419b03..34e8821ed01 100644 --- a/src/GafferImage/OpenColorIOConfigPlug.cpp +++ b/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() ) - { - 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(); 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,7 +186,7 @@ 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; @@ -204,7 +194,7 @@ void OpenColorIOConfigPlug::plugSet( Gaffer::Plug *plug ) if( auto stringPlug = variable->valuePlug() ) { - OpenColorIOAlgo::addVariable( scriptNode->context(), name, substitutedValue( stringPlug ) ); + OpenColorIOAlgo::addVariable( scriptNode->context(), name, stringPlug->getValue() ); validVariables.insert( name ); } else