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

Added Gaffer.NodeAlgo.applyUserDefaults() #1045

Merged
merged 1 commit into from Oct 15, 2014

Conversation

andrewkaufman
Copy link
Contributor

Nodes created via the NodeMenu apply default values to their plugs, using the userDefault key in the Metadata.

Fixes #1038.

Nodes created via the NodeMenu apply default values to their plugs, using the "userDefault" key in the Metadata.

Fixes GafferHQ#1038.
@johnhaddon
Copy link
Member

The other algo functions implemented in C++ aren't namespaced like this, so for example it's GafferScene::outputCamera() rather than GafferScene::RendererAlgo::outputCamera(). I think we should either follow suit here or namespace the other algo files. Any preference?

@andrewkaufman
Copy link
Contributor Author

I guess if the modules stay small and dedicated topics, as in your GafferCortex and GafferDispatch refactor, then we won't pollute the module namespaces with too many of these little functions. On the other hand, I certainly don't like the look of Gaffer.applyUserDefaults as it seems quite ambiguous. I know the arg is called node, but that isn't really evident when doing a dir()...

@andrewkaufman
Copy link
Contributor Author

Should it maybe be namespaced as Gaffer.Node.applyUserDefaults? Is there even a nice way of doing that?

@johnhaddon
Copy link
Member

You could inject it in with Gaffer.Node.applyUserDefaults = myFunctionWotIDeclaredEarlier. But that wouldn't work if we wanted to reimplement in C++, unless we actually put the code on the Node class itself. One of the main rationales for the algo files is to keep the core classes slim and easy to understand, and to provide less commonly used functionality separately.

I agree that Gaffer.applyUserDefaults() doesn't sound specific enough though - maybe GafferGraph.applyUserDefaults() would if we had that module? In the meantime though maybe I should just stop nitpicking and merge it, and we can see how it feels in practice, and how much more NodeAlgo we come up with in future?

@andrewkaufman
Copy link
Contributor Author

Sounds like a plan.

node = GafferTest.AddNode()

self.assertEqual( node["op1"].getValue(), 0 )
Gaffer.Metadata.registerPlugValue( GafferTest.AddNode.staticTypeId(), "op1", "userDefault", IECore.IntData( 7 ) )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor note for future reference - it's possible to pass just 7 and the IntData will be constructed for you behind the scenes.

johnhaddon added a commit that referenced this pull request Oct 15, 2014
Added Gaffer.NodeAlgo.applyUserDefaults()
@johnhaddon johnhaddon merged commit d5c2bcc into GafferHQ:master Oct 15, 2014
@andrewkaufman andrewkaufman deleted the plugConfig branch October 15, 2014 17:01
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.

Config mechanism for plug defaults
2 participants