From d8458d80afb72d7d9af827ccd6075ab492efd6a5 Mon Sep 17 00:00:00 2001 From: Daniel Dresser Date: Thu, 7 Nov 2024 09:12:45 -0800 Subject: [PATCH] ImageWriter : Match Nuke view metadata when using Nuke layouts. This should address an issue where EXRs written from Gaffer using Nuke layouts sometimes did not load correctly in Nuke. Fixes #6120. --- Changes.md | 1 + python/GafferImageTest/ImageWriterTest.py | 3 - src/GafferImage/ImageWriter.cpp | 67 ++++++++++++++++++----- 3 files changed, 54 insertions(+), 17 deletions(-) diff --git a/Changes.md b/Changes.md index b836392d358..42f47729002 100644 --- a/Changes.md +++ b/Changes.md @@ -21,6 +21,7 @@ Fixes - LocalDispatcher : Fixed job status update when a job was killed _immediately_ after being launched. - `gaffer view` : Fixed default OpenColorIO display transform. - AnimationEditor : Fixed changing of the current frame by dragging the frame indicator or clicking on the time axis. +- ImageWriter : Matched view metadata to Nuke when using the Nuke options for `layout`. This should address an issue where EXRs written from Gaffer using Nuke layouts sometimes did not load correctly in Nuke (#6120). In the unlikely situation that you were relying on the old behaviour, you can set the env var `GAFFERIMAGE_IMAGEWRITER_OMIT_DEFAULT_NUKE_VIEW = 1` in order to keep the old behaviour. API --- diff --git a/python/GafferImageTest/ImageWriterTest.py b/python/GafferImageTest/ImageWriterTest.py index e7230a54205..269df2f35dc 100644 --- a/python/GafferImageTest/ImageWriterTest.py +++ b/python/GafferImageTest/ImageWriterTest.py @@ -1636,9 +1636,6 @@ def testWithChannelTestImage( self ): header = self.usefulHeader( writePath ) refHeader = self.usefulHeader( self.imagesPath() / ( "channelTest" + referenceFile + ".exr" ) ) - if layout == "Nuke/Interleave Channels": - # We don't match the view metadata which Nuke sticks on files without specific views - refHeader = list( filter( lambda i : i != 'view (type string): "main"', refHeader ) ) self.assertEqual( header, refHeader ) def testWithMultiViewChannelTestImage( self ): diff --git a/src/GafferImage/ImageWriter.cpp b/src/GafferImage/ImageWriter.cpp index 9f2753ddd01..9821164e4d7 100644 --- a/src/GafferImage/ImageWriter.cpp +++ b/src/GafferImage/ImageWriter.cpp @@ -1379,8 +1379,17 @@ std::string cleanExcessDots( std::string name ) return name; } +struct LayoutForChannel +{ + std::string partName; + std::string channelName; + std::string requestedDataType; + bool usesNukeView; +}; + + // Get the EXR names for the part, and channel for a Gaffer channel, along with the channel's data type -std::tuple< std::string, std::string, std::string > partChannelNameDataType( const ImageWriter *node, const StringPlug *dataTypePlug, const std::string &view, const std::string &gafferChannel, const std::vector< std::string > &viewNames, bool isDeep ) +LayoutForChannel evaluateLayoutForChannel( const ImageWriter *node, const StringPlug *dataTypePlug, const std::string &view, const std::string &gafferChannel, const std::vector< std::string > &viewNames, bool isDeep, bool testNukeView = false ) { std::string layer = ImageAlgo::layerName( gafferChannel ); std::string baseName = ImageAlgo::baseName( gafferChannel ); @@ -1456,11 +1465,36 @@ std::tuple< std::string, std::string, std::string > partChannelNameDataType( con namingContext.set( "imageWriter:nukeLayerName", &nukeLayerName ); namingContext.set( "imageWriter:nukeBaseName", &nukeBaseName ); - return std::make_tuple( + if( testNukeView ) + { + static const std::string nukeViewTestToken( "__nukeViewTest" ); + namingContext.set( "imageWriter:nukeViewName", &nukeViewTestToken ); + return { "", "", "", node->layoutPartNamePlug()->getValue().find( nukeViewTestToken ) != std::string::npos }; + } + + return { cleanExcessDots( node->layoutPartNamePlug()->getValue() ), cleanExcessDots( node->layoutChannelNamePlug()->getValue() ), - dataTypePlug ? dataTypePlug->getValue() : "" - ); + dataTypePlug ? dataTypePlug->getValue() : "", + false + }; +} + +bool testNukeView( const ImageWriter *node, const StringPlug *dataTypePlug, const std::string &gafferChannel ) +{ + // Do a special evaluation to test whether layoutPartName depends on the nukeViewName context + // variable, because if it does, we enable some Nuke specific behaviour ( Nuke sometimes names + // image parts after the view even if no views are used, and then those parts need to get their + // view metadata set ). + + // The alternative to this would be to disable substitutions on the plug, and do the substitutions + // ourselves with a custom Context::SubstitutionProvider that tracks whether nukeViewName is used. + // That wouldn't handle the case where layoutPartName depends on an expression that uses nukeViewName, + // though. + const std::vector defaultViews = { ImagePlug::defaultViewName }; + return evaluateLayoutForChannel( + node, dataTypePlug, ImagePlug::defaultViewName, gafferChannel, defaultViews, false, true + ).usesNukeView; } struct MetadataRegistration @@ -1952,9 +1986,8 @@ void ImageWriter::execute() const for( const string &i : channelsToWrite ) { - const auto & [ partName, channelName, requestedDataType ] = partChannelNameDataType( this, dataTypePlug, viewName, i, viewNames, defaultSpec.deep ); - - std::string dataType = requestedDataType; + LayoutForChannel layout = evaluateLayoutForChannel( this, dataTypePlug, viewName, i, viewNames, defaultSpec.deep ); + std::string dataType = layout.requestedDataType; if( depthDataTypeOverride != "" && ( i == "Z" || i == "ZBack" ) ) { dataType = depthDataTypeOverride; @@ -1968,17 +2001,14 @@ void ImageWriter::execute() const viewDataType = dataType; } - // Note that `partName = partName` is a hack to get around an issue with capturing from a - // structured binding. GCC allows it, but the C++17 spec doesn't, and it doesn't work in our - // Mac compiler. Once we're on C++20, it is explicitly supported, and we can remove the ` = partName` size_t partIndex = std::distance( parts.begin(), - std::find_if( parts.begin(), parts.end(), [partName = partName] (Part const& p) { return p.name == partName; } ) + std::find_if( parts.begin(), parts.end(), [&layout] (Part const& p) { return p.name == layout.partName; } ) ); if( partIndex >= parts.size() ) { - parts.push_back( { partName, { viewName }, defaultSpec, imageFormat, dataWindow, {}, {}, {} } ); + parts.push_back( { layout.partName, { viewName }, defaultSpec, imageFormat, dataWindow, {}, {}, {} } ); } else { @@ -1998,10 +2028,10 @@ void ImageWriter::execute() const } parts[ partIndex ].channels.push_back( viewName + "." + i ); - parts[ partIndex ].channelNames.push_back( channelName ); + parts[ partIndex ].channelNames.push_back( layout.channelName ); parts[ partIndex ].channelDataTypes.push_back( dataType ); - if( channelName == "A" ) + if( layout.channelName == "A" ) { hasAlpha = true; } @@ -2045,6 +2075,11 @@ void ImageWriter::execute() const } } + // \todo: deprecated, this variable can be treated as always true in the next major version, + // once we're confident no one is relying on the old behaviour. + static const char *nukeViewMetadataDeprecatedBehaviourString = getenv( "GAFFERIMAGE_IMAGEWRITER_OMIT_DEFAULT_NUKE_VIEW" ); + static const bool nukeViewMetadataDeprecatedBehaviour = nukeViewMetadataDeprecatedBehaviourString && std::string( nukeViewMetadataDeprecatedBehaviourString ) != "0"; + for( Part &part : parts ) { if( part.views.size() > 1 ) @@ -2069,6 +2104,10 @@ void ImageWriter::execute() const { part.spec.attribute("view", part.views[0] ); } + else if( ( !nukeViewMetadataDeprecatedBehaviour ) && testNukeView( this, dataTypePlug, part.channels[0] ) ) + { + part.spec.attribute("view", "main" ); + } if( part.name.size() ) {