Merge pull request #5682 from johnhaddon/lightToolCrashFix

LightTool crash fix

Changes.md

@@ -20,6 +20,7 @@ Fixes
 - Windows : Fixed "{path} was unexpected at this time." startup error when environment variables such as `PATH` contain `"` characters.
 - PathListingWidget : Fixed bug which caused the pointer to be stuck displaying the "values" icon after dragging cells with no value.
 - SceneAlgo : Fixed computation of history through Expression nodes.
+- LightTool : Fixed crash when deleting the node being viewed.
 - USD : Fixed loading of Arnold lights previously exported from Gaffer to USD.
 - Catalogue : Fixed connection delays on Windows.
 

python/GafferSceneUITest/LightToolTest.py

@@ -142,6 +142,52 @@ def testSelectionChangedSignal( self ) :
 		self.assertTrue( len( cs ) )
 		self.assertEqual( cs[0][0], tool )
 
+	def testDeleteNodeCrash( self ) :
+
+		# Make a spotlight and get the LightTool to edit it.
+
+		script = Gaffer.ScriptNode()
+
+		script["spotLight"] = GafferSceneTest.TestLight()
+		script["spotLight"]["parameters"].addChild( Gaffer.FloatPlug( "coneAngle", defaultValue = 10 ) )
+		script["spotLight"]["parameters"].addChild( Gaffer.FloatPlug( "penumbraAngle", defaultValue = 1 ) )
+
+		script["shaderAssignment"] = GafferScene.ShaderAssignment()
+		script["shaderAssignment"]["in"].setInput( script["spotLight"]["out"] )
+
+		view = GafferSceneUI.SceneView()
+		view["in"].setInput( script["shaderAssignment"]["out"] )
+
+		tool = GafferSceneUI.LightTool( view )
+		tool["active"].setValue( True )
+
+		GafferSceneUI.ContextAlgo.setSelectedPaths( view.getContext(), IECore.PathMatcher( [ "/light" ] ) )
+
+		with GafferUI.Window() as window :
+			gadgetWidget = GafferUI.GadgetWidget( view.viewportGadget() )
+		window.setVisible( True )
+
+		# Wait for the viewport to be rendered.
+
+		preRenderSlot = GafferTest.CapturingSlot( view.viewportGadget().preRenderSignal() )
+		while not len( preRenderSlot ) :
+			self.waitForIdle( 1000 )
+
+		# Delete the node being viewed.
+
+		del script["shaderAssignment"]
+
+		# Wait for the viewport to be rendered again. This used to crash, so
+		# we're pretty happy if it doesn't.
+
+		del preRenderSlot[:]
+		with IECore.CapturingMessageHandler() as mh :
+			while not len( preRenderSlot ) :
+				self.waitForIdle( 1000 )
+
+		# Ignore unrelated message from BackgroundTask. This needs a separate fix.
+		self.assertEqual( len( mh.messages ), 1 )
+		self.assertEqual( mh.messages[0].message, "Unable to find ScriptNode for SceneView.__preprocessor.out" )
 
 if __name__ == "__main__" :
-	unittest.main()
+	unittest.main()

src/GafferSceneUI/LightTool.cpp

@@ -794,15 +794,13 @@ class LightToolHandle : public Handle
 
 		// Update inspectors and data needed to display and interact with the handle. Called
 		// in `preRender()` if the inspections are dirty.
