Merge pull request #6062 from johnhaddon/shaderViewFix

ShaderView : Don't crash if SceneCreator returns null

Changes.md

@@ -1,7 +1,10 @@
 1.4.x.x (relative to 1.4.14.0)
 =======
 
+Fixes
+-----
 
+- ShaderView : Fixed crash caused by a SceneCreator returning `None`.
 
 1.4.14.0 (relative to 1.4.13.0)
 ========

python/GafferSceneUITest/ShaderViewTest.py

@@ -212,5 +212,70 @@ def testCannotViewSceneSwitch( self ) :
 		self.assertFalse( view["in"].acceptsInput( sphere["out"] ) )
 		self.assertFalse( view["in"].acceptsInput( switch["out"] ) )
 
+	def testSceneCreatorReturningNone( self ) :
+
+		script = Gaffer.ScriptNode()
+		script["shader"] = GafferSceneTest.TestShader()
+		script["shader"]["type"].setValue( "test:surface" )
+		script["shader"]["name"].setValue( "test" )
+
+		GafferSceneUI.ShaderView.registerScene( "test", "Bad", lambda : None )
+		view = GafferUI.View.create( script["shader"]["out"] )
+		with IECore.CapturingMessageHandler() as mh :
+			view["scene"].setValue( "Bad" )
+
+		self.assertEqual( len( mh.messages ), 1 )
+		self.assertEqual( mh.messages[0].message, 'SceneCreator "Bad" returned null' )
+		self.assertIsNone( view.scene() )
+
+	def testSceneWithoutShaderPlug( self ) :
+
+		script = Gaffer.ScriptNode()
+		script["shader"] = GafferSceneTest.TestShader()
+		script["shader"]["type"].setValue( "test:surface" )
+		script["shader"]["name"].setValue( "test" )
+
+		GafferSceneUI.ShaderView.registerScene( "test", "NoShaderPlug", lambda : GafferScene.Plane() )
+		view = GafferUI.View.create( script["shader"]["out"] )
+		with IECore.CapturingMessageHandler() as mh :
+			view["scene"].setValue( "NoShaderPlug" )
+
+		self.assertEqual( len( mh.messages ), 1 )
+		self.assertEqual( mh.messages[0].message, 'Scene "NoShaderPlug" does not have a "shader" input plug' )
+
+	def testSceneWithoutOutPlug( self ) :
+
+		script = Gaffer.ScriptNode()
+		script["shader"] = GafferSceneTest.TestShader()
+		script["shader"]["type"].setValue( "test:surface" )
+		script["shader"]["name"].setValue( "test" )
+
+		def noOutPlug() :
+
+			node = Gaffer.Node()
+			node["shader"] = GafferScene.ShaderPlug()
+			return node
+
+		GafferSceneUI.ShaderView.registerScene( "test", "NoOutPlug", noOutPlug )
+		view = GafferUI.View.create( script["shader"]["out"] )
+		with IECore.CapturingMessageHandler() as mh :
+			view["scene"].setValue( "NoOutPlug" )
+
+		self.assertEqual( len( mh.messages ), 1 )
+		self.assertEqual( mh.messages[0].message, 'Scene "NoOutPlug" does not have an "out" output scene plug' )
+
+	def testIdleConnectionDoesntExtendLifetime( self ) :
+
+		script = Gaffer.ScriptNode()
+		script["shader"] = GafferSceneTest.TestShader()
+		script["shader"]["type"].setValue( "test:surface" )
+		script["shader"]["name"].setValue( "test" )
+
+		numSlots = GafferUI.Gadget.idleSignal().numSlots()
+		view = GafferUI.View.create( script["shader"]["out"] )
+		self.assertEqual( GafferUI.Gadget.idleSignal().numSlots(), numSlots + 1 )
+		del view
+		self.assertEqual( GafferUI.Gadget.idleSignal().numSlots(), numSlots )
+
 if __name__ == "__main__":
 	unittest.main()

src/GafferSceneUI/ShaderView.cpp

