From ab4980925d3db9320610285167c6fe9f68faa254 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Wed, 1 Nov 2023 15:05:28 +0000 Subject: [PATCH] RendererAlgo : Default `render:includedPurposes` to `default, render` This is the expected default that avoids doubling up `proxy` and `render` geometry in the same image. We couldn't use that for the default in `1.3.x` as it would have been a breaking change that could affect existing renders, but we want to provide the best default behaviour in 1.4. --- Changes.md | 1 + python/GafferSceneTest/RenderControllerTest.py | 18 ++++++++++-------- python/GafferSceneTest/RendererAlgoTest.py | 4 ++-- src/GafferScene/RendererAlgo.cpp | 4 +--- 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/Changes.md b/Changes.md index 41aac6c0d8b..76164812705 100644 --- a/Changes.md +++ b/Changes.md @@ -10,6 +10,7 @@ Improvements Breaking Changes ---------------- +- Render : Changed `render:includedPurposes` default to `"default", "render"`. - ValuePlug : Removed deprecated `getObjectValue()` overload. - Dispatcher : Removed `createMatching()` method. - Process : Removed non-const variant of the `handleException()` method. diff --git a/python/GafferSceneTest/RenderControllerTest.py b/python/GafferSceneTest/RenderControllerTest.py index 55e8329c512..8d112866343 100644 --- a/python/GafferSceneTest/RenderControllerTest.py +++ b/python/GafferSceneTest/RenderControllerTest.py @@ -1535,10 +1535,10 @@ def testIncludedPurposes( self ) : self.assertIsNotNone( renderer.capturedObject( "/group/sphere" ) ) - # Should still be visible when we add a purpose attribute, because we haven't - # specified the `render:includedPurposes` option. + # Should still be visible when we add a purpose of `render`, because that's included + # in the default for `render:includedPurposes`. - sphereAttributes["attributes"].addChild( Gaffer.NameValuePlug( "usd:purpose", "proxy", defaultEnabled = True ) ) + sphereAttributes["attributes"].addChild( Gaffer.NameValuePlug( "usd:purpose", "render", defaultEnabled = True ) ) self.assertTrue( controller.updateRequired() ) controller.update() self.assertIsNotNone( renderer.capturedObject( "/group/sphere" ) ) @@ -1546,9 +1546,10 @@ def testIncludedPurposes( self ) : # But should be hidden when we add `render:includedPurposes` to exclude it. standardOptions["options"]["includedPurposes"]["enabled"].setValue( True ) + standardOptions["options"]["includedPurposes"]["value"].setValue( IECore.StringVectorData( [ "default", "proxy" ] ) ) self.assertEqual( standardOptions["options"]["includedPurposes"]["value"].getValue(), - IECore.StringVectorData( [ "default", "render" ] ), + IECore.StringVectorData( [ "default", "proxy" ] ), ) self.assertTrue( controller.updateRequired() ) controller.update() @@ -1556,7 +1557,7 @@ def testIncludedPurposes( self ) : # Should be shown again if we change purpose to one that is included. - sphereAttributes["attributes"][0]["value"].setValue( "render" ) + sphereAttributes["attributes"][0]["value"].setValue( "proxy" ) self.assertTrue( controller.updateRequired() ) controller.update() self.assertIsNotNone( renderer.capturedObject( "/group/sphere" ) ) @@ -1564,7 +1565,7 @@ def testIncludedPurposes( self ) : # Shouldn't matter if parent has a purpose which is excluded, because local # purpose will override that. - groupAttributes["attributes"].addChild( Gaffer.NameValuePlug( "usd:purpose", "proxy", defaultEnabled = True ) ) + groupAttributes["attributes"].addChild( Gaffer.NameValuePlug( "usd:purpose", "render", defaultEnabled = True ) ) self.assertTrue( controller.updateRequired() ) controller.update() self.assertIsNotNone( renderer.capturedObject( "/group/sphere" ) ) @@ -1577,7 +1578,8 @@ def testIncludedPurposes( self ) : controller.update() self.assertIsNone( renderer.capturedObject( "/group/sphere" ) ) - # Reverting to no `includedPurposes` option should revert to showing everything. + # Reverting to no `includedPurposes` option should revert to showing + # just `default` and `render`. standardOptions["options"]["includedPurposes"]["enabled"].setValue( False ) self.assertTrue( controller.updateRequired() ) @@ -1640,7 +1642,7 @@ def assertExpectedRenderOptions() : # get another update. del capture - standardOptions["options"]["includedPurposes"]["value"].setValue( IECore.StringVectorData( [ "default", "render", "proxy", "guide" ] ) ) + standardOptions["options"]["includedPurposes"]["value"].setToDefault() self.assertTrue( controller.updateRequired() ) controller.update() assertExpectedRenderOptions() diff --git a/python/GafferSceneTest/RendererAlgoTest.py b/python/GafferSceneTest/RendererAlgoTest.py index 6f7e799177e..6e5c06b38e4 100644 --- a/python/GafferSceneTest/RendererAlgoTest.py +++ b/python/GafferSceneTest/RendererAlgoTest.py @@ -672,14 +672,14 @@ def assertIncludedObjects( scene, includedPurposes, paths ) : else : self.assertIsNone( renderer.capturedObject( path ) ) - # If we don't specify a purpose, then we should get everything. + # If we don't specify a purpose, then we should get just "default" + # and "render". assertIncludedObjects( group["out"], None, { "/group/innerGroup1/cube", "/group/innerGroup1/sphere", - "/group/innerGroup2/cube", "/group/innerGroup2/sphere", } ) diff --git a/src/GafferScene/RendererAlgo.cpp b/src/GafferScene/RendererAlgo.cpp index 3d3dd733d9b..9993a1e5d4a 100644 --- a/src/GafferScene/RendererAlgo.cpp +++ b/src/GafferScene/RendererAlgo.cpp @@ -91,9 +91,7 @@ const InternedString g_shutterOptionName( "option:render:shutter" ); const InternedString g_includedPurposesOptionName( "option:render:includedPurposes" ); const InternedString g_purposeAttributeName( "usd:purpose" ); -/// \todo We should really default to `{ "default", "render" }`, but can only -/// change that on a major version update. -const ConstStringVectorDataPtr g_defaultIncludedPurposes( new StringVectorData( { "default", "render", "proxy", "guide" } ) ); +const ConstStringVectorDataPtr g_defaultIncludedPurposes( new StringVectorData( { "default", "render" } ) ); const std::string g_defaultPurpose( "default" ); } // namespace