Merge pull request #6768 from GafferHQ/renderManCropFixes

RenderMan crop window fixes

Author: murraystevenson

Changes.md

@@ -14,8 +14,19 @@ Fixes
 
 - NodeEditor : Fixed "Revert to Defaults" to handle ganged plugs, and other plugs where a subset of children have input connections. In this case, the subset without inputs now revert correctly to their default values.
 - ShaderTweaks : Fixed context handling in "From Affected" and "From Selected" menu items.
-- RenderMan : Fixed `R10043 {WARNING} inputMaterial, unknown or mismatched input parameter of PxrSurface`.
 - SceneTestCase, ImageTestCase : Sanitisers are no longer installed when testing performance, since they add additional overhead.
+- RenderMan :
+  - Fixed `R10043 {WARNING} inputMaterial, unknown or mismatched input parameter of PxrSurface`.
+  - Fixed offset when reducing crop window size in RIS (#6727).
+  - Fixed unwanted creation of new Catalogue images from InteractiveRenders in the following situations :
+    - Changing camera.
+    - Changing pixel filter or filter width.
+    - Enlarging the crop region when rendering with RIS.
+    - Changing the resolution.
+    - Adding or removing outputs.
+  - Added workaround for RenderMan interactive denoiser bugs :
+    - Data window not updating when the crop window is edited.
+    - Crashes when the crop window is edited.
 
 API
 ---

include/GafferScene/InteractiveRender.h

@@ -143,6 +143,9 @@ class GAFFERSCENE_API InteractiveRender : public Gaffer::ComputeNode
 		IE_CORE_FORWARDDECLARE( RenderMessageHandler )
 		RenderMessageHandlerPtr  m_messageHandler;
 
+		friend class Catalogue;
+		static bool renderIsActive( const std::string &renderId );
+
 		static size_t g_firstPlugIndex;
 
 };

include/GafferScene/Private/RendererAlgo.h

@@ -90,6 +90,8 @@ struct GAFFERSCENE_API RenderOptions
 /// Creates the directories necessary to receive the outputs defined in globals.
 GAFFERSCENE_API void createOutputDirectories( const IECore::CompoundObject *globals );
 
+// Returns the "gaffer:renderID" parameter added to outputs for the provided renderer.
+GAFFERSCENE_API std::string renderID( const IECoreScenePreview::Renderer *renderer );
 
 // Returns whether the globals contain an Output that references the data `float id`
 GAFFERSCENE_API bool hasIDOutput( const IECore::CompoundObject *globals );