@@ -278,7 +278,9 @@ void ShaderView::plugDirtied( Gaffer::Plug *plug )
 		if( !m_idleConnection.connected() )
 		{
 			m_idleConnection = GafferUI::Gadget::idleSignal().connect(
-				boost::bind( &ShaderView::idleUpdate, ShaderViewPtr( this ) )
+				// OK to bind a raw pointer, because our destructor
+				// will disconnect `m_idleConnection`.
+				boost::bind( &ShaderView::idleUpdate, this )
 			);
 		}
 	}
@@ -370,14 +372,11 @@ void ShaderView::updateRendererState()
 void ShaderView::updateScene()
 {
 	PrefixAndName prefixAndName( shaderPrefix(), scenePlug()->getValue() );
-	if( m_scene && m_scenePrefixAndName == prefixAndName )
+	if( m_scenePrefixAndName == prefixAndName )
 	{
 		return;
 	}
 
-	m_scene = nullptr;
-	m_scenePrefixAndName = prefixAndName;
-
 	Scenes::const_iterator it = m_scenes.find( prefixAndName );
 	if( it != m_scenes.end() )
 	{
@@ -391,34 +390,58 @@ void ShaderView::updateScene()
 		SceneCreators::const_iterator it = sc.find( prefixAndName );
 		if( it == sc.end() )
 		{
-			it = sc.find( PrefixAndName( prefixAndName.first, "Default" ) );
+			IECore::msg(
+				IECore::Msg::Error, "ShaderView",
+				fmt::format( "SceneCreator \"{}\" not registered", prefixAndName.second )
+			);
+			m_scene = nullptr;
 		}
-
-		if( it == sc.end() )
+		else
 		{
-			sceneChangedSignal()( this );
-			return;
+			m_scene = it->second();
+			if( !m_scene )
+			{
+				IECore::msg(
+					IECore::Msg::Error, "ShaderView",
+					fmt::format( "SceneCreator \"{}\" returned null", prefixAndName.second )
+				);
+			}
 		}
-
-		m_scene = it->second();
 		m_scenes[prefixAndName] = m_scene;
 	}
 
-	Plug *shaderPlug = m_scene->getChild<Plug>( "shader" );
-	if( !shaderPlug || shaderPlug->direction() != Plug::In )
+	if( m_scene )
 	{
-		throw IECore::Exception( "Scene does not have a \"shader\" input plug" );
-	}
-
-	shaderPlug->setInput( m_imageConverter->getChild<Plug>( "in" ) );
+		Plug *shaderPlug = m_scene->getChild<Plug>( "shader" );
+		if( !shaderPlug || shaderPlug->direction() != Plug::In )
+		{
+			IECore::msg(
+				IECore::Msg::Error, "ShaderView",
+				fmt::format( "Scene \"{}\" does not have a \"shader\" input plug", prefixAndName.second )
+			);
+		}
+		else
+		{
+			shaderPlug->setInput( m_imageConverter->getChild<Plug>( "in" ) );
+		}
 
-	ScenePlug *outPlug = m_scene->getChild<ScenePlug>( "out" );
-	if( !outPlug || outPlug->direction() != Plug::Out )
+		ScenePlug *outPlug = m_scene->getChild<ScenePlug>( "out" );
+		if( !outPlug || outPlug->direction() != Plug::Out )
+		{
+			IECore::msg(
+				IECore::Msg::Error, "ShaderView",
+				fmt::format( "Scene \"{}\" does not have an \"out\" output scene plug", prefixAndName.second )
+			);
+			outPlug = nullptr;
+		}
+		m_imageConverter->getChild<DeleteOutputs>( "DeleteOutputs" )->inPlug()->setInput( outPlug );
+	}
+	else
 	{
-		throw IECore::Exception( "Scene does not have an \"out\" output scene plug" );
+		m_imageConverter->getChild<DeleteOutputs>( "DeleteOutputs" )->inPlug()->setInput( nullptr );
 	}
-	m_imageConverter->getChild<DeleteOutputs>( "DeleteOutputs" )->inPlug()->setInput( outPlug );
 
+	m_scenePrefixAndName = prefixAndName;
 	sceneChangedSignal()( this );
 }