Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PathFilterUI : Allow Select Affected Objects on promoted Spreadsheets #6205

Conversation

murraystevenson
Copy link
Contributor

This can be particularly useful for investigating paths on Processors inside of EditScope nodes.

@murraystevenson murraystevenson self-assigned this Jan 11, 2025
@johnhaddon
Copy link
Member

johnhaddon commented Jan 13, 2025

Thanks Murray. This seems to be working as advertised for promoted spreadsheets, but in this little test example I can't get it to work with a non-promoted spreadsheet (and it seems it didn't work before either). If I right click on the /pla* row name, I do get the menu item, but then when I invoke that item, nothing happens.

import Gaffer
import GafferScene
import imath

Gaffer.Metadata.registerValue( parent, "serialiser:milestoneVersion", 1, persistent=False )
Gaffer.Metadata.registerValue( parent, "serialiser:majorVersion", 4, persistent=False )
Gaffer.Metadata.registerValue( parent, "serialiser:minorVersion", 15, persistent=False )
Gaffer.Metadata.registerValue( parent, "serialiser:patchVersion", 3, persistent=False )

__children = {}

__children["Plane"] = GafferScene.Plane( "Plane" )
parent.addChild( __children["Plane"] )
__children["Plane"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
__children["ShaderAssignment"] = GafferScene.ShaderAssignment( "ShaderAssignment" )
parent.addChild( __children["ShaderAssignment"] )
__children["ShaderAssignment"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
__children["PathFilter"] = GafferScene.PathFilter( "PathFilter" )
parent.addChild( __children["PathFilter"] )
__children["PathFilter"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
__children["PathFilterSpreadsheet"] = Gaffer.Spreadsheet( "PathFilterSpreadsheet" )
parent.addChild( __children["PathFilterSpreadsheet"] )
__children["PathFilterSpreadsheet"]["rows"].addRows( 1 )
__children["PathFilterSpreadsheet"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
__children["Plane"]["__uiPosition"].setValue( imath.V2f( -8.90000057, 4.4000001 ) )
__children["ShaderAssignment"]["in"].setInput( __children["Plane"]["out"] )
__children["ShaderAssignment"]["filter"].setInput( __children["PathFilter"]["out"] )
__children["ShaderAssignment"]["__uiPosition"].setValue( imath.V2f( -8.90000057, -3.7640624 ) )
__children["PathFilter"]["paths"].setInput( __children["PathFilterSpreadsheet"]["enabledRowNames"] )
__children["PathFilter"]["__uiPosition"].setValue( imath.V2f( 12.2739754, 8.54897594 ) )
__children["PathFilterSpreadsheet"]["selector"].setValue( '${scene:path}' )
Gaffer.Metadata.registerValue( __children["PathFilterSpreadsheet"]["selector"], 'readOnly', True )
__children["PathFilterSpreadsheet"]["rows"][1]["name"].setValue( '/pla*' )
__children["PathFilterSpreadsheet"]["__uiPosition"].setValue( imath.V2f( 2.0731535, 8.54853249 ) )


del __children


I haven't really followed everything that is going on in __popupMenu() and _selectAffected(), but I wonder if maybe the latter could be simplified a bit, by using work done already by the former?

We also use this as an excuse to simplify `_selectAffected()` and consolidate the spreadsheet handling in `__popupMenu()`.
This appears to have been broken as far back as 1.2.0.0.

File "/opt/gaffer-1.2.0.0-linux/python/GafferSceneUI/PathFilterUI.py", line 213, in __popupMenu
	selection = IECore.PathMatcher( plugValueWidget.vectorDataWidget().getData()[0] )
IndexError: list index out of range
@murraystevenson
Copy link
Contributor Author

Thanks for the feedback! I tracked down the issue in your example and have addressed it in 889c6e6, taking the opportunity to simplify _selectAffected() a bit in the process.

In testing I found another existing bug related to the use of "Select Affected Objects" on cells in Spreadsheet columns created from a PathFilter.paths plug, so I've fixed that as well in f4259cf.

import GafferScene
import IECore
import imath

Gaffer.Metadata.registerValue( parent, "serialiser:milestoneVersion", 1, persistent=False )
Gaffer.Metadata.registerValue( parent, "serialiser:majorVersion", 4, persistent=False )
Gaffer.Metadata.registerValue( parent, "serialiser:minorVersion", 15, persistent=False )
Gaffer.Metadata.registerValue( parent, "serialiser:patchVersion", 3, persistent=False )

__children = {}

__children["PathFilter"] = GafferScene.PathFilter( "PathFilter" )
parent.addChild( __children["PathFilter"] )
__children["PathFilter"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
__children["Sphere1"] = GafferScene.Sphere( "Sphere1" )
parent.addChild( __children["Sphere1"] )
__children["Sphere1"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
__children["CustomAttributes2"] = GafferScene.CustomAttributes( "CustomAttributes2" )
parent.addChild( __children["CustomAttributes2"] )
__children["CustomAttributes2"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
__children["Spreadsheet1"] = Gaffer.Spreadsheet( "Spreadsheet1" )
parent.addChild( __children["Spreadsheet1"] )
__children["Spreadsheet1"]["rows"].addColumn( Gaffer.StringVectorDataPlug( "paths", defaultValue = IECore.StringVectorData( [  ] ), ) )
__children["Spreadsheet1"]["rows"].addRows( 3 )
__children["Spreadsheet1"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
__children["Plane2"] = GafferScene.Plane( "Plane2" )
parent.addChild( __children["Plane2"] )
__children["Plane2"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
__children["Parent1"] = GafferScene.Parent( "Parent1" )
parent.addChild( __children["Parent1"] )
__children["Parent1"]["children"].addChild( GafferScene.ScenePlug( "child1", flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
__children["Parent1"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
__children["PathFilter"]["paths"].setInput( __children["Spreadsheet1"]["out"]["paths"] )
__children["PathFilter"]["__uiPosition"].setValue( imath.V2f( 75.0779037, -7.76982832 ) )
__children["Sphere1"]["transform"]["translate"].setValue( imath.V3f( -2.50736856, 0, 0 ) )
__children["Sphere1"]["__uiPosition"].setValue( imath.V2f( 48.348156, 1.74329376 ) )
__children["CustomAttributes2"]["in"].setInput( __children["Parent1"]["out"] )
__children["CustomAttributes2"]["filter"].setInput( __children["PathFilter"]["out"] )
__children["CustomAttributes2"]["__uiPosition"].setValue( imath.V2f( 50.598156, -14.5848312 ) )
__children["Spreadsheet1"]["rows"][0]["cells"]["paths"]["value"].setValue( IECore.StringVectorData( [ '/sph*' ] ) )
__children["Spreadsheet1"]["rows"][1]["cells"]["paths"]["value"].setValue( IECore.StringVectorData( [ '/sph*', '/pla*' ] ) )
__children["Spreadsheet1"]["rows"][2]["cells"]["paths"]["value"].setValue( IECore.StringVectorData( [ '/sph*' ] ) )
__children["Spreadsheet1"]["rows"][3]["cells"]["paths"]["value"].setValue( IECore.StringVectorData( [ '/pla*' ] ) )
Gaffer.Metadata.registerValue( __children["Spreadsheet1"]["rows"][0]["cells"]["paths"]["value"], 'ui:scene:acceptsSetName:promotable', False )
Gaffer.Metadata.registerValue( __children["Spreadsheet1"]["rows"][0]["cells"]["paths"]["value"], 'ui:scene:acceptsSetNames:promotable', False )
Gaffer.Metadata.registerValue( __children["Spreadsheet1"]["rows"][0]["cells"]["paths"]["value"], 'ui:scene:acceptsSetExpression:promotable', False )
Gaffer.Metadata.registerValue( __children["Spreadsheet1"]["rows"][0]["cells"]["paths"]["value"], 'description', "The list of paths to the locations to be matched by the filter.\nA path is formed by a sequence of names separated by `/`, and\nspecifies the hierarchical position of a location within the scene.\nPaths may use Gaffer's standard wildcard characters to match\nmultiple locations.\n\nThe `*` wildcard matches any sequence of characters within\nan individual name, but never matches across names separated\nby a `/`.\n\n - `/robot/*Arm` matches `/robot/leftArm`, `/robot/rightArm` and\n   `/robot/Arm`. But does not match `/robot/limbs/leftArm` or\n   `/robot/arm`.\n\nThe `...` wildcard matches any sequence of names, and can be\nused to match locations no matter where they are parented in\nthe hierarchy.\n\n - `/.../house` matches `/house`, `/street/house` and `/city/street/house`." )
Gaffer.Metadata.registerValue( __children["Spreadsheet1"]["rows"][0]["cells"]["paths"]["value"], 'nodule:type', '' )
Gaffer.Metadata.registerValue( __children["Spreadsheet1"]["rows"][0]["cells"]["paths"]["value"], 'ui:scene:acceptsPaths', True )
Gaffer.Metadata.registerValue( __children["Spreadsheet1"]["rows"][0]["cells"]["paths"]["value"], 'vectorDataPlugValueWidget:dragPointer', 'objects' )
Gaffer.Metadata.registerValue( __children["Spreadsheet1"]["rows"][0]["cells"]["paths"]["value"], 'plugValueWidget:type', 'GafferSceneUI.PathFilterUI._PathsPlugValueWidget' )
__children["Spreadsheet1"]["__uiPosition"].setValue( imath.V2f( 66.2778931, -7.77065039 ) )
__children["Plane2"]["__uiPosition"].setValue( imath.V2f( 61.3122597, 1.74329376 ) )
__children["Parent1"]["in"].setInput( __children["Sphere1"]["out"] )
__children["Parent1"]["parent"].setValue( '/' )
__children["Parent1"]["children"][0].setInput( __children["Plane2"]["out"] )
__children["Parent1"]["__uiPosition"].setValue( imath.V2f( 50.598156, -6.42076874 ) )


del __children

This was confusing for several reasons :

- Several callers passed `targetPlug` to the `plug` argument, but then internally `targetPlug` was acquired again separately.
- The `plug` argument could be either a StringVectorDataPlug used to find a PathFilter, or a ScenePlug, but this wasn't documented.

Now, `_selectedAffected()` is much dumber, and just takes a PathMatcher and a list of ScenePlugs, and selects the matching paths. All logic about where the scenes and PathMatchers come from is now on the client side.
Since the refactoring of `_selectedAffected()` we are no longer using the plug, and only care about finding a PathFilter. So we replace `_targetFilterPlug()` with a more descriptive `_destinationPathFilter()`. This removes the last use of the could-mean-anything `targetPlug` variables.
@johnhaddon
Copy link
Member

Thanks Murray - all your fixes are working nicely for me. For some reason, I still found this code very hard to read, I think mostly due to the vague and inconsistent usage of targetPlug inherited from the original code. I've pushed a couple of commits which are my attempt to add a bit of clarity as I wrapped my head around the code. Could you see what you think to those and then merge either with or without as you see fit?

@murraystevenson
Copy link
Contributor Author

Thanks John, those commits are a definite improvement in clarity and do pass the majority of my test cases. Where they didn't pass was in the simple case of a disconnected PathFilter where _filteredScenes() returns an empty list.

I would prefer to go forward with your changes so I've pushed a commit that better handles that error case, and would be happy to see this merged if my changes make sense to you.

@johnhaddon johnhaddon merged commit 9508c0e into GafferHQ:1.4_maintenance Jan 17, 2025
5 checks passed
@johnhaddon
Copy link
Member

Thanks for the additional fix - I've gone ahead and merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants