Merge branch '1.5_maintenance'

Changes.md

@@ -9,7 +9,13 @@ Breaking Changes
 1.5.x.x (relative to 1.5.3.0)
 =======
 
+Fixes
+-----
 
+- AttributeEditor, LightEditor, RenderPassEditor :
+  - Fixed bugs which prevented edits being made in "Source" scope when there was a downstream edit in an EditScope (#6172).
+  - Fixed warning messages when attempting to disable a non-existent edit.
+  - Fixed warning message which referred to "None" rather than the "Source" scope.
 
 1.5.3.0 (relative to 1.5.2.0)
 =======

include/GafferSceneUI/Private/Inspector.h

@@ -51,10 +51,10 @@
 #include "boost/multi_index/member.hpp"
 #include "boost/multi_index/random_access_index.hpp"
 #include "boost/multi_index_container.hpp"
-#include "boost/variant.hpp"
 
 #include <unordered_set>
 #include <unordered_map>
+#include <variant>
 
 namespace GafferSceneUIModule
 {
@@ -171,7 +171,7 @@ class GAFFERSCENEUI_API Inspector : public IECore::RefCounted, public Gaffer::Si
 		virtual Gaffer::ValuePlugPtr source( const GafferScene::SceneAlgo::History *history, std::string &editWarning ) const;
 
 		using EditFunction = std::function<Gaffer::ValuePlugPtr ( bool createIfNecessary )>;
-		using EditFunctionOrFailure = boost::variant<EditFunction, std::string>;
+		using EditFunctionOrFailure = std::variant<EditFunction, std::string>;
 		/// Should be implemented to return a function that will acquire
 		/// an edit from the EditScope at the specified point in the history.
 		/// If this is not possible, should return an error explaining why
@@ -184,7 +184,7 @@ class GAFFERSCENEUI_API Inspector : public IECore::RefCounted, public Gaffer::Si
 		virtual EditFunctionOrFailure editFunction( Gaffer::EditScope *editScope, const GafferScene::SceneAlgo::History *history ) const;
 
 		using DisableEditFunction = std::function<void ()>;
-		using DisableEditFunctionOrFailure = boost::variant<DisableEditFunction, std::string>;
+		using DisableEditFunctionOrFailure = std::variant<DisableEditFunction, std::string>;
 		/// Can be implemented to return a function that will disable an edit
 		/// at the specified plug. If this is not possible, should return an
 		/// error explaining why (this is typically due to `readOnly` metadata).
@@ -374,10 +374,15 @@ class GAFFERSCENEUI_API Inspector::Result : public IECore::RefCounted
 		Gaffer::EditScopePtr m_editScope;
 		bool m_editScopeInHistory;
 
-		EditFunctionOrFailure m_editFunction;
-		std::string m_editWarning;
+		struct Editors
+		{
+			/// \todo Rename to `acquireEditFunction`?
+			EditFunctionOrFailure editFunction;
+			std::string editWarning;
+			DisableEditFunctionOrFailure disableEditFunction;
+		};
 
-		DisableEditFunctionOrFailure m_disableEditFunction;
+		std::optional<Editors> m_editors;
 
 };
 

python/GafferSceneTest/InstancerTest.py

@@ -3123,6 +3123,41 @@ def testRenderHashes( self ) :
 			else:
 				rootsByHash[ co.hash() ] = co.root()
 
+	def testDestinationBug( self ) :
+
+		# The destination plug should be automatically handled by BranchCreator, but previously Instancer had
+		# some weird special cases that broke this - this tests the specific things that were previously broken
+
+		sphere = GafferScene.Sphere()
+		sphere["sets"].setValue( "sphereSet" )
+
+		cube = GafferScene.Cube()
+
+		cubeFilter = GafferScene.PathFilter()
+		cubeFilter["paths"].setValue( IECore.StringVectorData( [ '/cube' ] ) )
+
+		instancer = GafferScene.Instancer()
+		instancer["in"].setInput( cube["out"] )
+		instancer["prototypes"].setInput( sphere["out"] )
+		instancer["filter"].setInput( cubeFilter["out"] )
+		instancer["__destination"].setValue( "${scene:path}/.." )
+
+		# Check that the instances are going to right place
+		self.assertEqual(
+			instancer["out"].childNames( "/instances/sphere" ),
+			IECore.InternedStringVectorData( [ str( i ) for i in range( 8 ) ] )
+		)
+		self.assertEqual( instancer["out"].object( "/instances/sphere/4" ), sphere["out"].object( "/sphere" ) )
+
+		# Check the specifics that were previously broken
+		self.assertEqual( instancer["out"].bound( "/" ), imath.Box3f( imath.V3f( -1.5 ), imath.V3f( 1.5 ) ) )
+		self.assertEqual(
+			instancer["out"].set( "sphereSet" ),
+			IECore.PathMatcherData( IECore.PathMatcher( [ "/instances/sphere/%i" % i for i in range( 8 ) ] ) )
+		)
+
+		self.assertSceneValid( instancer["out"] )
+
 	@GafferTest.TestRunner.PerformanceTestMethod( repeat = 10 )
 	def testBoundPerformance( self ) :
 

python/GafferSceneUITest/AttributeInspectorTest.py

@@ -912,8 +912,8 @@ def testDisableEdit( self ) :
 
 		inspection = self.__inspect( s["editScope2"]["out"], "/group/light", "gl:visualiser:scale", s["editScope2"] )
 		self.assertFalse( inspection.canDisableEdit() )
-		self.assertEqual( inspection.nonDisableableReason(), "Edit is not in the current edit scope. Change scope to editScope1 to disable." )
-		self.assertRaisesRegex( IECore.Exception, "Cannot disable edit : Edit is not in the current edit scope. Change scope to editScope1 to disable.", inspection.disableEdit )
+		self.assertEqual( inspection.nonDisableableReason(), "There is no edit in editScope2." )
+		self.assertRaisesRegex( IECore.Exception, "Cannot disable edit : There is no edit in editScope2.", inspection.disableEdit )
 
 		Gaffer.MetadataAlgo.setReadOnly( s["editScope1"], True )
 		inspection = self.__inspect( s["editScope1"]["out"], "/group/light", "gl:visualiser:scale", s["editScope1"] )
@@ -930,7 +930,7 @@ def testDisableEdit( self ) :
 
 		inspection = self.__inspect( s["editScope1"]["out"], "/group/light", "gl:visualiser:scale", s["editScope1"] )
 		self.assertFalse( inspection.canDisableEdit() )
-		self.assertEqual( inspection.nonDisableableReason(), "Edit is not in the current edit scope. Change scope to None to disable." )
+		self.assertEqual( inspection.nonDisableableReason(), "There is no edit in editScope1." )
 
 		inspection = self.__inspect( s["editScope1"]["out"], "/group/light", "gl:visualiser:scale", None )
 		self.assertFalse( inspection.canDisableEdit() )

python/GafferSceneUITest/OptionInspectorTest.py

@@ -652,13 +652,15 @@ def testRenderPassSourceAndEdits( self ) :
 			)
 
 			# When using no scope, make sure that we don't inadvertently edit the contents of an EditScope.