-		void updateHandlePath( ScenePlugPtr scene, const Context *context, const ScenePlug::ScenePath &handlePath )
+		void updateHandlePath( const ScenePlug::ScenePath &handlePath )
 		{
-			m_scene = scene;
-			m_context = context;
 			m_handlePath = handlePath;
 
 			m_inspectors.clear();
 
-			if( !m_scene->exists( m_handlePath ) )
+			if( !scene()->exists( m_handlePath ) )
 			{
 				return;
 			}
@@ -812,7 +810,7 @@ class LightToolHandle : public Handle
 			/// \todo This can be simplified and some of the logic, especially getting the inspectors, can
 			/// be moved to the constructor when we standardize on a single USDLux light representation.
 
-			ConstCompoundObjectPtr attributes = m_scene->fullAttributes( m_handlePath );
+			ConstCompoundObjectPtr attributes = scene()->fullAttributes( m_handlePath );
 
 			for( const auto &[attributeName, value ] : attributes->members() )
 			{
@@ -834,7 +832,7 @@ class LightToolHandle : public Handle
 						if( auto parameter = Metadata::value<StringData>( shaderAttribute, m ) )
 						{
 							m_inspectors[m] = new ParameterInspector(
-								m_scene,
+								scene(),
 								m_editScope,
 								attributeName,
 								ShaderNetwork::Parameter( "", parameter->readable() )
@@ -849,22 +847,6 @@ class LightToolHandle : public Handle
 			handlePathChanged();
 		}
 
-		/// \todo Should these three be protected, or left out entirely until they are needed by client code?
-		const ScenePlug *scene() const
-		{
-			return m_scene.get();
-		}
-
-		const Context *context() const
-		{
-			return m_context;
-		}
-
-		const ScenePlug::ScenePath &handlePath() const
-		{
-			return m_handlePath;
-		}
-
 		/// \todo Remove these and handle the lookThrough logic internally?
 		void setLookThroughLight( bool lookThroughLight )
 		{
@@ -980,6 +962,16 @@ class LightToolHandle : public Handle
 			mouseMoveSignal().connect( boost::bind( &LightToolHandle::mouseMove, this, ::_2 ) );
 		}
 
+		ScenePlug *scene() const
+		{
+			return m_view->inPlug<ScenePlug>();
+		}
+
+		const ScenePlug::ScenePath &handlePath() const
+		{
+			return m_handlePath;
+		}
+
 		// Returns true if `shaderAttribute` refers to the same light type
 		// as this handle was constructed to apply to.
 		bool isLightType( const std::string &shaderAttribute ) const
@@ -1010,7 +1002,7 @@ class LightToolHandle : public Handle
 		// Returns `nullptr` if no inspection exists for the handle.
 		Inspector::ResultPtr handleInspection( const InternedString &metaParameter ) const
 		{
-			ScenePlug::PathScope pathScope( m_context );
+			ScenePlug::PathScope pathScope( m_view->getContext() );
 			pathScope.setPath( &m_handlePath );
 
 			return inspection( metaParameter );
@@ -1266,18 +1258,13 @@ class LightToolHandle : public Handle
 		// Returns `nullptr` if no inspector or no inspection exists.
 		Inspector::ResultPtr inspection( const InternedString &metaParameter ) const
 		{
-			auto it = m_inspectors.find( metaParameter );
+			const auto it = m_inspectors.find( metaParameter );
 			if( it == m_inspectors.end() )
 			{
 				return nullptr;
 			}
 
-			if( Inspector::ResultPtr inspection = it->second->inspect() )
-			{
-				return inspection;
-			}
-
-			return nullptr;
+			return it->second->inspect();
 		}
 
 		bool allInspectionsEnabled() const
@@ -1297,8 +1284,6 @@ class LightToolHandle : public Handle
 			return enabled;
 		}
 
-		ScenePlugPtr m_scene;
-		const Context *m_context;
 		ScenePlug::ScenePath m_handlePath;
 
 		const std::string m_lightTypePattern;
@@ -3122,13 +3107,6 @@ void LightTool::updateHandleInspections()
 		return;
 	}
 
-	auto scene = scenePlug()->getInput<ScenePlug>();
-	scene = scene ? scene->getInput<ScenePlug>() : scene;
-	if( !scene )
-	{
-		return;
-	}
-
 	m_inspectorsDirtiedConnection.clear();
 
 	const PathMatcher selection = this->selection();
@@ -3163,7 +3141,7 @@ void LightTool::updateHandleInspections()
 		auto handle = runTimeCast<LightToolHandle>( c );
 		assert( handle );
 
-		handle->updateHandlePath( scene, view()->getContext(), lastSelectedPath );
+		handle->updateHandlePath( lastSelectedPath );
 
 		bool handleVisible = true;
 		bool handleEnabled = true;