From bfdada265995ab4f1c604661597dfcb0db100141 Mon Sep 17 00:00:00 2001 From: Murray Stevenson <50844517+murraystevenson@users.noreply.github.com> Date: Tue, 9 Jul 2024 18:03:56 -0700 Subject: [PATCH 1/7] AttributeInspector : Implement fallbackValue to return inherited values This enables us to fall back to inspecting the values of inherited attributes, using the fallbackValue approach already used by OptionInspector for default option values registered as metadata. The initial use-case for this is in the LightEditor, but this would also be of use in a future where we include columns displaying attribute values in the Hierarchy View and other Editors. --- Changes.md | 3 ++ .../Private/AttributeInspector.h | 1 + include/GafferSceneUI/Private/Inspector.h | 2 +- .../GafferSceneUI/Private/OptionInspector.h | 2 +- .../Private/ParameterInspector.h | 1 + .../AttributeInspectorTest.py | 32 +++++++++++++++++++ src/GafferSceneUI/AttributeInspector.cpp | 5 +++ src/GafferSceneUI/Inspector.cpp | 4 +-- src/GafferSceneUI/OptionInspector.cpp | 2 +- src/GafferSceneUI/ParameterInspector.cpp | 6 ++++ 10 files changed, 53 insertions(+), 5 deletions(-) diff --git a/Changes.md b/Changes.md index 543f5273c92..a8dfcbc5e20 100644 --- a/Changes.md +++ b/Changes.md @@ -1,7 +1,10 @@ 1.4.x.x (relative to 1.4.9.0) ======= +Improvements +------------ +- LightEditor : Values of inherited attributes are now displayed in the Light Editor. These are presented as dimmed "fallback" values. 1.4.9.0 (relative to 1.4.8.0) ======= diff --git a/include/GafferSceneUI/Private/AttributeInspector.h b/include/GafferSceneUI/Private/AttributeInspector.h index 2cf30971144..920cca95253 100644 --- a/include/GafferSceneUI/Private/AttributeInspector.h +++ b/include/GafferSceneUI/Private/AttributeInspector.h @@ -67,6 +67,7 @@ class GAFFERSCENEUI_API AttributeInspector : public Inspector IECore::ConstObjectPtr value( const GafferScene::SceneAlgo::History *history) const override; Gaffer::ValuePlugPtr source( const GafferScene::SceneAlgo::History *history, std::string &editWarning ) const override; EditFunctionOrFailure editFunction( Gaffer::EditScope *scope, const GafferScene::SceneAlgo::History *history) const override; + IECore::ConstObjectPtr fallbackValue( const GafferScene::SceneAlgo::History *history ) const override; bool attributeExists() const; diff --git a/include/GafferSceneUI/Private/Inspector.h b/include/GafferSceneUI/Private/Inspector.h index 48fc2293e95..c0c1d8e46e4 100644 --- a/include/GafferSceneUI/Private/Inspector.h +++ b/include/GafferSceneUI/Private/Inspector.h @@ -179,7 +179,7 @@ class GAFFERSCENEUI_API Inspector : public IECore::RefCounted, public Gaffer::Si /// Can be implemented by derived classes to provide a fallback value for the inspection, /// used when no value is returned from `value()`. - virtual IECore::ConstObjectPtr fallbackValue() const; + virtual IECore::ConstObjectPtr fallbackValue( const GafferScene::SceneAlgo::History *history ) const; protected : diff --git a/include/GafferSceneUI/Private/OptionInspector.h b/include/GafferSceneUI/Private/OptionInspector.h index aaf3db525dc..617ee73e803 100644 --- a/include/GafferSceneUI/Private/OptionInspector.h +++ b/include/GafferSceneUI/Private/OptionInspector.h @@ -65,7 +65,7 @@ class GAFFERSCENEUI_API OptionInspector : public Inspector IECore::ConstObjectPtr value( const GafferScene::SceneAlgo::History *history ) const override; Gaffer::ValuePlugPtr source( const GafferScene::SceneAlgo::History *history, std::string &editWarning ) const override; EditFunctionOrFailure editFunction( Gaffer::EditScope *scope, const GafferScene::SceneAlgo::History *history ) const override; - IECore::ConstObjectPtr fallbackValue() const override; + IECore::ConstObjectPtr fallbackValue( const GafferScene::SceneAlgo::History *history ) const override; private : diff --git a/include/GafferSceneUI/Private/ParameterInspector.h b/include/GafferSceneUI/Private/ParameterInspector.h index a63d6a062c1..97ac7e992d3 100644 --- a/include/GafferSceneUI/Private/ParameterInspector.h +++ b/include/GafferSceneUI/Private/ParameterInspector.h @@ -69,6 +69,7 @@ class GAFFERSCENEUI_API ParameterInspector : public AttributeInspector IECore::ConstObjectPtr value( const GafferScene::SceneAlgo::History *history ) const override; Gaffer::ValuePlugPtr source( const GafferScene::SceneAlgo::History *history, std::string &editWarning ) const override; EditFunctionOrFailure editFunction( Gaffer::EditScope *editScope, const GafferScene::SceneAlgo::History *history ) const override; + IECore::ConstObjectPtr fallbackValue( const GafferScene::SceneAlgo::History *history ) const override; const IECoreScene::ShaderNetwork::Parameter m_parameter; diff --git a/python/GafferSceneUITest/AttributeInspectorTest.py b/python/GafferSceneUITest/AttributeInspectorTest.py index 85d4fb0e134..a76e3fe255d 100644 --- a/python/GafferSceneUITest/AttributeInspectorTest.py +++ b/python/GafferSceneUITest/AttributeInspectorTest.py @@ -116,6 +116,38 @@ def testValue( self ) : IECore.FloatData( 2.0 ) ) + def testFallbackValue( self ) : + + light = GafferSceneTest.TestLight() + group = GafferScene.Group() + group["in"][0].setInput( light["out"] ) + + groupFilter = GafferScene.PathFilter() + groupFilter["paths"].setValue( IECore.StringVectorData( [ "/group" ] ) ) + + glAttributes = GafferScene.OpenGLAttributes() + glAttributes["in"].setInput( group["out"] ) + glAttributes["filter"].setInput( groupFilter["out"] ) + glAttributes["attributes"]["visualiserScale"]["enabled"].setValue( True ) + glAttributes["attributes"]["visualiserScale"]["value"].setValue( 2.0 ) + + # With no "gl:visualiser:scale" attribute at /group/light, the inspection returns + # the inherited attribute value with `sourceType` identifying it as a fallback. + + inspection = self.__inspect( glAttributes["out"], "/group/light", "gl:visualiser:scale" ) + self.assertEqual( inspection.value(), IECore.FloatData( 2.0 ) ) + self.assertEqual( inspection.sourceType(), GafferSceneUI.Private.Inspector.Result.SourceType.Fallback ) + + # With a "gl:visualiser:scale" attribute created at the inspected location, it is + # returned instead of the inherited fallback. + + light["visualiserAttributes"]["scale"]["enabled"].setValue( True ) + light["visualiserAttributes"]["scale"]["value"].setValue( 4.0 ) + + inspection = self.__inspect( glAttributes["out"], "/group/light", "gl:visualiser:scale" ) + self.assertEqual( inspection.value(), IECore.FloatData( 4.0 ) ) + self.assertEqual( inspection.sourceType(), GafferSceneUI.Private.Inspector.Result.SourceType.Other ) + def testSourceAndEdits( self ) : s = Gaffer.ScriptNode() diff --git a/src/GafferSceneUI/AttributeInspector.cpp b/src/GafferSceneUI/AttributeInspector.cpp index 9a5e20b6052..34b2e18fc40 100644 --- a/src/GafferSceneUI/AttributeInspector.cpp +++ b/src/GafferSceneUI/AttributeInspector.cpp @@ -245,6 +245,11 @@ IECore::ConstObjectPtr AttributeInspector::value( const GafferScene::SceneAlgo:: return nullptr; } +IECore::ConstObjectPtr AttributeInspector::fallbackValue( const GafferScene::SceneAlgo::History *history ) const +{ + return history->scene->fullAttributes( history->context->get( ScenePlug::scenePathContextName ) )->member( m_attribute ); +} + Gaffer::ValuePlugPtr AttributeInspector::source( const GafferScene::SceneAlgo::History *history, std::string &editWarning ) const { auto sceneNode = runTimeCast( history->scene->node() ); diff --git a/src/GafferSceneUI/Inspector.cpp b/src/GafferSceneUI/Inspector.cpp index 63a4c53ccd9..4de806889d1 100644 --- a/src/GafferSceneUI/Inspector.cpp +++ b/src/GafferSceneUI/Inspector.cpp @@ -193,7 +193,7 @@ Inspector::ResultPtr Inspector::inspect() const bool fallbackValue = false; if( !value ) { - value = this->fallbackValue(); + value = this->fallbackValue( history.get() ); fallbackValue = (bool)value; } @@ -365,7 +365,7 @@ Inspector::EditFunctionOrFailure Inspector::editFunction( Gaffer::EditScope *edi return "Editing not supported"; } -IECore::ConstObjectPtr Inspector::fallbackValue() const +IECore::ConstObjectPtr Inspector::fallbackValue( const GafferScene::SceneAlgo::History *history ) const { return nullptr; } diff --git a/src/GafferSceneUI/OptionInspector.cpp b/src/GafferSceneUI/OptionInspector.cpp index ea1ae2aaa52..579c6cb55be 100644 --- a/src/GafferSceneUI/OptionInspector.cpp +++ b/src/GafferSceneUI/OptionInspector.cpp @@ -215,7 +215,7 @@ IECore::ConstObjectPtr OptionInspector::value( const GafferScene::SceneAlgo::His return nullptr; } -IECore::ConstObjectPtr OptionInspector::fallbackValue() const +IECore::ConstObjectPtr OptionInspector::fallbackValue( const GafferScene::SceneAlgo::History *history ) const { if( const auto defaultValue = Gaffer::Metadata::value( g_optionPrefix + m_option.string(), g_defaultValue ) ) { diff --git a/src/GafferSceneUI/ParameterInspector.cpp b/src/GafferSceneUI/ParameterInspector.cpp index 72376b8c935..f35ae77d427 100644 --- a/src/GafferSceneUI/ParameterInspector.cpp +++ b/src/GafferSceneUI/ParameterInspector.cpp @@ -98,6 +98,12 @@ IECore::ConstObjectPtr ParameterInspector::value( const GafferScene::SceneAlgo:: return shader->parametersData()->member( m_parameter.name ); } +IECore::ConstObjectPtr ParameterInspector::fallbackValue( const GafferScene::SceneAlgo::History *history ) const +{ + // No fallback values are provided for parameters. Implemented to override AttributeInspector::fallbackValue(). + return nullptr; +} + Gaffer::ValuePlugPtr ParameterInspector::source( const GafferScene::SceneAlgo::History *history, std::string &editWarning ) const { auto sceneNode = runTimeCast( history->scene->node() ); From 41c0d936ebb7fe6599f3c3b4c29fa129b3d87135 Mon Sep 17 00:00:00 2001 From: Murray Stevenson <50844517+murraystevenson@users.noreply.github.com> Date: Tue, 9 Jul 2024 18:06:58 -0700 Subject: [PATCH 2/7] LightEditor : Update MuteColumn cellData to use Inspector fallbackValue With the AttributeInspector now providing inherited `light:mute` attribute values as a fallback, we can use it to determine which mute icon to display for a path based on the inspected value and the inspection sourceType. Though for paths inheriting a `light:mute` attribute we still need to walk up the hierarchy to find the location inherited from in order to display it in the tooltip... --- .../LightEditorBinding.cpp | 61 +++++++++++-------- 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/src/GafferSceneUIModule/LightEditorBinding.cpp b/src/GafferSceneUIModule/LightEditorBinding.cpp index dec64e0dc77..292bedb2b25 100644 --- a/src/GafferSceneUIModule/LightEditorBinding.cpp +++ b/src/GafferSceneUIModule/LightEditorBinding.cpp @@ -191,7 +191,7 @@ class InspectorColumn : public PathColumn m_inspector->dirtiedSignal().connect( boost::bind( &InspectorColumn::inspectorDirtied, this ) ); } - GafferSceneUI::Private::Inspector *inspector() + GafferSceneUI::Private::Inspector *inspector() const { return m_inspector.get(); } @@ -292,39 +292,46 @@ class MuteColumn : public InspectorColumn if( auto value = runTimeCast( result.value ) ) { - result.icon = value->readable() ? m_muteIconData : m_unMuteIconData; - } - else - { - ScenePlug::PathScope pathScope( scenePath->getContext() ); - ScenePlug::ScenePath currentPath( scenePath->names() ); - while( !currentPath.empty() ) + ScenePlug::PathScope pathScope( scenePath->getContext(), &scenePath->names() ); + pathScope.setCanceller( canceller ); + + Inspector::ConstResultPtr inspectorResult = inspector()->inspect(); + if( inspectorResult->sourceType() != Inspector::Result::SourceType::Fallback ) { - pathScope.setPath( ¤tPath ); - auto a = scenePath->getScene()->attributesPlug()->getValue(); - if( auto fullValue = a->member( "light:mute" ) ) - { - result.icon = fullValue->readable() ? m_muteFadedIconData : m_unMuteFadedIconData; - result.toolTip = new StringData( "Inherited from : " + ScenePlug::pathToString( currentPath ) ); - break; - } - currentPath.pop_back(); + result.icon = value->readable() ? m_muteIconData : m_unMuteIconData; } - if( !result.icon ) + else { - // Use a transparent icon to reserve space in the UI. Without this, - // the top row will resize when setting the mute value, causing a full - // table resize. - if( path.isEmpty() ) - { - result.icon = m_muteBlankIconName; - } - else + result.icon = value->readable() ? m_muteFadedIconData : m_unMuteFadedIconData; + + ScenePlug::ScenePath currentPath( scenePath->names() ); + while( !currentPath.empty() ) { - result.icon = m_muteUndefinedIconData; + pathScope.setPath( ¤tPath ); + auto a = scenePath->getScene()->attributesPlug()->getValue(); + if( a->member( "light:mute" ) ) + { + result.toolTip = new StringData( "Inherited from : " + ScenePlug::pathToString( currentPath ) ); + break; + } + currentPath.pop_back(); } } } + if( !result.icon ) + { + // Use a transparent icon to reserve space in the UI. Without this, + // the top row will resize when setting the mute value, causing a full + // table resize. + if( path.isEmpty() ) + { + result.icon = m_muteBlankIconName; + } + else + { + result.icon = m_muteUndefinedIconData; + } + } result.value = nullptr; if( auto toolTipData = runTimeCast( result.toolTip ) ) From 830926a2976c0be56ae5a3cce481c94ba73cf4cd Mon Sep 17 00:00:00 2001 From: Murray Stevenson <50844517+murraystevenson@users.noreply.github.com> Date: Tue, 9 Jul 2024 18:07:31 -0700 Subject: [PATCH 3/7] SetMembershipInspector : Use fallbackValue to differentiate matches Rather than returning the PathMatcher::Result as the inspected value, we instead implement fallbackValue to allow us to return true from `value()` for an ExactMatch or true from `fallbackValue()` for an AncestorMatch. This aligns the SetMembershipInspector behaviour more closely with the AttributeInspector, where `fallbackValue()` returns inherited attribute values when no attribute exists at the location, and will help us to reduce the amount of special handling when dealing with each inspector in the future. We can also simplify the SetMembershipColumn tooltip handling a little here, as InspectorColumn will already be adding "Double-click to toggle" to the tooltip based on the BoolData now returned by the SetMembershipInspector. --- .../Private/SetMembershipInspector.h | 1 + .../SetMembershipInspectorTest.py | 18 +++++++- src/GafferSceneUI/SetMembershipInspector.cpp | 14 +++++- .../LightEditorBinding.cpp | 46 +++++++++---------- 4 files changed, 53 insertions(+), 26 deletions(-) diff --git a/include/GafferSceneUI/Private/SetMembershipInspector.h b/include/GafferSceneUI/Private/SetMembershipInspector.h index 81f2c36f45b..3289b05c673 100644 --- a/include/GafferSceneUI/Private/SetMembershipInspector.h +++ b/include/GafferSceneUI/Private/SetMembershipInspector.h @@ -81,6 +81,7 @@ class GAFFERSCENEUI_API SetMembershipInspector : public Inspector /// those are found. Gaffer::ValuePlugPtr source( const GafferScene::SceneAlgo::History *history, std::string &editWarning ) const override; EditFunctionOrFailure editFunction( Gaffer::EditScope *scope, const GafferScene::SceneAlgo::History *history ) const override; + IECore::ConstObjectPtr fallbackValue( const GafferScene::SceneAlgo::History *history ) const override; private : diff --git a/python/GafferSceneUITest/SetMembershipInspectorTest.py b/python/GafferSceneUITest/SetMembershipInspectorTest.py index a1080818a3e..4ff50b6ec23 100644 --- a/python/GafferSceneUITest/SetMembershipInspectorTest.py +++ b/python/GafferSceneUITest/SetMembershipInspectorTest.py @@ -110,7 +110,21 @@ def testValue( self ) : self.assertEqual( self.__inspect( plane["out"], "/plane", "planeSet" ).value().value, - IECore.PathMatcher.Result.ExactMatch + True + ) + + def testFallbackValue( self ) : + + plane = GafferScene.Plane() + group = GafferScene.Group() + group["sets"].setValue( "planeSet" ) + group["in"][0].setInput( plane["out"] ) + + inspection = self.__inspect( group["out"], "/group/plane", "planeSet" ) + self.assertEqual( inspection.value().value, True ) + self.assertEqual( + inspection.sourceType(), + GafferSceneUI.Private.Inspector.Result.SourceType.Fallback ) def testSourceAndEdits( self ) : @@ -346,7 +360,7 @@ def testObjectSourceFallback( self ) : self.__assertExpectedResult( self.__inspect( plane["out"], "/plane", "planeSet" ), source = plane["sets"], - sourceType = GafferSceneUI.Private.Inspector.Result.SourceType.Other, + sourceType = GafferSceneUI.Private.Inspector.Result.SourceType.Fallback, editable = True ) diff --git a/src/GafferSceneUI/SetMembershipInspector.cpp b/src/GafferSceneUI/SetMembershipInspector.cpp index 894fdf4bcc3..3116c95b931 100644 --- a/src/GafferSceneUI/SetMembershipInspector.cpp +++ b/src/GafferSceneUI/SetMembershipInspector.cpp @@ -211,7 +211,19 @@ IECore::ConstObjectPtr SetMembershipInspector::value( const GafferScene::SceneAl auto matchResult = (PathMatcher::Result)setMembers->readable().match( path ); - return new IntData( matchResult ); + // Return nullptr for non-exact match so `fallbackValue()` has the opportunity to provide + // an AncestorMatch. + return matchResult & IECore::PathMatcher::Result::ExactMatch ? new BoolData( true ) : nullptr; +} + +IECore::ConstObjectPtr SetMembershipInspector::fallbackValue( const GafferScene::SceneAlgo::History *history ) const +{ + const auto &path = history->context->get( ScenePlug::scenePathContextName ); + ConstPathMatcherDataPtr setMembers = history->scene->set( m_setName ); + + auto matchResult = (PathMatcher::Result)setMembers->readable().match( path ); + + return new BoolData( matchResult & IECore::PathMatcher::Result::AncestorMatch ); } Gaffer::ValuePlugPtr SetMembershipInspector::source( const GafferScene::SceneAlgo::History *history, std::string &editWarning ) const diff --git a/src/GafferSceneUIModule/LightEditorBinding.cpp b/src/GafferSceneUIModule/LightEditorBinding.cpp index 292bedb2b25..01c65e66acb 100644 --- a/src/GafferSceneUIModule/LightEditorBinding.cpp +++ b/src/GafferSceneUIModule/LightEditorBinding.cpp @@ -417,44 +417,44 @@ class SetMembershipColumn : public InspectorColumn return result; } - std::string toolTip; - if( auto toolTipData = runTimeCast( result.toolTip ) ) - { - toolTip = toolTipData->readable(); - } - - if( auto value = runTimeCast( result.value ) ) + if( auto value = runTimeCast( result.value ) ) { - if( value->readable() & PathMatcher::Result::ExactMatch ) - { - result.icon = m_setMemberIconData; - } - else if( value->readable() & PathMatcher::Result::AncestorMatch ) + if( value->readable() ) { - ConstPathMatcherDataPtr setMembersData = scenePath->getScene()->set( m_setName ); - const PathMatcher &setMembers = setMembersData->readable(); + ScenePlug::PathScope pathScope( scenePath->getContext(), &scenePath->names() ); + pathScope.setCanceller( canceller ); - ScenePlug::ScenePath currentPath( scenePath->names() ); - while( !currentPath.empty() ) + Inspector::ConstResultPtr inspectorResult = inspector()->inspect(); + if( inspectorResult->sourceType() != Inspector::Result::SourceType::Fallback ) + { + result.icon = m_setMemberIconData; + } + else { - if( setMembers.match( currentPath ) & PathMatcher::Result::ExactMatch ) + result.icon = m_setMemberIconFadedData; + + ConstPathMatcherDataPtr setMembersData = scenePath->getScene()->set( m_setName ); + const PathMatcher &setMembers = setMembersData->readable(); + + ScenePlug::ScenePath currentPath( scenePath->names() ); + while( !currentPath.empty() ) { - result.icon = m_setMemberIconFadedData; - toolTip = "Inherited from : " + ScenePlug::pathToString( currentPath ); - break; + if( setMembers.match( currentPath ) & PathMatcher::Result::ExactMatch ) + { + result.toolTip = new StringData( "Inherited from : " + ScenePlug::pathToString( currentPath ) + "\n\nDouble-click to toggle" ); + break; + } + currentPath.pop_back(); } - currentPath.pop_back(); } } } - if( !result.icon ) { result.icon = m_setMemberUndefinedIconData; } result.value = nullptr; - result.toolTip = new StringData( toolTip + ( toolTip.size() ? "\n\n" : "" ) + "Double-click to toggle" ); return result; } From 79aafbaa302620c8733a93dea5ff3a98180be9ad Mon Sep 17 00:00:00 2001 From: Murray Stevenson <50844517+murraystevenson@users.noreply.github.com> Date: Thu, 11 Jul 2024 12:05:36 -0700 Subject: [PATCH 4/7] LightEditor : Improve display of fallback values By using the same dimmed foreground colour for fallback values as used by the RenderPassEditor. This does introduce a small amount of additional duplication with the RenderPassEditor's OptionInspectorColumn, but this will be fairly short lived as we'll be replacing both of these columns with a single common InspectorColumn for Gaffer 1.5. --- src/GafferSceneUIModule/LightEditorBinding.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/GafferSceneUIModule/LightEditorBinding.cpp b/src/GafferSceneUIModule/LightEditorBinding.cpp index 01c65e66acb..f6275f1a306 100644 --- a/src/GafferSceneUIModule/LightEditorBinding.cpp +++ b/src/GafferSceneUIModule/LightEditorBinding.cpp @@ -177,6 +177,7 @@ const boost::container::flat_map g_sourceTypeColors = { (int)Inspector::Result::SourceType::Other, nullptr }, { (int)Inspector::Result::SourceType::Fallback, nullptr }, }; +const Color4fDataPtr g_fallbackValueForegroundColor = new Color4fData( Imath::Color4f( 163, 163, 163, 255 ) / 255.0f ); class InspectorColumn : public PathColumn { @@ -221,7 +222,12 @@ class InspectorColumn : public PathColumn result.icon = runTimeCast( inspectorResult->value() ); result.background = g_sourceTypeColors.at( (int)inspectorResult->sourceType() ); std::string toolTip; - if( auto source = inspectorResult->source() ) + if( inspectorResult->sourceType() == Inspector::Result::SourceType::Fallback ) + { + toolTip = "Source : Fallback value"; + result.foreground = g_fallbackValueForegroundColor; + } + else if( auto source = inspectorResult->source() ) { toolTip = "Source : " + source->relativeName( source->ancestor() ); } From da9d731a0eedd820dc8cdf783c523ff8245293b0 Mon Sep 17 00:00:00 2001 From: Murray Stevenson <50844517+murraystevenson@users.noreply.github.com> Date: Tue, 9 Jul 2024 18:09:53 -0700 Subject: [PATCH 5/7] LightEditor : Toggle based on inspected value Now that the AttributeInspector returns inherited attribute values as a fallback, and the SetMembershipInspector is returning True when the path is a set member or the descendant of a set member, we can remove this special handling and simply toggle based on the inspected values. --- python/GafferSceneUI/LightEditor.py | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/python/GafferSceneUI/LightEditor.py b/python/GafferSceneUI/LightEditor.py index d59dc072838..06442fbaa0e 100644 --- a/python/GafferSceneUI/LightEditor.py +++ b/python/GafferSceneUI/LightEditor.py @@ -458,26 +458,7 @@ def __toggleBoolean( self, inspectors, inspections ) : # First we need to find out what the new value would be for each plug in isolation. for inspector, pathInspections in inspectors.items() : for path, inspection in pathInspections.items() : - currentValue = inspection.value().value if inspection.value() is not None else None - - if isinstance( inspector, GafferSceneUI.Private.AttributeInspector ) : - fullAttributes = self.__settingsNode["in"].fullAttributes( path[:-1] ) - parentValueData = fullAttributes.get( inspector.name(), None ) - parentValue = parentValueData.value if parentValueData is not None else False - - currentValues.append( currentValue if currentValue is not None else parentValue ) - elif isinstance( inspector, GafferSceneUI.Private.SetMembershipInspector ) : - if currentValue is not None : - currentValues.append( - True if currentValue & ( - IECore.PathMatcher.Result.ExactMatch | - IECore.PathMatcher.Result.AncestorMatch - ) else False - ) - else : - currentValues.append( False ) - else : - currentValues.append( currentValue ) + currentValues.append( inspection.value().value if inspection.value() is not None else False ) # Now set the value for all plugs, defaulting to `True` if they are not # currently all the same. From 46786d59ad2fa019021c5cd6314853bc2cc4d928 Mon Sep 17 00:00:00 2001 From: Murray Stevenson <50844517+murraystevenson@users.noreply.github.com> Date: Fri, 12 Jul 2024 13:22:23 -0700 Subject: [PATCH 6/7] HistoryWindow : Improve presentation of fallback values Improve the display of fallback values by replacing the SetMembershipInspector specific `_SetMembershipColumn` with a more generic `_ValueColumn` able to present fallback values provided by the inspectors via the new `history:fallbackValue` HistoryPath property. In the process we lose the "green dot" icon presentation of set membership, but we're already not presenting `light:mute` attributes with their "red dot" icon so this seems a reasonable tradeoff. There's a future where we may be able to start configuring this with metadata specifying icons. There are further improvements that could be made to the HistoryPath itself, such as including entries when values change so it'd be possible to view changes to inherited attributes as part of the history, and making it more apparent that a plug in the history belongs to a disabled node, though these are left to be tackled at another time... --- Changes.md | 1 + include/GafferSceneUI/Private/Inspector.h | 4 +- python/GafferSceneUI/_HistoryWindow.py | 26 ++++--- python/GafferSceneUITest/HistoryPathTest.py | 77 +++++++++++++++++++++ src/GafferSceneUI/Inspector.cpp | 7 ++ 5 files changed, 102 insertions(+), 13 deletions(-) diff --git a/Changes.md b/Changes.md index a8dfcbc5e20..3ca8fb7b1a5 100644 --- a/Changes.md +++ b/Changes.md @@ -5,6 +5,7 @@ Improvements ------------ - LightEditor : Values of inherited attributes are now displayed in the Light Editor. These are presented as dimmed "fallback" values. +- LightEditor, RenderPassEditor : Fallback values shown in the history window are displayed with the same dimmed text colour used for fallback values in editor columns. 1.4.9.0 (relative to 1.4.8.0) ======= diff --git a/include/GafferSceneUI/Private/Inspector.h b/include/GafferSceneUI/Private/Inspector.h index c0c1d8e46e4..372e3186d7e 100644 --- a/include/GafferSceneUI/Private/Inspector.h +++ b/include/GafferSceneUI/Private/Inspector.h @@ -126,8 +126,8 @@ class GAFFERSCENEUI_API Inspector : public IECore::RefCounted, public Gaffer::Si /// Returns a `Path` representing the history for the inspected property /// in the current context. The path has a child for each predecessor in - /// the history, and properties `history:value`, `history:operation`, - /// `history:source`, `history:editWarning` and `history:node`. + /// the history, and properties `history:value`, `history:fallbackValue`, + /// `history:operation`, `history:source`, `history:editWarning` and `history:node`. Gaffer::PathPtr historyPath(); protected : diff --git a/python/GafferSceneUI/_HistoryWindow.py b/python/GafferSceneUI/_HistoryWindow.py index efc5ee31385..498994dbe45 100644 --- a/python/GafferSceneUI/_HistoryWindow.py +++ b/python/GafferSceneUI/_HistoryWindow.py @@ -34,7 +34,7 @@ # ########################################################################## -import IECore +import imath import Gaffer import GafferUI @@ -98,28 +98,34 @@ def headerData( self, canceller = None ) : return self.CellData( self.__title ) -# \todo This duplicates logic from (in this case) `_GafferSceneUI._LightEditorSetMembershipColumn`. +# \todo This duplicates logic from (in this case) `_GafferSceneUI._LightEditorInspectorColumn`. # Refactor to allow calling `_GafferSceneUI.InspectorColumn.cellData()` from `_HistoryWindow` to # remove this duplication for columns that customize their value presentation. -class _SetMembershipColumn( GafferUI.PathColumn ) : +class _ValueColumn( GafferUI.PathColumn ) : - def __init__( self, title, property ) : + def __init__( self, title, property, fallbackProperty ) : GafferUI.PathColumn.__init__( self ) self.__title = title self.__property = property + self.__fallbackProperty = fallbackProperty def cellData( self, path, canceller = None ) : cellValue = path.property( self.__property ) + fallbackValue = path.property( self.__fallbackProperty ) data = self.CellData() - if cellValue & IECore.PathMatcher.Result.ExactMatch : - data.icon = "setMember.png" - elif cellValue & IECore.PathMatcher.Result.AncestorMatch : - data.icon = "setMemberFaded.png" + if cellValue is not None : + data.value = cellValue + elif fallbackValue is not None : + data.value = fallbackValue + data.foreground = imath.Color4f( 0.64, 0.64, 0.64, 1.0 ) + + if isinstance( data.value, ( imath.Color3f, imath.Color4f ) ) : + data.icon = data.value return data @@ -147,9 +153,7 @@ def __init__( self, inspector, scenePath, context, scriptNode, title=None, **kw Gaffer.DictPath( {}, "/" ), columns = ( _NodeNameColumn( "Node", "history:node", self.__scriptNode ), - _SetMembershipColumn( "Value", "history:value" ) if ( - isinstance( inspector, GafferSceneUI.Private.SetMembershipInspector ) - ) else GafferUI.PathListingWidget.StandardColumn( "Value", "history:value" ), + _ValueColumn( "Value", "history:value", "history:fallbackValue" ), _OperationIconColumn( "Operation", "history:operation" ), ), sortable = False, diff --git a/python/GafferSceneUITest/HistoryPathTest.py b/python/GafferSceneUITest/HistoryPathTest.py index b620dd775eb..e7a55d1de1b 100644 --- a/python/GafferSceneUITest/HistoryPathTest.py +++ b/python/GafferSceneUITest/HistoryPathTest.py @@ -201,6 +201,7 @@ def testPropertyNames( self ) : "fullName", "name", "history:value", + "history:fallbackValue", "history:operation", "history:source", "history:editWarning", @@ -247,6 +248,7 @@ def testProperties( self ) : self.assertEqual( c[0].property( "name" ), str( c[0][-1] ) ) self.assertEqual( c[0].property( "history:node" ), s["testLight"] ) self.assertEqual( c[0].property( "history:value" ), 0.0 ) + self.assertEqual( c[0].property( "history:fallbackValue" ), None ) self.assertEqual( c[0].property( "history:operation" ), Gaffer.TweakPlug.Mode.Create ) self.assertEqual( c[0].property( "history:source" ), s["testLight"]["parameters"]["exposure"] ) self.assertEqual( c[0].property( "history:editWarning" ), "" ) @@ -254,6 +256,7 @@ def testProperties( self ) : self.assertEqual( c[1].property( "name" ), str( c[1][-1] ) ) self.assertEqual( c[1].property( "history:node" ), s["tweaks"] ) self.assertEqual( c[1].property( "history:value" ), 2.0 ) + self.assertEqual( c[1].property( "history:fallbackValue" ), None ) self.assertEqual( c[1].property( "history:operation" ), Gaffer.TweakPlug.Mode.Add ) self.assertEqual( c[1].property( "history:source" ), exposureTweak ) self.assertEqual( c[1].property( "history:editWarning" ), "" ) @@ -261,6 +264,7 @@ def testProperties( self ) : self.assertEqual( c[2].property( "name" ), str( c[2][-1] ) ) self.assertEqual( c[2].property( "history:node" ), s["editScope"]["LightEdits"]["ShaderTweaks"] ) self.assertEqual( c[2].property( "history:value" ), 3.0 ) + self.assertEqual( c[2].property( "history:fallbackValue" ), None ) self.assertEqual( c[2].property( "history:operation" ), Gaffer.TweakPlug.Mode.Replace ) self.assertEqual( c[2].property( "history:source" ), edit ) self.assertEqual( c[2].property( "history:editWarning" ), "" ) @@ -354,6 +358,79 @@ def testEmptyHistory( self ) : self.assertEqual( len( historyPath.children() ), 0 ) + def testAttributeFallbackValues( self ) : + + s = Gaffer.ScriptNode() + + s["testLight"] = GafferSceneTest.TestLight() + + s["group"] = GafferScene.Group() + s["group"]["in"][0].setInput( s["testLight"]["out"] ) + + s["groupFilter"] = GafferScene.PathFilter() + s["groupFilter"]["paths"].setValue( IECore.StringVectorData( [ "/group" ] ) ) + + s["tweaks"] = GafferScene.AttributeTweaks() + s["tweaks"]["in"].setInput( s["group"]["out"] ) + s["tweaks"]["filter"].setInput( s["groupFilter"]["out"] ) + + groupTextureResolutionTweak = Gaffer.TweakPlug( "gl:visualiser:maxTextureResolution", 1024 ) + groupTextureResolutionTweak["mode"].setValue( Gaffer.TweakPlug.Mode.Create ) + s["tweaks"]["tweaks"].addChild( groupTextureResolutionTweak ) + + s["lightFilter"] = GafferScene.PathFilter() + s["lightFilter"]["paths"].setValue( IECore.StringVectorData( [ "/group/light" ] ) ) + + s["openGLAttributes"] = GafferScene.OpenGLAttributes() + s["openGLAttributes"]["in"].setInput( s["tweaks"]["out"] ) + s["openGLAttributes"]["filter"].setInput( s["lightFilter"]["out"] ) + s["openGLAttributes"]["attributes"]["visualiserMaxTextureResolution"]["enabled"].setValue( True ) + s["openGLAttributes"]["attributes"]["visualiserMaxTextureResolution"]["value"].setValue( 1536 ) + + inspector = GafferSceneUI.Private.AttributeInspector( + s["openGLAttributes"]["out"], None, "gl:visualiser:maxTextureResolution", + ) + + with Gaffer.Context() as context : + context["scene:path"] = IECore.InternedStringVectorData( [ "group", "light" ] ) + historyPath = inspector.historyPath() + + c = historyPath.children() + + self.assertEqual( c[0].property( "name" ), str( c[0][-1] ) ) + self.assertEqual( c[0].property( "history:node" ), s["openGLAttributes"] ) + self.assertEqual( c[0].property( "history:value" ), 1536 ) + self.assertEqual( c[0].property( "history:fallbackValue" ), 1536 ) + self.assertEqual( c[0].property( "history:operation" ), Gaffer.TweakPlug.Mode.Create ) + self.assertEqual( c[0].property( "history:source" ), s["openGLAttributes"]["attributes"]["visualiserMaxTextureResolution"] ) + self.assertEqual( c[0].property( "history:editWarning" ), "Edits to \"gl:visualiser:maxTextureResolution\" may affect other locations in the scene." ) + + s["openGLAttributes"]["enabled"].setValue( False ) + + with Gaffer.Context() as context : + context["scene:path"] = IECore.InternedStringVectorData( [ "group", "light" ] ) + historyPath = inspector.historyPath() + + c = historyPath.children() + + self.assertEqual( c[0].property( "name" ), str( c[0][-1] ) ) + self.assertEqual( c[0].property( "history:node" ), s["testLight"] ) + self.assertEqual( c[0].property( "history:value" ), None ) + self.assertEqual( c[0].property( "history:fallbackValue" ), None ) + self.assertEqual( c[0].property( "history:operation" ), Gaffer.TweakPlug.Mode.Create ) + self.assertEqual( c[0].property( "history:source" ), s["testLight"]["visualiserAttributes"]["maxTextureResolution"] ) + self.assertEqual( c[0].property( "history:editWarning" ), "" ) + + # Disabling the openGLAttributes node results in its `visualiserMaxTextureResolution` plug remaining + # in the history but providing no value. We'll be able to see the value set on /group as the fallback + # value. + self.assertEqual( c[1].property( "name" ), str( c[1][-1] ) ) + self.assertEqual( c[1].property( "history:node" ), s["openGLAttributes"] ) + self.assertEqual( c[1].property( "history:value" ), None ) + self.assertEqual( c[1].property( "history:fallbackValue" ), 1024 ) + self.assertEqual( c[1].property( "history:operation" ), Gaffer.TweakPlug.Mode.Create ) + self.assertEqual( c[1].property( "history:source" ), s["openGLAttributes"]["attributes"]["visualiserMaxTextureResolution"] ) + self.assertEqual( c[1].property( "history:editWarning" ), "Edits to \"gl:visualiser:maxTextureResolution\" may affect other locations in the scene." ) if __name__ == "__main__": unittest.main() diff --git a/src/GafferSceneUI/Inspector.cpp b/src/GafferSceneUI/Inspector.cpp index 4de806889d1..1ba1531e28f 100644 --- a/src/GafferSceneUI/Inspector.cpp +++ b/src/GafferSceneUI/Inspector.cpp @@ -56,6 +56,7 @@ using namespace GafferSceneUI::Private; using ConstPredecessors = std::vector; static InternedString g_valuePropertyName( "history:value" ); +static InternedString g_fallbackValuePropertyName( "history:fallbackValue" ); static InternedString g_operationPropertyName( "history:operation" ); static InternedString g_sourcePropertyName( "history:source" ); static InternedString g_editWarningPropertyName( "history:editWarning" ); @@ -441,6 +442,7 @@ void Inspector::HistoryPath::propertyNames( std::vector &names, if( isLeaf() ) { names.push_back( g_valuePropertyName ); + names.push_back( g_fallbackValuePropertyName ); names.push_back( g_operationPropertyName); names.push_back( g_sourcePropertyName ); names.push_back( g_editWarningPropertyName ); @@ -466,6 +468,7 @@ ConstRunTimeTypedPtr Inspector::HistoryPath::property( const InternedString &nam if( isLeaf() && ( name == g_valuePropertyName || + name == g_fallbackValuePropertyName || name == g_operationPropertyName || name == g_sourcePropertyName || name == g_editWarningPropertyName @@ -486,6 +489,10 @@ ConstRunTimeTypedPtr Inspector::HistoryPath::property( const InternedString &nam { return runTimeCast( m_inspector->value( it->history.get() ) ); } + else if( name == g_fallbackValuePropertyName ) + { + return runTimeCast( m_inspector->fallbackValue( it->history.get() ) ); + } else if( name == g_operationPropertyName ) { if( auto tweakPlug = runTimeCast( source ) ) From ce53e1f376234297c2a449343a48de8a7f4dc56d Mon Sep 17 00:00:00 2001 From: Murray Stevenson <50844517+murraystevenson@users.noreply.github.com> Date: Thu, 18 Jul 2024 12:47:14 -0700 Subject: [PATCH 7/7] LightEditor : Add todos --- src/GafferSceneUIModule/LightEditorBinding.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/GafferSceneUIModule/LightEditorBinding.cpp b/src/GafferSceneUIModule/LightEditorBinding.cpp index f6275f1a306..fcb077c8044 100644 --- a/src/GafferSceneUIModule/LightEditorBinding.cpp +++ b/src/GafferSceneUIModule/LightEditorBinding.cpp @@ -274,6 +274,8 @@ class InspectorColumn : public PathColumn }; +/// \todo `MuteColumn` and `SetMembershipColumn` should not exist and we intend +/// to continue refactoring until it is possible to remove them entirely. class MuteColumn : public InspectorColumn { @@ -340,6 +342,8 @@ class MuteColumn : public InspectorColumn } result.value = nullptr; + /// \todo Remove this once AttributeInspector can provide a default value when + /// no attribute exists, then InspectorColumn would always provide the toggle tooltip. if( auto toolTipData = runTimeCast( result.toolTip ) ) { std::string toolTip = toolTipData->readable();