From 6366b40c0d30c687ff048f736724fdd3db4e7c1f Mon Sep 17 00:00:00 2001 From: John Haddon Date: Wed, 15 Nov 2023 10:10:42 +0000 Subject: [PATCH 01/10] ScenePathPlugValueWidget : Use `SceneAlgo.sourceScene()` to find scene This allows us to find a scene when the focused node outputs a rendered image rather than a scene. --- python/GafferSceneUI/ScenePathPlugValueWidget.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/python/GafferSceneUI/ScenePathPlugValueWidget.py b/python/GafferSceneUI/ScenePathPlugValueWidget.py index 26ff2e11c3f..cf1cf2be64a 100644 --- a/python/GafferSceneUI/ScenePathPlugValueWidget.py +++ b/python/GafferSceneUI/ScenePathPlugValueWidget.py @@ -36,6 +36,7 @@ import Gaffer import GafferUI +import GafferImage import GafferScene class ScenePathPlugValueWidget( GafferUI.PathPlugValueWidget ) : @@ -111,7 +112,12 @@ def predicate( plug ) : focusNode = plug.ancestor( Gaffer.ScriptNode ).getFocus() if focusNode is not None : - return next( GafferScene.ScenePlug.RecursiveOutputRange( focusNode ), None ) + outputScene = next( GafferScene.ScenePlug.RecursiveOutputRange( focusNode ), None ) + if outputScene is not None : + return outputScene + outputImage = next( GafferImage.ImagePlug.RecursiveOutputRange( focusNode ), None ) + if outputImage is not None : + return GafferScene.SceneAlgo.sourceScene( outputImage ) def __focusChanged( self, scriptNode, node ) : From 8540262ce5194fd844896b7be281d9ea98c0b2f0 Mon Sep 17 00:00:00 2001 From: ivanimanishi Date: Wed, 15 Nov 2023 13:43:05 -0800 Subject: [PATCH 02/10] ie/options : Updated to support building for Nuke 13 --- config/ie/options | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/config/ie/options b/config/ie/options index 6a6fc66e66c..7024974e030 100644 --- a/config/ie/options +++ b/config/ie/options @@ -252,6 +252,7 @@ ocioVersion = targetAppReg.get( "OpenColorIOVersion", cortexReg["OpenColorIOVers exrVersion = targetAppReg.get( "OpenEXRVersion", cortexReg["OpenEXRVersion"] ) oiioVersion = targetAppReg.get( "OpenImageIOVersion", cortexReg["OpenImageIOVersion"] ) oiioLibSuffix = targetAppReg.get( "OpenImageIOLibSuffix", oiioVersion ) +ocioLibSuffix = targetAppReg.get( "OpenColorIOLibSuffix", IEEnv.BuildUtil.libSuffix( "OpenColorIO", ocioVersion ) ) vdbVersion = targetAppReg.get( "OpenVDBVersion", cortexReg["OpenVDBVersion"] ) oslVersion = targetAppReg.get( "OpenShadingLanguageVersion", gafferReg.get( "OpenShadingLanguageVersion", cortexReg["OpenShadingLanguageVersion"] ) ) tbbVersion = targetAppReg.get( "tbbVersion", cortexReg["tbbVersion"] ) @@ -337,6 +338,7 @@ LOCATE_DEPENDENCY_LIBPATH = [ os.path.join( IEEnv.Environment.rootPath(), "apps", "openexr", exrVersion, IEEnv.platform(), compiler, compilerVersion, "python", pythonVersion, "boost", boostVersion, "lib64" ), os.path.join( IEEnv.Environment.rootPath(), "tools", "lib", IEEnv.platform(), compiler, compilerVersion ), os.path.join( IEEnv.Environment.rootPath(), "apps", "embree", embreeVersion, IEEnv.platform(), compiler, compilerVersion, "lib" ), + os.path.join( IEEnv.Environment.rootPath(), "apps", "OpenColorIO", ocioVersion, IEEnv.platform(), compiler, compilerVersion, "lib" ), ] @@ -487,7 +489,9 @@ if "install" in sys.argv : IMATH_LIB_SUFFIX = IEEnv.BuildUtil.libSuffix( "OpenEXR", exrVersion ) OIIO_LIB_SUFFIX = IEEnv.BuildUtil.libSuffix( "OpenImageIO", oiioLibSuffix ) -OCIO_LIB_SUFFIX = IEEnv.BuildUtil.libSuffix( "OpenColorIO", ocioVersion ) +# ocioLibSuffix, when provided, should be used directly, without the addition of "-" +# For example, when set to "Foundry" +OCIO_LIB_SUFFIX = ocioLibSuffix OSL_LIB_SUFFIX = IEEnv.BuildUtil.libSuffix( "OpenShadingLanguage", oslVersion ) BOOST_LIB_SUFFIX = IEEnv.BuildUtil.libSuffix( "boost", boostVersion, { "compiler" : compiler, "compilerVersion" : compilerVersion } ) BOOST_PYTHON_LIB_SUFFIX = IEEnv.BuildUtil.libSuffix( From d3fecf8e22c9999b204b5f7c0793aeb6bc828e1c Mon Sep 17 00:00:00 2001 From: John Haddon Date: Thu, 16 Nov 2023 15:06:24 +0000 Subject: [PATCH 03/10] BackgroundMethodTest : Increase `waitForIdle()` count We've been seeing regular failures on Windows CI in `assertIsNone( w() )` at L259. `w` can only die when the BackgroundMethod has returned, so my assumption is that when a CI machine is under heavy load, we're not waiting long enough for this to happen. So I'm upping the limit in the hope that it helps matters. --- python/GafferUITest/BackgroundMethodTest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/GafferUITest/BackgroundMethodTest.py b/python/GafferUITest/BackgroundMethodTest.py index 7a1bf850de0..502bfc0900a 100644 --- a/python/GafferUITest/BackgroundMethodTest.py +++ b/python/GafferUITest/BackgroundMethodTest.py @@ -254,7 +254,7 @@ def testDeleteWidgetDuringCall( self ) : # a zombie widget, we'd get PySide errors in our redirected output, # so make sure that doesn't happen. del window - self.waitForIdle( 1000 ) + self.waitForIdle( 10000 ) self.assertIsNone( w() ) self.assertEqual( out, [] ) From 5b618002e951323d16a65537a8f602f7077ac5ed Mon Sep 17 00:00:00 2001 From: Eric Mehl Date: Fri, 10 Nov 2023 12:37:07 -0500 Subject: [PATCH 04/10] OpenGLShaderUI : Fix leading `\` on Windows menu --- Changes.md | 3 +++ python/GafferSceneUI/OpenGLShaderUI.py | 11 ++++++----- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/Changes.md b/Changes.md index 167afddc6f8..ed7a4344a30 100644 --- a/Changes.md +++ b/Changes.md @@ -1,7 +1,10 @@ 1.3.x.x (relative to 1.3.7.0) ======= +Fixes +----- +- Windows `Scene/OpenGL/Shader` Menu : Removed `\` at the beginning of menu items. 1.3.7.0 (relative to 1.3.6.1) ======= diff --git a/python/GafferSceneUI/OpenGLShaderUI.py b/python/GafferSceneUI/OpenGLShaderUI.py index d4f14ee7a73..e173e9b725d 100644 --- a/python/GafferSceneUI/OpenGLShaderUI.py +++ b/python/GafferSceneUI/OpenGLShaderUI.py @@ -37,6 +37,7 @@ import os import string import functools +import pathlib import IECore @@ -99,11 +100,11 @@ def shaderSubMenu() : # a lot of irrelevancies at IE at the moment. paths = [ os.environ["GAFFER_ROOT"] + "/glsl" ] for path in paths : - for root, dirs, files in os.walk( path ) : - for file in files : - if os.path.splitext( file )[1] in ( ".vert", ".frag" ) : - shaderPath = os.path.join( root, file ).partition( path )[-1].lstrip( "/" ) - shaders.add( os.path.splitext( shaderPath )[0] ) + for extension in [ ".vert", ".frag" ] : + shaderPaths = pathlib.Path( path ).glob( "**/*" + extension ) + + for shaderPath in shaderPaths : + shaders.add( shaderPath.relative_to( path ).as_posix()[:-len( extension )] ) result = IECore.MenuDefinition() for shader in sorted( list( shaders ) ) : From d06fc1f9ea92163fbd83f374da2b211e20b89832 Mon Sep 17 00:00:00 2001 From: Eric Mehl Date: Mon, 6 Nov 2023 17:41:28 -0500 Subject: [PATCH 05/10] SceneGadget : Add `snapshotToFile()` --- Changes.md | 5 +++ SConstruct | 2 +- include/GafferSceneUI/Private/OutputBuffer.h | 8 ++++ include/GafferSceneUI/SceneGadget.h | 13 ++++++ src/GafferSceneUI/OutputBuffer.cpp | 43 +++++++++++++++++++ src/GafferSceneUI/SceneGadget.cpp | 14 ++++++ .../SceneGadgetBinding.cpp | 11 +++++ 7 files changed, 95 insertions(+), 1 deletion(-) diff --git a/Changes.md b/Changes.md index ed7a4344a30..ce20d6695f6 100644 --- a/Changes.md +++ b/Changes.md @@ -6,6 +6,11 @@ Fixes - Windows `Scene/OpenGL/Shader` Menu : Removed `\` at the beginning of menu items. +API +--- + +- SceneGadget : Added `snapshotToFile()` method. + 1.3.7.0 (relative to 1.3.6.1) ======= diff --git a/SConstruct b/SConstruct index 86971382166..9a3eb646bd2 100644 --- a/SConstruct +++ b/SConstruct @@ -1105,7 +1105,7 @@ libraries = { "GafferSceneUI" : { "envAppends" : { - "LIBS" : [ "Gaffer", "GafferUI", "GafferImage", "GafferImageUI", "GafferScene", "Iex$IMATH_LIB_SUFFIX", "IECoreGL$CORTEX_LIB_SUFFIX", "IECoreImage$CORTEX_LIB_SUFFIX", "IECoreScene$CORTEX_LIB_SUFFIX" ], + "LIBS" : [ "Gaffer", "GafferUI", "GafferImage", "GafferImageUI", "GafferScene", "Iex$IMATH_LIB_SUFFIX", "IECoreGL$CORTEX_LIB_SUFFIX", "IECoreImage$CORTEX_LIB_SUFFIX", "IECoreScene$CORTEX_LIB_SUFFIX", "OpenImageIO$OIIO_LIB_SUFFIX", "OpenImageIO_Util$OIIO_LIB_SUFFIX" ], }, "pythonEnvAppends" : { "LIBS" : [ "IECoreGL$CORTEX_LIB_SUFFIX", "GafferBindings", "GafferScene", "GafferImage", "GafferUI", "GafferImageUI", "GafferSceneUI", "IECoreScene$CORTEX_LIB_SUFFIX" ], diff --git a/include/GafferSceneUI/Private/OutputBuffer.h b/include/GafferSceneUI/Private/OutputBuffer.h index 9c8b8ef956b..94bba165982 100644 --- a/include/GafferSceneUI/Private/OutputBuffer.h +++ b/include/GafferSceneUI/Private/OutputBuffer.h @@ -44,6 +44,7 @@ #include "IECore/PathMatcher.h" +#include #include namespace GafferSceneUI @@ -81,6 +82,13 @@ class OutputBuffer using BufferChangedSignal = Gaffer::Signals::Signal; BufferChangedSignal &bufferChangedSignal(); + /// See `SceneGadget::snapshotToFile()` for documentation. + void snapshotToFile( + const std::filesystem::path &fileName, + const Imath::Box2f &resolutionGate = Imath::Box2f(), + const IECore::CompoundData *metadata = nullptr + ); + private : class DisplayDriver; diff --git a/include/GafferSceneUI/SceneGadget.h b/include/GafferSceneUI/SceneGadget.h index 26fdbbc1ad3..bba05075bba 100644 --- a/include/GafferSceneUI/SceneGadget.h +++ b/include/GafferSceneUI/SceneGadget.h @@ -53,6 +53,8 @@ #include "IECoreGL/State.h" +#include + namespace GafferSceneUI { @@ -191,6 +193,17 @@ class GAFFERSCENEUI_API SceneGadget : public GafferUI::Gadget /// Implemented to return the name of the object under the mouse. std::string getToolTip( const IECore::LineSegment3f &line ) const override; + /// Saves a snapshot of the current rendered scene. All renderers are supported _except_ + /// the OpenGL renderer. All formats supported by OpenImageIO can be used. The output + /// display window will be set to `resolutionGate` if it is not an empty `Box2f`. + /// All of the supplied metadata will be written, regardless of conflicts with + /// OpenImageIO built-in metadata. + void snapshotToFile( + const std::filesystem::path &fileName, + const Imath::Box2f &resolutionGate = Imath::Box2f(), + const IECore::CompoundData *metadata = nullptr + ) const; + protected : void renderLayer( Layer layer, const GafferUI::Style *style, RenderReason reason ) const override; diff --git a/src/GafferSceneUI/OutputBuffer.cpp b/src/GafferSceneUI/OutputBuffer.cpp index 7071966fcbf..cbf5122fcd6 100644 --- a/src/GafferSceneUI/OutputBuffer.cpp +++ b/src/GafferSceneUI/OutputBuffer.cpp @@ -37,6 +37,7 @@ #include "IECoreGL/ShaderLoader.h" #include "IECoreImage/DisplayDriver.h" +#include "IECoreImage/OpenImageIOAlgo.h" #include "OpenEXR/OpenEXRConfig.h" #if OPENEXR_VERSION_MAJOR < 3 @@ -45,6 +46,8 @@ #include "Imath/ImathBoxAlgo.h" #endif +#include "OpenImageIO/imageio.h" + #include "boost/lexical_cast.hpp" #include @@ -462,6 +465,46 @@ void OutputBuffer::dirtyTexture() } } +void OutputBuffer::snapshotToFile( + const std::filesystem::path &fileName, + const Box2f &resolutionGate, + const CompoundData *metadata +) +{ + std::filesystem::create_directories( fileName.parent_path() ); + + std::unique_lock lock( m_bufferReallocationMutex ); + + OIIO::ImageSpec spec( m_dataWindow.size().x + 1, m_dataWindow.size().y + 1, 4, OIIO::TypeDesc::HALF ); + + if( !resolutionGate.isEmpty() ) + { + spec.x = -resolutionGate.min.x; + spec.y = -resolutionGate.min.y; + + spec.full_x = 0; + spec.full_y = 0; + spec.full_width = resolutionGate.size().x; + spec.full_height = resolutionGate.size().y; + } + + const std::vector &rgbaBuffer = m_rgbaBuffer; + + for( const auto &[key, value] : metadata->readable() ) + { + const IECoreImage::OpenImageIOAlgo::DataView dataView( value.get() ); + if( dataView.data ) + { + spec.attribute( key.c_str(), dataView.type, dataView.data ); + } + } + + std::unique_ptr output = OIIO::ImageOutput::create( fileName.c_str() ); + output->open( fileName.c_str(), spec ); + output->write_image( OIIO::TypeDesc::FLOAT, &rgbaBuffer[0] ); + output->close(); +} + ////////////////////////////////////////////////////////////////////////// // DisplayDriver ////////////////////////////////////////////////////////////////////////// diff --git a/src/GafferSceneUI/SceneGadget.cpp b/src/GafferSceneUI/SceneGadget.cpp index 031ef4e25c0..68264f2fc9b 100644 --- a/src/GafferSceneUI/SceneGadget.cpp +++ b/src/GafferSceneUI/SceneGadget.cpp @@ -780,6 +780,20 @@ Imath::Box3f SceneGadget::bound() const return bound( /* selection = */ false ); } +void SceneGadget::snapshotToFile( + const std::filesystem::path &fileName, + const Box2f &resolutionGate, + const CompoundData *metadata +) const +{ + if( !m_outputBuffer ) + { + return; + } + + m_outputBuffer->snapshotToFile( fileName, resolutionGate, metadata ); +} + void SceneGadget::renderLayer( Layer layer, const GafferUI::Style *style, RenderReason reason ) const { assert( layer == m_layer || layer == Layer::MidFront ); diff --git a/src/GafferSceneUIModule/SceneGadgetBinding.cpp b/src/GafferSceneUIModule/SceneGadgetBinding.cpp index a0295a5bded..93a0fda0a07 100644 --- a/src/GafferSceneUIModule/SceneGadgetBinding.cpp +++ b/src/GafferSceneUIModule/SceneGadgetBinding.cpp @@ -179,6 +179,12 @@ Imath::Box3f bound( SceneGadget &g, bool selected, const IECore::PathMatcher *om return g.bound( selected, omitted ); } +void snapshotToFile( SceneGadget &g, const std::filesystem::path &fileName, const Imath::Box2f &resolutionGate, const IECore::CompoundData *metadata ) +{ + ScopedGILRelease gilRelease; + g.snapshotToFile( fileName, resolutionGate, metadata ); +} + } // namespace void GafferSceneUIModule::bindSceneGadget() @@ -214,6 +220,11 @@ void GafferSceneUIModule::bindSceneGadget() .def( "getSelection", &SceneGadget::getSelection, return_value_policy() ) .def( "selectionBound", &selectionBound ) .def( "bound", &bound, ( arg( "selected" ), arg( "omitted" ) = object() ) ) + .def( + "snapshotToFile", + &snapshotToFile, + ( arg( "fileName" ), arg( "resolutionGate" ) = Imath::Box2f(), arg( "metadata" ) = object() ) + ) ; enum_( "State" ) From 1e1cc0c1138052e7e106d384e24ce0f0fdbaf1ed Mon Sep 17 00:00:00 2001 From: Eric Mehl Date: Mon, 6 Nov 2023 18:25:12 -0500 Subject: [PATCH 06/10] SceneViewUI : Add "Send to Catalog" context menu --- Changes.md | 5 ++ python/GafferSceneUI/SceneViewUI.py | 109 ++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+) diff --git a/Changes.md b/Changes.md index ce20d6695f6..e7d31c1c8c1 100644 --- a/Changes.md +++ b/Changes.md @@ -1,6 +1,11 @@ 1.3.x.x (relative to 1.3.7.0) ======= +Features +-------- + +- Viewer : Added "Snapshot To Catalogue" command to the right-click menu of the 3D view. + Fixes ----- diff --git a/python/GafferSceneUI/SceneViewUI.py b/python/GafferSceneUI/SceneViewUI.py index 04a5834d69f..fe48cd7c5be 100644 --- a/python/GafferSceneUI/SceneViewUI.py +++ b/python/GafferSceneUI/SceneViewUI.py @@ -36,6 +36,8 @@ ########################################################################## import functools +import datetime +import pathlib import imath @@ -46,6 +48,7 @@ import GafferUI import GafferScene import GafferSceneUI +import GafferImage from ._SceneViewInspector import _SceneViewInspector @@ -1082,6 +1085,98 @@ def __appendClippingPlaneMenuItems( menuDefinition, prefix, view, parentWidget ) } ) +def __snapshotDescription( view ) : + + sceneGadget = view.viewportGadget().getPrimaryChild() + if sceneGadget.getRenderer() == "OpenGL" : + return "Viewport snapshots are only available for rendered (non-OpenGL) previews." + + return "Snapshot viewport and send to catalogue." + +def __snapshotToCatalogue( catalogue, view ) : + + timeStamp = str( datetime.datetime.now() ) + + scriptRoot = view["in"].getInput().ancestor( Gaffer.ScriptNode ) + + fileName = pathlib.Path( + scriptRoot.context().substitute( catalogue["directory"].getValue() ) + ) / ( f"viewerSnapshot-{ timeStamp.replace( ':', '-' ).replace(' ', '_' ) }.exr" ) + + resolutionGate = imath.Box3f() + if isinstance( view, GafferSceneUI.SceneView ) : + resolutionGate = view.resolutionGate() + + metadata = IECore.CompoundData( + { + "gaffer:sourceScene" : view["in"].getInput().relativeName( scriptRoot ), + "gaffer:context:frame" : view["in"].node().getContext().getFrame() + } + ) + + sceneGadget = view.viewportGadget().getPrimaryChild() + sceneGadget.snapshotToFile( fileName, resolutionGate, metadata ) + + image = GafferImage.Catalogue.Image( "Snapshot1", Gaffer.Plug.Direction.In, Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) + image["fileName"].setValue( fileName ) + + image["description"].setValue( + "Snapshot of {} at frame {} taken at {}".format( + metadata["gaffer:sourceScene"], + metadata["gaffer:context:frame"], timeStamp[:-6] # Remove trailing microseconds + ) + ) + + catalogue["images"].source().addChild( image ) + catalogue["imageIndex"].source().setValue( len( catalogue["images"].source().children() ) - 1 ) + +def __snapshotCataloguesSubMenu( view, scriptNode ) : + + menuDefinition = IECore.MenuDefinition() + + catalogueList = list( GafferImage.Catalogue.RecursiveRange( scriptNode ) ) + + if len( catalogueList ) == 0 : + menuDefinition.append( + "/No Catalogues Available", + { + "active" : False, + } + ) + + else : + commonDescription = __snapshotDescription( view ) + commonActive = view["renderer"]["name"].getValue() != "OpenGL" + + for c in catalogueList : + + cName = c["name"].getValue() + nName = c["imageIndex"].source().node().relativeName( scriptNode ) + + snapshotActive = ( + commonActive and + not Gaffer.MetadataAlgo.readOnly( c["images"].source() ) and + not Gaffer.MetadataAlgo.readOnly( c["imageIndex"].source() ) + ) + + snapshotDescription = commonDescription + if Gaffer.MetadataAlgo.readOnly( c["images"].source() ) : + snapshotDescription = "\"images\" plug is read-only" + if Gaffer.MetadataAlgo.readOnly( c["imageIndex"].source() ) : + snapshotDescription = "\"imageIndex\" plug is read-only" + + menuDefinition.append( + "/" + ( nName + ( " ({})".format( cName ) if cName else "" ) ), + { + "active" : snapshotActive, + "command" : functools.partial( __snapshotToCatalogue, c, view ), + "description" : snapshotDescription + } + ) + + return menuDefinition + + def __viewContextMenu( viewer, view, menuDefinition ) : if not isinstance( view, GafferSceneUI.SceneView ) : @@ -1089,6 +1184,20 @@ def __viewContextMenu( viewer, view, menuDefinition ) : __appendClippingPlaneMenuItems( menuDefinition, "/Clipping Planes", view, viewer ) + scriptNode = view["in"].getInput().ancestor( Gaffer.ScriptNode ) + + menuDefinition.append( + "/Snapshot to Catalogue", + { + "subMenu" : functools.partial( + __snapshotCataloguesSubMenu, + view, + scriptNode + ) + } + ) + + GafferUI.Viewer.viewContextMenuSignal().connect( __viewContextMenu, scoped = False ) def __plugValueWidgetContextMenu( menuDefinition, plugValueWidget ) : From 7223221d770a2a939038a7b2d570c629228517ea Mon Sep 17 00:00:00 2001 From: John Haddon Date: Thu, 16 Nov 2023 13:01:02 +0000 Subject: [PATCH 07/10] Arnold Renderer : Reuse existing driver nodes where possible Previously we were deleting and recreating the driver node for _any_ change to the output definition. But when we did that, Arnold closed the old drivers before opening the new ones, no matter what order we deleted and created the nodes in. This meant the Catalogue thought the render was over and saved the image, creaing a new one when the new drivers opened. By reusing the existing drivers wherever possible, we are able to continue rendering to the original Catalogue image when `header:` metadata parameters and filter parameters change. --- Changes.md | 1 + .../InteractiveArnoldRenderTest.py | 167 ++++++++++++++++++ src/IECoreArnold/Renderer.cpp | 101 ++++++++--- 3 files changed, 240 insertions(+), 29 deletions(-) diff --git a/Changes.md b/Changes.md index e7d31c1c8c1..9891596242b 100644 --- a/Changes.md +++ b/Changes.md @@ -9,6 +9,7 @@ Features Fixes ----- +- InteractiveArnoldRender : Fixed creation of new Catalogue image when editing output metadata or pixel filter. - Windows `Scene/OpenGL/Shader` Menu : Removed `\` at the beginning of menu items. API diff --git a/python/GafferArnoldTest/InteractiveArnoldRenderTest.py b/python/GafferArnoldTest/InteractiveArnoldRenderTest.py index 0335eb80eee..e8ac5a97fac 100644 --- a/python/GafferArnoldTest/InteractiveArnoldRenderTest.py +++ b/python/GafferArnoldTest/InteractiveArnoldRenderTest.py @@ -747,6 +747,173 @@ def testEditLightGroups( self ) : self.assertEqual( len( script["catalogue"]["images"] ), 1 ) self.assertNotIn( "gaffer:isRendering", script["catalogue"]["out"].metadata() ) + ## \todo Promote to InteractiveRenderTest and check it works for other renderer backends. + def testEditOutputMetadata( self ) : + + script = Gaffer.ScriptNode() + script["catalogue"] = GafferImage.Catalogue() + + script["sphere"] = GafferScene.Sphere() + + script["outputs"] = GafferScene.Outputs() + script["outputs"]["in"].setInput( script["sphere"]["out"] ) + + script["outputs"].addOutput( + "beauty", + IECoreScene.Output( + "test", + "ieDisplay", + "rgba", + { + "driverType" : "ClientDisplayDriver", + "displayHost" : "localhost", + "displayPort" : str( script["catalogue"].displayDriverServer().portNumber() ), + "remoteDisplayType" : "GafferImage::GafferDisplayDriver", + "header:test1" : "hello", + "header:test2" : "world", + } + ) + ) + + script["renderer"] = self._createInteractiveRender() + script["renderer"]["in"].setInput( script["outputs"]["out"] ) + + # Start a render, give it time to finish, and check we have our + # custom metadata in the outputs. + + script["renderer"]["state"].setValue( script["renderer"].State.Running ) + self.uiThreadCallHandler.waitFor( 1 ) + + self.assertEqual( len( script["catalogue"]["images"] ), 1 ) + self.assertEqual( script["catalogue"]["out"].metadata()["test1"], IECore.StringData( "hello" ) ) + self.assertEqual( script["catalogue"]["out"].metadata()["test2"], IECore.StringData( "world" ) ) + + # Modify the header parameters and rerender. + + with Gaffer.DirtyPropagationScope() : + + script["outputs"]["outputs"][0]["parameters"]["header_test1"]["name"].setValue( "header:test1B" ) + script["outputs"]["outputs"][0]["parameters"]["header_test2"]["value"].setValue( "edited" ) + + self.uiThreadCallHandler.waitFor( 1 ) + + self.assertEqual( len( script["catalogue"]["images"] ), 1 ) + self.assertNotIn( "test1", script["catalogue"]["out"].metadata() ) + self.assertEqual( script["catalogue"]["out"].metadata()["test1B"], IECore.StringData( "hello" ) ) + self.assertEqual( script["catalogue"]["out"].metadata()["test2"], IECore.StringData( "edited" ) ) + + ## \todo Promote to InteractiveRenderTest and check it works for other renderer backends. + def testEditOutputType( self ) : + + script = Gaffer.ScriptNode() + script["catalogue"] = GafferImage.Catalogue() + + script["sphere"] = GafferScene.Sphere() + + script["outputs"] = GafferScene.Outputs() + script["outputs"]["in"].setInput( script["sphere"]["out"] ) + + script["outputs"].addOutput( + "beauty", + IECoreScene.Output( + "test", + "ieDisplay", + "rgba", + { + "driverType" : "ClientDisplayDriver", + "displayHost" : "localhost", + "displayPort" : str( script["catalogue"].displayDriverServer().portNumber() ), + "remoteDisplayType" : "GafferImage::GafferDisplayDriver", + } + ) + ) + + script["renderer"] = self._createInteractiveRender() + script["renderer"]["in"].setInput( script["outputs"]["out"] ) + + # Start a render, give it time to finish, and check we have an image. + + script["renderer"]["state"].setValue( script["renderer"].State.Running ) + self.uiThreadCallHandler.waitFor( 1 ) + + self.assertEqual( len( script["catalogue"]["images"] ), 1 ) + self.assertEqual( script["catalogue"]["out"].metadata()["gaffer:isRendering"], IECore.BoolData( True ) ) + + # Modify the output to render to file instead of the catalogue, and check + # the catalogue image is closed and the file is created. + + with Gaffer.DirtyPropagationScope() : + + script["outputs"]["outputs"][0]["fileName"].setValue( self.temporaryDirectory() / "test.exr" ) + script["outputs"]["outputs"][0]["type"].setValue( "exr" ) + + self.uiThreadCallHandler.waitFor( 1 ) + + self.assertEqual( len( script["catalogue"]["images"] ), 1 ) + self.assertNotIn( "gaffer:isRendering", script["catalogue"]["out"].metadata() ) + self.assertTrue( ( self.temporaryDirectory() / "test.exr" ).is_file() ) + + ## \todo Promote to InteractiveRenderTest and check it works for other renderer backends. + def testEditOutputFilterType( self ) : + + script = Gaffer.ScriptNode() + script["catalogue"] = GafferImage.Catalogue() + + script["sphere"] = GafferScene.Sphere() + + script["outputs"] = GafferScene.Outputs() + script["outputs"]["in"].setInput( script["sphere"]["out"] ) + + script["outputs"].addOutput( + "beauty", + IECoreScene.Output( + "test", + "ieDisplay", + "rgba", + { + "driverType" : "ClientDisplayDriver", + "displayHost" : "localhost", + "displayPort" : str( script["catalogue"].displayDriverServer().portNumber() ), + "remoteDisplayType" : "GafferImage::GafferDisplayDriver", + "filter" : "gaussian" + } + ) + ) + + script["renderer"] = self._createInteractiveRender() + script["renderer"]["in"].setInput( script["outputs"]["out"] ) + + # Start a render, give it time to finish, and check we have an image. + + script["renderer"]["state"].setValue( script["renderer"].State.Running ) + self.uiThreadCallHandler.waitFor( 1 ) + + self.assertEqual( len( script["catalogue"]["images"] ), 1 ) + self.assertEqual( script["catalogue"]["out"].metadata()["gaffer:isRendering"], IECore.BoolData( True ) ) + + self.assertAlmostEqual( + self._color4fAtUV( script["catalogue"], imath.V2f( 0.5 ) ).r, + 1, + delta = 0.01 + ) + + # Modify the output to use a different filter, check we're still + # rendering to the same image in the catalogue, and that the image + # has been affected by the filter. + + script["outputs"]["outputs"][0]["parameters"]["filter"]["value"].setValue( "variance" ) + + self.uiThreadCallHandler.waitFor( 1 ) + + self.assertEqual( len( script["catalogue"]["images"] ), 1 ) + self.assertEqual( script["catalogue"]["out"].metadata()["gaffer:isRendering"], IECore.BoolData( True ) ) + + self.assertAlmostEqual( + self._color4fAtUV( script["catalogue"], imath.V2f( 0.5 ) ).r, + 0, + delta = 0.01 + ) + def _createConstantShader( self ) : shader = GafferArnold.ArnoldShader() diff --git a/src/IECoreArnold/Renderer.cpp b/src/IECoreArnold/Renderer.cpp index 14dd93b73f9..fcea99b8c87 100644 --- a/src/IECoreArnold/Renderer.cpp +++ b/src/IECoreArnold/Renderer.cpp @@ -471,8 +471,14 @@ class ArnoldOutput : public IECore::RefCounted public : ArnoldOutput( AtUniverse *universe, const IECore::InternedString &name, const IECoreScene::Output *output, NodeDeleter nodeDeleter ) + : m_universe( universe ), m_name( name ), m_nodeDeleter( nodeDeleter ) { - // Create a driver node and set its parameters. + update( output ); + } + + void update( const IECoreScene::Output *output ) + { + // Create a driver node, or reuse the existing one if we can. AtString driverNodeType( output->getType().c_str() ); if( AiNodeEntryGetType( AiNodeEntryLookUp( driverNodeType ) ) != AI_NODE_DRIVER ) @@ -486,16 +492,28 @@ class ArnoldOutput : public IECore::RefCounted } } - const std::string driverNodeName = fmt::format( "ieCoreArnold:display:{}", name.string() ); - m_driver.reset( - AiNode( universe, driverNodeType, AtString( driverNodeName.c_str() ) ), - nodeDeleter - ); - if( !m_driver ) + if( m_driver && AiNodeEntryGetNameAtString( AiNodeGetNodeEntry( m_driver.get() ) ) == driverNodeType ) { - throw IECore::Exception( fmt::format( "Unable to create output driver of type \"{}\"", driverNodeType.c_str() ) ); + // Reuse + AiNodeReset( m_driver.get() ); + } + else + { + // Create + m_driver = nullptr; // Delete old driver so name doesn't clash with new driver + const std::string driverNodeName = fmt::format( "ieCoreArnold:display:{}", m_name.string() ); + m_driver.reset( + AiNode( m_universe, driverNodeType, AtString( driverNodeName.c_str() ) ), + m_nodeDeleter + ); + if( !m_driver ) + { + throw IECore::Exception( fmt::format( "Unable to create output driver of type \"{}\"", driverNodeType.c_str() ) ); + } } + // Set driver parameters. + if( const AtParamEntry *fileNameParameter = AiNodeEntryLookUpParameter( AiNodeGetNodeEntry( m_driver.get() ), g_fileNameArnoldString ) ) { AiNodeSetStr( m_driver.get(), AiParamGetName( fileNameParameter ), AtString( output->getName().c_str() ) ); @@ -511,6 +529,7 @@ class ArnoldOutput : public IECore::RefCounted customAttributesData = new IECore::StringVectorData(); } + m_cameraOverride = ""; std::vector &customAttributes = customAttributesData->writable(); for( IECore::CompoundDataMap::const_iterator it = output->parameters().begin(), eIt = output->parameters().end(); it != eIt; ++it ) { @@ -545,7 +564,7 @@ class ArnoldOutput : public IECore::RefCounted ParameterAlgo::setParameter( m_driver.get(), "custom_attributes", customAttributesData.get() ); } - // Create a filter. + // Create a filter node, or reuse an existing one if we can. std::string filterNodeType = parameter( output->parameters(), "filter", "gaussian" ); if( AiNodeEntryGetType( AiNodeEntryLookUp( AtString( filterNodeType.c_str() ) ) ) != AI_NODE_FILTER ) @@ -553,15 +572,27 @@ class ArnoldOutput : public IECore::RefCounted filterNodeType = filterNodeType + "_filter"; } - const std::string filterNodeName = fmt::format( "ieCoreArnold:filter:{}", name.string() ); - m_filter.reset( - AiNode( universe, AtString( filterNodeType.c_str() ), AtString( filterNodeName.c_str() ) ), - nodeDeleter - ); - if( AiNodeEntryGetType( AiNodeGetNodeEntry( m_filter.get() ) ) != AI_NODE_FILTER ) + if( m_filter && AiNodeEntryGetName( AiNodeGetNodeEntry( m_filter.get() ) ) == filterNodeType ) { - throw IECore::Exception( fmt::format( "Unable to create filter of type \"{}\"", filterNodeType ) ); + // Reuse + AiNodeReset( m_filter.get() ); } + else + { + // Create + m_filter = nullptr; // Delete old filter so name doesn't clash with new filter + const std::string filterNodeName = fmt::format( "ieCoreArnold:filter:{}", m_name.string() ); + m_filter.reset( + AiNode( m_universe, AtString( filterNodeType.c_str() ), AtString( filterNodeName.c_str() ) ), + m_nodeDeleter + ); + if( AiNodeEntryGetType( AiNodeGetNodeEntry( m_filter.get() ) ) != AI_NODE_FILTER ) + { + throw IECore::Exception( fmt::format( "Unable to create filter of type \"{}\"", filterNodeType ) ); + } + } + + // Set filter parameters. for( IECore::CompoundDataMap::const_iterator it = output->parameters().begin(), eIt = output->parameters().end(); it != eIt; ++it ) { @@ -632,7 +663,7 @@ class ArnoldOutput : public IECore::RefCounted } else if( tokens[0] == "lpe" ) { - m_lpeName = m_layerName.size() ? m_layerName : "ieCoreArnold:lpe:" + name.string(); + m_lpeName = m_layerName.size() ? m_layerName : "ieCoreArnold:lpe:" + m_name.string(); m_lpeValue = tokens[1]; m_data = m_lpeName; m_type = colorType; @@ -652,7 +683,7 @@ class ArnoldOutput : public IECore::RefCounted /// to use the standard Cortex formatting instead. IECore::msg( IECore::Msg::Warning, "ArnoldRenderer", - fmt::format( "Unknown data type \"{}\" for output \"{}\"", tokens[0], name.string() ) + fmt::format( "Unknown data type \"{}\" for output \"{}\"", tokens[0], m_name.string() ) ); m_data = tokens[0]; m_type = tokens[1]; @@ -663,7 +694,7 @@ class ArnoldOutput : public IECore::RefCounted /// \todo See above. IECore::msg( IECore::Msg::Warning, "ArnoldRenderer", - fmt::format( "Unknown data specification \"{}\" for output \"{}\"", output->getData(), name.string() ) + fmt::format( "Unknown data specification \"{}\" for output \"{}\"", output->getData(), m_name.string() ) ); m_data = output->getData(); m_type = ""; @@ -687,7 +718,7 @@ class ArnoldOutput : public IECore::RefCounted if( m_lpeName.empty() ) { IECore::msg( IECore::Msg::Warning, "ArnoldRenderer", - fmt::format( "Cannot use `layerName` with `layerPerLightGroup` for non-LPE output \"{}\", due to Arnold bug #12282", name.string() ) + fmt::format( "Cannot use `layerName` with `layerPerLightGroup` for non-LPE output \"{}\", due to Arnold bug #12282", m_name.string() ) ); } else @@ -740,6 +771,9 @@ class ArnoldOutput : public IECore::RefCounted private : + AtUniverse *m_universe; + const IECore::InternedString m_name; + NodeDeleter m_nodeDeleter; SharedAtNodePtr m_driver; SharedAtNodePtr m_filter; std::string m_data; @@ -3648,21 +3682,30 @@ class ArnoldGlobals void output( const IECore::InternedString &name, const IECoreScene::Output *output ) { - m_outputs.erase( name ); - if( output ) + if( !output ) + { + m_outputs.erase( name ); + return; + } + + try { - try + ArnoldOutputPtr &arnoldOutput = m_outputs[name]; + if( arnoldOutput ) { - ArnoldOutputPtr o = new ArnoldOutput( m_universeBlock->universe(), name, output, nodeDeleter( m_renderType ) ); - o->updateImager( m_imager ? m_imager->root() : nullptr ); - m_outputs[name] = o; + arnoldOutput->update( output ); } - catch( const std::exception &e ) + else { - IECore::msg( IECore::Msg::Warning, "IECoreArnold::Renderer::output", e.what() ); + arnoldOutput = new ArnoldOutput( m_universeBlock->universe(), name, output, nodeDeleter( m_renderType ) ); } + arnoldOutput->updateImager( m_imager ? m_imager->root() : nullptr ); + } + catch( const std::exception &e ) + { + IECore::msg( IECore::Msg::Warning, "IECoreArnold::Renderer::output", e.what() ); + m_outputs.erase( name ); } - } // Some of Arnold's globals come from camera parameters, so the From bfa74e6520d7532dd3384290ad95d57b6e0aab83 Mon Sep 17 00:00:00 2001 From: ivanimanishi Date: Thu, 16 Nov 2023 09:01:38 -0800 Subject: [PATCH 08/10] Instancer : Fix ambiguous reference compilation errors When building for nuke 13's environment, which uses boost 1.70, we started getting compiler errors `reference to '_1' is ambiguous`. --- Changes.md | 5 +++++ src/GafferScene/Instancer.cpp | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/Changes.md b/Changes.md index 9891596242b..19d443210ef 100644 --- a/Changes.md +++ b/Changes.md @@ -17,6 +17,11 @@ API - SceneGadget : Added `snapshotToFile()` method. +Build +----- + +- Instancer : Fixed ambiguous reference compilation errors when building with Boost 1.70. + 1.3.7.0 (relative to 1.3.6.1) ======= diff --git a/src/GafferScene/Instancer.cpp b/src/GafferScene/Instancer.cpp index 0303464cb1e..e637e93a6e2 100644 --- a/src/GafferScene/Instancer.cpp +++ b/src/GafferScene/Instancer.cpp @@ -756,13 +756,13 @@ class Instancer::EngineData : public Data template AttributeCreator operator()( const TypedData> *data ) { - return std::bind( &createAttribute, data->readable(), ::_1 ); + return std::bind( &createAttribute, data->readable(), std::placeholders::_1 ); } template AttributeCreator operator()( const GeometricTypedData> *data ) { - return std::bind( &createGeometricAttribute, data->readable(), data->getInterpretation(), ::_1 ); + return std::bind( &createGeometricAttribute, data->readable(), data->getInterpretation(), std::placeholders::_1 ); } AttributeCreator operator()( const Data *data ) From 5f24ab003a87b7c7cc4d9e0f90242d2fd2ef6827 Mon Sep 17 00:00:00 2001 From: Daniel Dresser Date: Wed, 15 Nov 2023 15:40:06 -0800 Subject: [PATCH 09/10] RendererAlgo : Use approximate hash for Procedural with deformation on This fixes a bug whereby we would send render updates for all capsules any time another object was deformed. --- Changes.md | 1 + .../GafferSceneTest/RenderControllerTest.py | 142 ++++++++++++++++++ src/GafferScene/RendererAlgo.cpp | 28 ++-- 3 files changed, 155 insertions(+), 16 deletions(-) diff --git a/Changes.md b/Changes.md index 19d443210ef..1706ffbae79 100644 --- a/Changes.md +++ b/Changes.md @@ -9,6 +9,7 @@ Features Fixes ----- +- InteractiveRender : Fixed unnecessary updates to encapsulated locations when deforming an unrelated object. - InteractiveArnoldRender : Fixed creation of new Catalogue image when editing output metadata or pixel filter. - Windows `Scene/OpenGL/Shader` Menu : Removed `\` at the beginning of menu items. diff --git a/python/GafferSceneTest/RenderControllerTest.py b/python/GafferSceneTest/RenderControllerTest.py index 55e8329c512..c3dc1687600 100644 --- a/python/GafferSceneTest/RenderControllerTest.py +++ b/python/GafferSceneTest/RenderControllerTest.py @@ -330,6 +330,148 @@ def testLightLinkPerformance( self ) : links = renderer.capturedObject( "/group/spheres/instances/sphere/0" ).capturedLinks( "lights" ) self.assertEqual( len( links ), numLights ) + @GafferTest.TestRunner.PerformanceTestMethod() + def testCapsuleDeformPerformance( self ) : + + # Test the performance of doing an edit to one thing in a scene which contains many Capsules + # with deformation blur turned on. This stresses our mechanism for determining that the Capsules + # haven't changed. + + sphere = GafferScene.Sphere() + + sphereFilter = GafferScene.PathFilter() + sphereFilter["paths"].setValue( IECore.StringVectorData( [ '/sphere' ] ) ) + + encapsulate = GafferScene.Encapsulate() + encapsulate["in"].setInput( sphere["out"] ) + encapsulate["filter"].setInput( sphereFilter["out"] ) + + duplicate = GafferScene.Duplicate() + duplicate["in"].setInput( encapsulate["out"] ) + duplicate["filter"].setInput( sphereFilter["out"] ) + duplicate["copies"].setValue( 50000 ) + duplicate["transform"]["translate"].setValue( imath.V3f( 1.5, 0, 0 ) ) + duplicate["transform"]["rotate"].setValue( imath.V3f( 0.03, 2.4, 0.8 ) ) + + editedSphere = GafferScene.Sphere() + editedSphere["radius"].setValue( 8 ) + + group = GafferScene.Group() + group["in"][0].setInput( duplicate["out"] ) + group["in"][1].setInput( editedSphere["out"] ) + + standardOptions = GafferScene.StandardOptions() + standardOptions["in"].setInput( group["out"] ) + standardOptions["options"]["deformationBlur"]["value"].setValue( True ) + standardOptions["options"]["deformationBlur"]["enabled"].setValue( True ) + + renderer = GafferScene.Private.IECoreScenePreview.CapturingRenderer() + controller = GafferScene.RenderController( standardOptions["out"], Gaffer.Context(), renderer ) + controller.setMinimumExpansionDepth( 10 ) + + controller.update() + + # Modify a simple object not related to the capsule - this shouldn't trigger a regenerate + editedSphere["divisions"].setValue( imath.V2i( 6 ) ) + + with GafferTest.TestRunner.PerformanceScope() : + controller.update() + + def testCapsuleNoBogusRegenerate( self ) : + + sphere = GafferScene.Sphere() + + sphereFilter = GafferScene.PathFilter() + sphereFilter["paths"].setValue( IECore.StringVectorData( [ '/sphere' ] ) ) + + encapsulate = GafferScene.Encapsulate() + encapsulate["in"].setInput( sphere["out"] ) + encapsulate["filter"].setInput( sphereFilter["out"] ) + + editedSphere = GafferScene.Sphere() + editedSphere["name"].setValue( "editedSphere" ) + editedSphere["radius"].setValue( 8 ) + + group = GafferScene.Group() + group["in"][0].setInput( encapsulate["out"] ) + group["in"][1].setInput( editedSphere["out"] ) + + standardOptions = GafferScene.StandardOptions() + standardOptions["in"].setInput( group["out"] ) + standardOptions["options"]["deformationBlur"]["value"].setValue( True ) + standardOptions["options"]["deformationBlur"]["enabled"].setValue( True ) + + renderer = GafferScene.Private.IECoreScenePreview.CapturingRenderer() + controller = GafferScene.RenderController( standardOptions["out"], Gaffer.Context(), renderer ) + controller.setMinimumExpansionDepth( 10 ) + + controller.update() + + original = renderer.capturedObject( "/group/sphere" ) + + editedSphere["divisions"].setValue( imath.V2i( 6 ) ) + controller.update() + + # Editing the sphere shouldn't cause the capsule to be regenerated + self.assertTrue( renderer.capturedObject( "/group/sphere" ).isSame( original ) ) + + @unittest.expectedFailure + def testCapsuleModifyOnFrameNotShutter( self ) : + + # Test a weird corner case where none of the shutter samples contributing to a Capsule have changed, + # but the capsule actually has changed - because it is sampled on frame, and the on-frame data is + # different even though none of the shutter samples have changed. + # + # This is currently an expected failure because we consider it worthwhile to use the same mechanism + # as other objects for caching things that don't support deformation blur - even though a Capsule + # doesn't support deformation blur, we still consider it to have not changed if the hashes of the + # deformation samples haven't changed. It introduces a minor inconsistency, but no other solution + # offers the same level of performance for Capsules that haven't changed without introducing more + # complex code + + rootFilter = GafferScene.PathFilter() + rootFilter["paths"].setValue( IECore.StringVectorData( [ '/sphere' ] ) ) + + sphere = GafferScene.Sphere() + + sphereEncapsulate = GafferScene.Encapsulate() + sphereEncapsulate["in"].setInput( sphere["out"] ) + sphereEncapsulate["filter"].setInput( rootFilter["out"] ) + + cube = GafferScene.Cube() + cube["name"].setValue( "sphere" ) + + cubeEncapsulate = GafferScene.Encapsulate() + cubeEncapsulate["in"].setInput( cube["out"] ) + cubeEncapsulate["filter"].setInput( rootFilter["out"] ) + + switch = Gaffer.Switch() + switch.setup( sphereEncapsulate["out"] ) + switch["in"][0].setInput( sphereEncapsulate["out"] ) + switch["in"][1].setInput( cubeEncapsulate["out"] ) + + standardOptions = GafferScene.StandardOptions() + standardOptions["in"].setInput( switch["out"] ) + standardOptions["options"]["deformationBlur"]["value"].setValue( True ) + standardOptions["options"]["deformationBlur"]["enabled"].setValue( True ) + + renderer = GafferScene.Private.IECoreScenePreview.CapturingRenderer() + controller = GafferScene.RenderController( standardOptions["out"], Gaffer.Context(), renderer ) + controller.setMinimumExpansionDepth( 10 ) + + controller.update() + self.assertEqual( renderer.capturedObject( "/sphere" ).capturedSamples()[0].scene(), sphere["out"] ) + + # We now modify the capsule on frame 1, but crucially, NOT at time 0.75 or 1.25, which are + # the times that get sampled by the shutter. This tests a corner case where a capsule is incorrectly + # cached ( because it wouldn't have changed if it was a usual object that is sampled by the shutter ) + + switch["expression"] = Gaffer.Expression() + switch["expression"].setExpression( 'parent["index"] = context.getFrame() == 1.0', "python" ) + + controller.update() + self.assertEqual( renderer.capturedObject( "/sphere" ).capturedSamples()[0].scene(), cube["out"] ) + def testHideLinkedLight( self ) : # One default light and one non-default light, which will diff --git a/src/GafferScene/RendererAlgo.cpp b/src/GafferScene/RendererAlgo.cpp index 3d3dd733d9b..44017f7ac85 100644 --- a/src/GafferScene/RendererAlgo.cpp +++ b/src/GafferScene/RendererAlgo.cpp @@ -442,25 +442,21 @@ bool objectSamples( const ObjectPlug *objectPlug, const std::vector &samp Context::Scope frameScope( frameContext ); std::vector tempTimes = {}; - // \todo - this is quite bad for the case of any Capsules, which use a naive hash - // that always varies with the context. This should be investigated soon as a follow - // up. - // - // This is a pretty weird case - we would have taken an earlier branch if the hashes - // had all matched, so it looks like this object is actual animated, despite not supporting - // animation. - // The most correct thing to do here is reset the hash, since we may not have included the - // on frame in the samples we hashed, and in theory, the on frame value could vary independently - // of shutter open and close. This means that an animated non-animateable object will never have - // a matching hash, and will be updated every pass. May be a performance hazard, but probably - // preferable to incorrect behaviour? Just means people need to be careful to make sure their - // heavy crowd procedurals don't have a hash that changes during the shutter? - // ( I guess in theory we could check if the on frame time is in sampleTimes, but I don't want to - // add any more special cases to this weird corner ). + // Use the hash from the shutter samples. This is technically incorrect, since we are going + // to evaluate the object on-frame, and the on-frame sample may not have been included in the + // sample times - but it does mean the hash will match if we are called again without the object + // changing. We're seeing a 2X speedup from this, so we've decided it's worthwhile. // + // The user facing inconsistency is that we should update whenever the on-frame data + // we are using changes, but if the user changes the on-frame result, while keeping the data + // the same at the shutter samples ( using an odd number of segments with a centered shutter, + // so the shutter samples don't include the on-frame time ), then we would fail to update + // until the render is restarted. It seems quite unlikely a user will ever actually do this + // ( in any normal operation, when you change something, you change it for a frame or more + // at a time ) if( hash ) { - *hash = IECore::MurmurHash(); + *hash = combinedHash; } return objectSamples( objectPlug, tempTimes, samples ); From 217f1ccb35d25d8381576de51e36414d677634bf Mon Sep 17 00:00:00 2001 From: Daniel Dresser Date: Fri, 8 Sep 2023 18:01:10 -0700 Subject: [PATCH 10/10] Resample : Optimize inseparable case with no scaling --- Changes.md | 5 ++ python/GafferImageTest/ResampleTest.py | 21 +++++ src/GafferImage/Resample.cpp | 115 +++++++++++++++++++++++-- 3 files changed, 133 insertions(+), 8 deletions(-) diff --git a/Changes.md b/Changes.md index 1706ffbae79..62a4e027123 100644 --- a/Changes.md +++ b/Changes.md @@ -6,6 +6,11 @@ Features - Viewer : Added "Snapshot To Catalogue" command to the right-click menu of the 3D view. +Improvements +------------ + +- ImageTransform, Resample : Improved performance for non-separable filters without scaling, with 2-6x speedups in some benchmark cases. + Fixes ----- diff --git a/python/GafferImageTest/ResampleTest.py b/python/GafferImageTest/ResampleTest.py index c8d9eedb5f7..1701602e97a 100644 --- a/python/GafferImageTest/ResampleTest.py +++ b/python/GafferImageTest/ResampleTest.py @@ -154,6 +154,27 @@ def __test( fileName, size, filter ) : with self.subTest( fileName = args[0], size = args[1], ftilter = args[2] ): __test( *args ) + def testInseparableFastPath( self ) : + + reader = GafferImage.ImageReader() + reader["fileName"].setValue( self.imagesPath() / "resamplePatterns.exr" ) + + # When applying an inseparable filter with no scaling, we can use a much faster code path. + # This code path should not have any effect on the result + resampleFastPath = GafferImage.Resample() + resampleFastPath["in"].setInput( reader["out"] ) + resampleFastPath['filterScale'].setValue( imath.V2f( 4 ) ) + resampleFastPath["filter"].setValue( "radial-lanczos3" ) + + # Force the slow code path using the "debug" parameter + resampleReference = GafferImage.Resample() + resampleReference["in"].setInput( reader["out"] ) + resampleReference['filterScale'].setValue( imath.V2f( 4 ) ) + resampleReference["filter"].setValue( "radial-lanczos3" ) + resampleReference["debug"].setValue( GafferImage.Resample.Debug.SinglePass ) + + self.assertImagesEqual( resampleFastPath["out"], resampleReference["out"] ) + def testSincUpsize( self ) : c = GafferImage.Constant() diff --git a/src/GafferImage/Resample.cpp b/src/GafferImage/Resample.cpp index 567602a66c2..ace0e3acddc 100644 --- a/src/GafferImage/Resample.cpp +++ b/src/GafferImage/Resample.cpp @@ -64,10 +64,14 @@ enum Passes { Horizontal = 1, Vertical = 2, - Both = Horizontal | Vertical + Both = Horizontal | Vertical, + + // Special pass label when we must compute both passes in one, but there is no scaling. + // This allows a special code path which is up to 6X faster. + BothOptimized = Both | 4 }; -unsigned requiredPasses( const Resample *resample, const ImagePlug *image, const OIIO::Filter2D *filter ) +unsigned requiredPasses( const Resample *resample, const ImagePlug *image, const OIIO::Filter2D *filter, V2f &ratio ) { int debug = resample->debugPlug()->getValue(); if( debug == Resample::HorizontalPass ) @@ -76,12 +80,24 @@ unsigned requiredPasses( const Resample *resample, const ImagePlug *image, const } else if( debug == Resample::SinglePass ) { + // For a SinglePass debug mode, we always use Both. + // Note that we don't use the optimized pass here, even if the ratio is 1 - we want debug to always + // use the same path. return Horizontal | Vertical; } if( image == image->parent()->outPlug() ) { - return filter->separable() ? Vertical : Both; + if( filter->separable() ) + { + return Vertical; + } + else + { + // The filter isn't separable, so we must process everything at once. If the ratio has no + // scaling though, we can use the optimized path. + return ( ratio == V2f( 1.0 ) ) ? BothOptimized : Both; + } } return Horizontal; } @@ -193,7 +209,7 @@ const OIIO::Filter2D *filterAndScale( const std::string &name, V2f ratio, V2f &i /// only computed once and then reused. At the time of writing, profiles indicate that /// accessing pixels via the Sampler is the main bottleneck, but once that is optimised /// perhaps cached filter weights could have a benefit. -void filterWeights( const OIIO::Filter2D *filter, const float inputFilterScale, const float filterRadius, const int x, const float ratio, const float offset, Passes pass, std::vector &supportRanges, std::vector &weights ) +void filterWeights1D( const OIIO::Filter2D *filter, const float inputFilterScale, const float filterRadius, const int x, const float ratio, const float offset, Passes pass, std::vector &supportRanges, std::vector &weights ) { weights.reserve( ( 2 * ceilf( filterRadius ) + 1 ) * ImagePlug::tileSize() ); supportRanges.reserve( 2 * ImagePlug::tileSize() ); @@ -221,6 +237,42 @@ void filterWeights( const OIIO::Filter2D *filter, const float inputFilterScale, } } +// For the inseparable case, we can't always reuse the weights for an adjacent row or column. +// There are a lot of possible scaling factors where the ratio can be represented as a fraction, +// and the weights needed would repeat after a certain number of pixels, and we could compute weights +// for a limited section of pixels, and reuse them in a tiling way. +// That's a bit complicated though, so we're just handling the simplest case currently ( since it is +// a common case ): +// if there is no scaling, then we only need to compute the weights for one pixel, and we can reuse them +// for all pixels. This means we don't loop over output pixels at all here - we just compute the weights +// for one output pixel, and return one 2D support for this pixel - it just gets shifted for each adjacent +// pixel. +void filterWeights2D( const OIIO::Filter2D *filter, const V2f inputFilterScale, const V2f filterRadius, const V2i p, const V2f offset, Box2i &support, std::vector &weights ) +{ + weights.reserve( ( 2 * ceilf( filterRadius.x ) + 1 ) * ( 2 * ceilf( filterRadius.y ) + 1 ) ); + + const V2f filterCoordinateMult( 1.0f / inputFilterScale.x, 1.0f / inputFilterScale.y ); + + // input pixel position (floating point) + V2f i = V2f( p ) + V2f( 0.5 ) + offset; + + support = Box2i( + V2i( ceilf( i.x - 0.5f - filterRadius.x ), ceilf( i.y - 0.5f - filterRadius.y ) ), + V2i( floorf( i.x + 0.5f + filterRadius.x ), floorf( i.y + 0.5f + filterRadius.y ) ) + ); + + for( int fY = support.min.y; fY < support.max.y; ++fY ) + { + const float fy = filterCoordinateMult.y * ( float( fY ) + 0.5 - i.y ); + for( int fX = support.min.x; fX < support.max.x; ++fX ) + { + const float fx = filterCoordinateMult.x * ( float( fX ) + 0.5f - i.x ); + const float w = (*filter)( fx, fy ); + weights.push_back( w ); + } + } +} + Box2f transform( const Box2f &b, const M33f &m ) { if( b.isEmpty() ) @@ -477,7 +529,7 @@ void Resample::hashChannelData( const GafferImage::ImagePlug *parent, const Gaff filterPlug()->hash( h ); - const unsigned passes = requiredPasses( this, parent, filter ); + const unsigned passes = requiredPasses( this, parent, filter, ratio ); if( passes & Horizontal ) { h.append( inputFilterScale.x ); @@ -491,6 +543,12 @@ void Resample::hashChannelData( const GafferImage::ImagePlug *parent, const Gaff h.append( offset.y ); } + if( passes == BothOptimized ) + { + // Append an extra flag so our hash reflects that we are going to take the optimized path + h.append( true ); + } + const V2i tileOrigin = context->get( ImagePlug::tileOriginContextName ); Sampler sampler( passes == Vertical ? horizontalPassPlug() : inPlug(), @@ -518,7 +576,7 @@ IECore::ConstFloatVectorDataPtr Resample::computeChannelData( const std::string const OIIO::Filter2D *filter = filterAndScale( filterPlug()->getValue(), ratio, inputFilterScale ); inputFilterScale *= filterScalePlug()->getValue(); - const unsigned passes = requiredPasses( this, parent, filter ); + const unsigned passes = requiredPasses( this, parent, filter, ratio ); Sampler sampler( passes == Vertical ? horizontalPassPlug() : inPlug(), @@ -588,6 +646,47 @@ IECore::ConstFloatVectorDataPtr Resample::computeChannelData( const std::string } } } + else if( passes == BothOptimized ) + { + Box2i support; + std::vector weights; + filterWeights2D( filter, inputFilterScale, filterRadius, tileBound.min, offset, support, weights ); + + V2i oP; // output pixel position + V2i supportOffset; + for( oP.y = tileBound.min.y; oP.y < tileBound.max.y; ++oP.y ) + { + supportOffset.y = oP.y - tileBound.min.y; + + for( oP.x = tileBound.min.x; oP.x < tileBound.max.x; ++oP.x ) + { + Canceller::check( context->canceller() ); + + supportOffset.x = oP.x - tileBound.min.x; + std::vector::const_iterator wIt = weights.begin(); + + float v = 0.0f; + float totalW = 0.0f; + sampler.visitPixels( + Imath::Box2i( support.min + supportOffset, support.max + supportOffset ), + [&wIt, &v, &totalW]( float cur, int x, int y ) + { + const float w = *wIt++; + v += w * cur; + totalW += w; + } + ); + + if( totalW != 0.0f ) + { + *pIt = v / totalW; + } + + ++pIt; + } + } + + } else if( passes == Horizontal ) { // When the filter is separable we can perform filtering in two @@ -600,7 +699,7 @@ IECore::ConstFloatVectorDataPtr Resample::computeChannelData( const std::string // we precompute the weights now to avoid repeating work later. std::vector supportRanges; std::vector weights; - filterWeights( filter, inputFilterScale.x, filterRadius.x, tileBound.min.x, ratio.x, offset.x, Horizontal, supportRanges, weights ); + filterWeights1D( filter, inputFilterScale.x, filterRadius.x, tileBound.min.x, ratio.x, offset.x, Horizontal, supportRanges, weights ); V2i oP; // output pixel position @@ -646,7 +745,7 @@ IECore::ConstFloatVectorDataPtr Resample::computeChannelData( const std::string // we precompute the weights now to avoid repeating work later. std::vector supportRanges; std::vector weights; - filterWeights( filter, inputFilterScale.y, filterRadius.y, tileBound.min.y, ratio.y, offset.y, Vertical, supportRanges, weights ); + filterWeights1D( filter, inputFilterScale.y, filterRadius.y, tileBound.min.y, ratio.y, offset.y, Vertical, supportRanges, weights ); std::vector::const_iterator supportIt = supportRanges.begin(); std::vector::const_iterator rowWeightsIt = weights.begin();