+			# We should be allowed to edit the source before the scope, but only with a warning about there
+			# being a downstream override.
 
 			self.__assertExpectedResult(
 				self.__inspect( s["editScope2"]["out"], "render:camera", None, context ),
 				source = s["editScope1"]["standardOptions3"]["options"]["renderCamera"],
-				sourceType = SourceType.Other,
-				editable = False,
-				nonEditableReason = "Source is in an EditScope. Change scope to editScope1 to edit."
+				sourceType = SourceType.Downstream,
+				editable = True,
+				editWarning = "Option has edits downstream in editScope1."
 			)
 
 			# If there is a StandardOptions node outside of an edit scope, make sure we use that with no scope
@@ -950,7 +952,7 @@ def testDisableEdit( self ) :
 
 		inspection = self.__inspect( s["editScope2"]["out"], "render:camera", s["editScope2"] )
 		self.assertFalse( inspection.canDisableEdit() )
-		self.assertEqual( inspection.nonDisableableReason(), "Edit is not in the current edit scope. Change scope to None to disable." )
+		self.assertEqual( inspection.nonDisableableReason(), "There is no edit in editScope2." )
 
 		Gaffer.MetadataAlgo.setReadOnly( s["standardOptions"]["options"], True )
 		inspection = self.__inspect( s["group"]["out"], "render:camera", None )
