Merge pull request #5547 from danieldresser-ie/capsuleHash

RendererAlgo : Experiments With How We Handle Capsules and Deformation Blur

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.
 

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

src/GafferScene/RendererAlgo.cpp

@@ -442,25 +442,21 @@ bool objectSamples( const ObjectPlug *objectPlug, const std::vector<float> &samp
 				Context::Scope frameScope( frameContext );
 				std::vector<float> 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 );