@@ -100,7 +102,6 @@ GAFFERSCENE_API bool hasInstanceIDOutput( const IECore::CompoundObject *globals
 // Returns a manifest file path if it's set in the globals, otherwise empty string
 GAFFERSCENE_API std::string renderManifestFilePath( const IECore::CompoundObject *globals );
 
-
 /// Sets `times` to a list of times to sample the transform or deformation of a
 /// location at, based on the render options and location attributes. Returns `true`
 /// if `times` was altered and `false` if it was already set correctly.

python/GafferArnoldTest/InteractiveArnoldRenderTest.py

@@ -844,8 +844,9 @@ def testEditOutputType( self ) :
 		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.
+		# Modify the output to render to file instead of the catalogue. This isn't
+		# particularly likely in practice, and would most likely indicate user error
+		# of some sort. We expect the Catalogue to keep the image open.
 
 		with Gaffer.DirtyPropagationScope() :
 
@@ -855,9 +856,30 @@ def testEditOutputType( self ) :
 		self.uiThreadCallHandler.waitFor( 1 )
 
 		self.assertEqual( len( script["catalogue"]["images"] ), 1 )
-		self.assertNotIn( "gaffer:isRendering", script["catalogue"]["out"].metadata() )
+		self.assertEqual( script["catalogue"]["out"].metadata()["gaffer:isRendering"], IECore.BoolData( True ) )
+		self.assertTrue( ( self.temporaryDirectory() / "test.exr" ).is_file() )
+
+		# Switch back to rendering to the Catalogue, as if the user has fixed their error.
+
+		with Gaffer.DirtyPropagationScope() :
+
+			script["outputs"]["outputs"][0]["fileName"].setValue( "test" )
+			script["outputs"]["outputs"][0]["type"].setValue( "ieDisplay" )
+
+		self.uiThreadCallHandler.waitFor( 1 )
+
+		self.assertEqual( len( script["catalogue"]["images"] ), 1 )
+		self.assertEqual( script["catalogue"]["out"].metadata()["gaffer:isRendering"], IECore.BoolData( True ) )
 		self.assertTrue( ( self.temporaryDirectory() / "test.exr" ).is_file() )
 
+		# Stop the render, and check the image is closed.
+
+		script["renderer"]["state"].setValue( script["renderer"].State.Stopped )
+		self.uiThreadCallHandler.waitFor( 1 )
+
+		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 testEditOutputFilterType( self ) :
 

python/GafferRenderManTest/InteractiveRenderManRenderTest.py

@@ -50,19 +50,20 @@ class InteractiveRenderManRenderTest( GafferSceneTest.InteractiveRenderTest ) :
 
 	renderer = "RenderMan"
 
-	def testEditCropWindow( self ) :
-
-		if self.renderer == "RenderMan" :
-			# RIS doesn't reopen the display drivers when the crop
-			# window decreases in size, only when it increases. This will
-			# cause the base class test to fail, even though we are passing
-			# edits and RenderMan is only re-rendering the requested area.
-			## \todo Perhaps we can deal with this ourself by recreating
-			# the RenderView to force reopening? Or modifying a single camera
-			# instead of making a new one when the crop changes?
-			self.skipTest( "Crop window doesn't change data window" )
-		else :
-			GafferSceneTest.InteractiveRenderTest.testEditCropWindow( self )
+	def testEditCropWindowWithInteractiveDenoiser( self ) :
+
+		# See `_createOptions()`, where we use the test name to
+		# enable the interactive denoiser.
+		self.testEditCropWindow()
+
+	def _createOptions( self ) :
+
+		options = GafferRenderMan.RenderManOptions()
+		if "WithInteractiveDenoiser" in self.id() :
+			options["options"]["ri:interactiveDenoiser:enabled"]["enabled"].setValue( True )
+			options["options"]["ri:interactiveDenoiser:enabled"]["value"].setValue( True )
+
+		return options
 
 	def _createConstantShader( self ) :
 

python/GafferRenderManTest/RenderManRenderTest.py

@@ -91,7 +91,14 @@ def testRepeatedRender( self ) :
 		s["render"]["renderer"].setValue( self.renderer )
 		s["render"]["in"].setInput( s["outputs"]["out"] )
 
-		for i in range( 10 ) :
+		## \todo This used to do 10 renders, but has been adjusted to avoid failures
+		# later in CI. Bafflingly, what seems to be happening is that after a
+		# certain number of renders, RenderMan's EXR driver generates bogus
+		# channel names for a single render and then recovers. For CI to pass,
+		# that broken render needs to not be one where the bug breaks one of our
+		# assertions. We've reported this upstream, to hopefully we can revert
+		# to 10 renders one day.
+		for i in range( 5 ) :
 
 			s["render"]["task"].execute()
 

python/GafferSceneTest/CatalogueTest.py

@@ -1071,6 +1071,36 @@ def testYouOnlySaveOnce( self ) :
 			# made.
 			handler.assertDone()
 
+	def testUnfinishedRender( self ) :
+
+		# Send an image to a catalogue and "forget" to close the driver.
+		# The most likely cause of this would be all outputs accidentally being
+		# removed from an InteractiveRender. The image remains open (unsaved)
+		# in anticipation of the error being fixed.
+
+		script = Gaffer.ScriptNode()
+
+		script["catalogue"] = GafferScene.Catalogue()
+
+		script["constant"] = GafferImage.Constant()
+		script["constant"]["format"].setValue( GafferImage.Format( 100, 100 ) )
+		script["constant"]["color"].setValue( imath.Color4f( 1, 0, 0, 1 ) )
+
+		driver = self.sendImage( script["constant"]["out"], script["catalogue"], waitForSave = False, close = False )
+		self.assertEqual( len( script["catalogue"]["images"] ), 1 )
+		self.assertEqual( script["catalogue"]["out"]["metadata"].getValue()[ self.__catalogueIsRenderingMetadataKey ].value, True )
+
+		# Save and load while the image is still unsaved. We expect the image
+		# to not be loaded, because it isn't backed by an image on disk.
+
+		script2 = Gaffer.ScriptNode()
+		script2.execute( script.serialise() )
+		self.assertEqual( len( script2["catalogue"]["images"] ), 0 )
+
+		# Clean up.
+
+		driver.close()
+
 	def testReorder( self ) :
 
 		for newOrder in [

python/GafferSceneTest/InteractiveRenderTest.py

@@ -753,6 +753,7 @@ def testSwitchRenderCamera( self ) :
 
 		# `camera1` places the sphere in the centre of the image.
 
+		self.assertEqual( len( script["catalogue"]["images"] ), 1 )
 		self.assertAlmostEqual( self._color4fAtUV( script["catalogue"], imath.V2f( 0.5 ) ).a, 1, delta = 0.01 )
 		self.assertAlmostEqual( self._color4fAtUV( script["catalogue"], imath.V2f( 0.99, 0.5 ) ).a, 0, delta = 0.01 )
 
@@ -761,6 +762,7 @@ def testSwitchRenderCamera( self ) :
 		script["options"]["options"]["render:camera"]["value"].setValue( "/group/camera2" )
 		self.uiThreadCallHandler.waitFor( 1.0 )
 
+		self.assertEqual( len( script["catalogue"]["images"] ), 1 )
 		self.assertAlmostEqual( self._color4fAtUV( script["catalogue"], imath.V2f( 0.99, 0.5 ) ).a, 1, delta = 0.01 )
 		self.assertAlmostEqual( self._color4fAtUV( script["catalogue"], imath.V2f( 0.5 ) ).a, 0, delta = 0.01 )
 

src/GafferScene/Catalogue.cpp

@@ -417,6 +417,14 @@ class Catalogue::InternalImage : public ImageNode
 				}
 			}
 