@@ -986,8 +988,8 @@ def testDisableEdit( self ) :
 
 		inspection = self.__inspect( s["editScope2"]["out"], "render:camera", s["editScope2"] )
 		self.assertFalse( inspection.canDisableEdit() )
-		self.assertEqual( inspection.nonDisableableReason(), "Edit is not in the current edit scope. Change scope to editScope1 to disable." )
-		self.assertRaisesRegex( IECore.Exception, "Cannot disable edit : Edit is not in the current edit scope. Change scope to editScope1 to disable.", inspection.disableEdit )
+		self.assertEqual( inspection.nonDisableableReason(), "There is no edit in editScope2." )
+		self.assertRaisesRegex( IECore.Exception, "Cannot disable edit : There is no edit in editScope2.", inspection.disableEdit )
 
 		Gaffer.MetadataAlgo.setReadOnly( s["editScope1"], True )
 		inspection = self.__inspect( s["editScope1"]["out"], "render:camera", s["editScope1"] )

python/GafferSceneUITest/ParameterInspectorTest.py

@@ -478,14 +478,15 @@ def testDisableEdit( self ) :
 		self.assertEqual( inspection.nonDisableableReason(), "The target edit scope editScope2 is not in the scene history." )
 
 		inspection = self.__inspect( s["editScope2"]["out"], "/light", "exposure", None )
+		self.assertTrue( inspection.acquireEdit( False ).isSame( s["light"]["parameters"]["exposure"] ) )
 		self.assertFalse( inspection.canDisableEdit() )
-		self.assertEqual( inspection.nonDisableableReason(), "Source is in an EditScope. Change scope to editScope to disable." )
-		self.assertRaisesRegex( IECore.Exception, "Cannot disable edit : Source is in an EditScope. Change scope to editScope to disable.", inspection.disableEdit )
+		self.assertEqual( inspection.nonDisableableReason(), "Disabling edits not supported for this plug." )
+		self.assertRaisesRegex( IECore.Exception, "Cannot disable edit : Disabling edits not supported for this plug.", inspection.disableEdit )
 
 		inspection = self.__inspect( s["editScope2"]["out"], "/light", "exposure", s["editScope2"] )
 		self.assertFalse( inspection.canDisableEdit() )
-		self.assertEqual( inspection.nonDisableableReason(), "Edit is not in the current edit scope. Change scope to editScope to disable." )
-		self.assertRaisesRegex( IECore.Exception, "Cannot disable edit : Edit is not in the current edit scope. Change scope to editScope to disable.", inspection.disableEdit )
+		self.assertEqual( inspection.nonDisableableReason(), "There is no edit in editScope2." )
+		self.assertRaisesRegex( IECore.Exception, "Cannot disable edit : There is no edit in editScope2.", inspection.disableEdit )
 
 		Gaffer.MetadataAlgo.setReadOnly( s["editScope"], True )
 		inspection = self.__inspect( s["editScope"]["out"], "/light", "exposure", s["editScope"] )
@@ -502,7 +503,7 @@ def testDisableEdit( self ) :
 
 		inspection = self.__inspect( s["editScope"]["out"], "/light", "exposure", s["editScope"] )
 		self.assertFalse( inspection.canDisableEdit() )
-		self.assertEqual( inspection.nonDisableableReason(), "Edit is not in the current edit scope. Change scope to None to disable." )
+		self.assertEqual( inspection.nonDisableableReason(), "There is no edit in editScope." )
 
 		inspection = self.__inspect( s["editScope"]["out"], "/light", "exposure", None )
 		self.assertEqual( inspection.source(), s["light"]["parameters"]["exposure"] )
@@ -1088,6 +1089,72 @@ def testShaderNetwork( self ) :
 			editWarning = "Edits to box.add may affect other locations in the scene."
 		)
 
+	def testLightCreatedInEditScope( self ) :
+
+		light = GafferSceneTest.TestLight()
+
+		editScope1 = Gaffer.EditScope( "EditScope1" )
+		editScope1.setup( light["out"] )
+
+		editScope1["light"] = light
+		editScope1["parent"] = GafferScene.Parent()
+		editScope1["parent"]["parent"].setValue( "/" )
+		editScope1["parent"]["in"].setInput( editScope1["BoxIn"]["out"] )
+		editScope1["parent"]["children"][0].setInput( editScope1["light"]["out"] )
+
+		editScope1["BoxOut"]["in"].setInput( editScope1["parent"]["out"] )
+
+		editScope2 = Gaffer.EditScope( "EditScope2" )
+		editScope2.setup( editScope1["out"] )
+		editScope2["in"].setInput( editScope1["out"] )
+
+		# Make edit in EditScope2.
+
+		i = self.__inspect( editScope2["out"], "/light", "exposure", editScope2 )
+		scope2Edit = i.acquireEdit()
+		self.assertTrue( editScope2.isAncestorOf( scope2Edit ) )
+		scope2Edit["enabled"].setValue( True )
+		scope2Edit["value"].setValue( 2 )
+
+		# Check that we can still edit in EditScope1, accompanied by
+		# a suitable warning.
+
+		self.__assertExpectedResult(
+			self.__inspect( editScope2["out"], "/light", "exposure", editScope1 ),
+			source = scope2Edit,
+			sourceType = GafferSceneUI.Private.Inspector.Result.SourceType.Downstream,
+			editable = True,
+			edit = light["parameters"]["exposure"],
+			editWarning = "Parameter has edits downstream in EditScope2."
+		)
+
+	def testSourceWithDownstreamOverride( self ) :
+
+		light = GafferSceneTest.TestLight()
+
+		editScope = Gaffer.EditScope()
+		editScope.setup( light["out"] )
+		editScope["in"].setInput( light["out"] )
+
+		# Make edit in EditScope.
+
+		i = self.__inspect( editScope["out"], "/light", "exposure", editScope )
+		scopeEdit = i.acquireEdit()
+		self.assertTrue( editScope.isAncestorOf( scopeEdit ) )
+		scopeEdit["enabled"].setValue( True )
+		scopeEdit["value"].setValue( 2 )
+
+		# Check that we can still edit the source, accompanied by
+		# a suitable warning.
+
+		self.__assertExpectedResult(
+			self.__inspect( editScope["out"], "/light", "exposure", editScope = None ),
+			source = scopeEdit,
+			sourceType = GafferSceneUI.Private.Inspector.Result.SourceType.Downstream,
+			editable = True,
+			edit = light["parameters"]["exposure"],
+			editWarning = "Parameter has edits downstream in EditScope."
+		)
 
 if __name__ == "__main__":
 	unittest.main()

python/GafferSceneUITest/SetMembershipInspectorTest.py

@@ -690,9 +690,10 @@ def testDisableEdit( self ) :
 
 			Gaffer.MetadataAlgo.setReadOnly( s["editScope1"], False )
 			inspection = self.__inspect( s["editScope1"]["out"], "/group/plane", "planeSetEditScope", None )
+			self.assertTrue( inspection.acquireEdit( False ).isSame( s["plane"]["sets"] ) )
 			self.assertFalse( inspection.canDisableEdit() )
-			self.assertEqual( inspection.nonDisableableReason(), "Source is in an EditScope. Change scope to editScope1 to disable." )
-			self.assertRaisesRegex( IECore.Exception, "Cannot disable edit : Source is in an EditScope. Change scope to editScope1 to disable.", inspection.disableEdit )
+			self.assertEqual( inspection.nonDisableableReason(), "plane.sets has no edit to disable." )
+			self.assertRaisesRegex( IECore.Exception, "Cannot disable edit : plane.sets has no edit to disable.", inspection.disableEdit )
 
 			inspection = self.__inspect( s["editScope1"]["out"], "/group/plane", "planeSetEditScope", s["editScope1"] )
 			self.assertTrue( inspection.canDisableEdit() )