+			if( InteractiveRender::renderIsActive( m_renderID ) )
+			{
+				// Render is still active. Assume drivers were just closed
+				// in preparation for creating new drivers (with a new
+				// data window or header data for example).
+				return;
+			}
+
 			// All our drivers have been closed, so the render has completed.
 			// Save the image to disk. We do this in the background because
 			// saving large images with many AOVs takes several seconds.

src/GafferScene/InteractiveRender.cpp

@@ -85,6 +85,12 @@ PendingUpdates &pendingUpdates()
 
 const InternedString g_rendererOptionName( "option:render:defaultRenderer" );
 
+unordered_set<string> &activeRenderIds()
+{
+	static unordered_set<string> g_activeRenderIds;
+	return g_activeRenderIds;
+}
+
 } // anon namespace
 
 // A thread-safe message handler for render messaging
@@ -362,6 +368,7 @@ void InteractiveRender::update()
 			"",
 			m_messageHandler.get()
 		);
+		activeRenderIds().insert( Private::RendererAlgo::renderID( m_renderer.get() ) );
 
 		m_controller.reset(
 			new RenderController( adaptedInPlug(), effectiveContext(), m_renderer )
@@ -413,6 +420,11 @@ Gaffer::ConstContextPtr InteractiveRender::effectiveContext()
 	}
 }
 
+bool InteractiveRender::renderIsActive( const std::string &renderId )
+{
+	return activeRenderIds().count( renderId );
+}
+
 void InteractiveRender::stop()
 {
 	// A Catalogue may need to access the render manifest as part of saving out the images from this
@@ -425,6 +437,8 @@ void InteractiveRender::stop()
 		m_lastRenderManifest = m_controller->renderManifest();
 	}
 
+	activeRenderIds().erase( Private::RendererAlgo::renderID( m_renderer.get() ) );
+
 	m_controller.reset();
 	m_renderer.reset();
 	m_state = Stopped;