@@ -705,4 +706,4 @@ def testDisableEdit( self ) :
 			)
 
 if __name__ == "__main__" :
-	unittest.main()
+	unittest.main()

src/GafferScene/Instancer.cpp

@@ -2195,13 +2195,7 @@ void Instancer::hashBranchBound( const ScenePath &sourcePath, const ScenePath &b
 	if( branchPath.size() < 2 )
 	{
 		// "/" or "/instances"
-		ScenePath path = sourcePath;
-		path.insert( path.end(), branchPath.begin(), branchPath.end() );
-		if( branchPath.size() == 0 )
-		{
-			path.push_back( namePlug()->getValue() );
-		}
-		h = outPlug()->childBoundsHash( path );
+		h = outPlug()->childBoundsPlug()->hash();
 	}
 	else if( branchPath.size() == 2 )
 	{
@@ -2231,13 +2225,7 @@ Imath::Box3f Instancer::computeBranchBound( const ScenePath &sourcePath, const S
 	if( branchPath.size() < 2 )
 	{
 		// "/" or "/instances"
-		ScenePath path = sourcePath;
-		path.insert( path.end(), branchPath.begin(), branchPath.end() );
-		if( branchPath.size() == 0 )
-		{
-			path.push_back( namePlug()->getValue() );
-		}
-		return outPlug()->childBounds( path );
+		return outPlug()->childBoundsPlug()->getValue();
 	}
 	else if( branchPath.size() == 2 )
 	{
@@ -2678,6 +2666,7 @@ bool Instancer::affectsBranchSet( const Gaffer::Plug *input ) const
 {
 	return
 		input == enginePlug() ||
+		input == engineSplitPrototypesPlug() ||
 		input == prototypesPlug()->setPlug() ||
 		input == namePlug() ||
 		input == setCollaboratePlug()
@@ -2715,6 +2704,7 @@ void Instancer::hashBranchSet( const ScenePath &sourcePath, const IECore::Intern
 	else
 	{
 		engineHash( sourcePath, context, h );
+		engineSplitPrototypesHash( sourcePath, context, h );
 		prototypesPlug()->setPlug()->hash( h );
 		namePlug()->hash( h );
 	}
@@ -2735,30 +2725,25 @@ IECore::ConstPathMatcherDataPtr Instancer::computeBranchSet( const ScenePath &so
 		return setCollaboratePlug()->getValue();
 	}
 
+	ConstEngineSplitPrototypesDataPtr esp = engineSplitPrototypes( sourcePath, context );
+
 	ConstPathMatcherDataPtr inputSet = prototypesPlug()->setPlug()->getValue();
 
 	PathMatcherDataPtr outputSetData = new PathMatcherData;
 	PathMatcher &outputSet = outputSetData->writable();
 
 	vector<InternedString> branchPath( { namePlug()->getValue(), InternedString(), InternedString() } );
 
-	vector<InternedString> outputPrototypePath( sourcePath.size() + 2 );
-	outputPrototypePath = sourcePath;
-	outputPrototypePath.push_back( namePlug()->getValue() );
-	outputPrototypePath.push_back( InternedString() );
-
 	for( const auto &prototypeName : engine->prototypeNames()->readable() )
 	{
+		const std::vector<size_t> &pointIndicesForPrototype = esp->pointIndicesForPrototype( prototypeName );
+
 		PathMatcher instanceSet = inputSet->readable().subTree( *engine->prototypeRoot( prototypeName ) );
 		branchPath[1] = prototypeName;
 
-		outputPrototypePath.back() = prototypeName;
-
-		ConstInternedStringVectorDataPtr childNamesData = capsuleScenePlug()->childNames( outputPrototypePath );
-
-		for( const auto &childName : childNamesData->readable() )
+		for( const size_t &index : pointIndicesForPrototype )
 		{
-			branchPath[2] = childName;
+			branchPath[2] = engine->instanceId( index );
 			outputSet.addPaths( instanceSet, branchPath );
 		}
 	}