Reconsider Dispatcher::setupPlugs() mechanism

[johnhaddon]

Currently, Dispatchers are given the opportunity of adding custom plugs to ExecutableNodes, so that they can add dispatcher-specific settings to each node. For instance, a farm dispatcher might add plugs to control resource requirements like machine type or license reservations. This is achieved by the ExecutableNode constructor calling Dispatcher::setupPlugs(), but has a couple of problems :

• The ExecutableNode isn't fully constructed at the point that Dispatcher::doSetupPlugs() gets called, so node->typeId() will always return ExecutableNodeTypeId, and the dispatcher can't choose what plugs to add based on node type.
• When an ExecutableNode is constructed while loading a file, a dispatcher might want to remove plugs that are no longer relevant, but during loading any dynamic plugs are added after construction, and therefore Dispatcher::doSetupPlugs() doesn't see them.

We need to consider how best to resolve this.

[andrewkaufman]

So this has just bit me on #871. I was trying to add a batchSize plug in setupPlugs(), except when an ExecutableNode defines requiresSequenceExecution() == true (see #870 for details on that method). Since the node isn't fully constructed, it's not picking up the re-implemented requiresSequenceExecution(). Any ideas on how we can sort this all out?

[johnhaddon]

Nothing I'm totally happy with, but here's a couple of options to mull over :

1. Call setupPlugs() in ExecutableNode::parentChanging() the first time we get a parent, because we know we'll be fully constructed by then. Pros - easy. Cons - forces people to use a ScriptNode if they're just chucking a little network together in a pipeline script, maybe others I can't think of right now.
2. Call setupPlugs() at the end of the constructor for the most derived class. Pros - more correct, will always work. Cons - requires more work by ExecutableNode authors. For any nodes which might be used directly or as a base class, we'd need a public constructor which does call setupPlugs(), and a protected one for derived classes which doesn't call setupPlugs(). Although I suppose calling setupPlugs() more than once isn't technically an error, it's just a bit wasteful and ugly.

[andrewkaufman]

Dispatcher::dispatch() requires using a ScriptNode already, and since these are all Dispatcher plugs we're setting up, it doesn't seem so bad that setupPlugs() requires a ScriptNode too. Just to clarify the parentChanging idea, would this still be valid:

n = Gaffer.SomeExecutableNode()
## do some other stuff to n before adding to a script
script["n"] = n # is this when setupPlugs runs?
dispatcher.dispatch( script["n"] )

[andrewkaufman]

I tried implementing ExecutableNode::parentChanging() and it looks like the old parent is already null by the time parentChanging() gets called. Is that expected? If so, how do I make sure to only call setupPlugs() the first time I get a parent?

[johnhaddon]

To answer your original question, yes that would be OK.

I've put up a pull request (#937) with a fix to ExecutableNode::parentChanging(). With that you should now be able to do !parent<GraphComponent>() && newParent to tell if you're going from having no parent to having a parent.

I'm not sure I like this approach though - it has the same problem with dynamic plugs as the current implementation, in that plugs will be added during construction when loading a file, and then any previously saved dynamic plugs will get added over the top (and renamed in the process). This is a problem with either 1) or 2).

What do you think to making it necessary to make an explicit call to setupPlugs() outside the node? We'd do that in the menu items that create them, and have a serialiser that puts the call in at the end of the .gfr script so it happened during load. Too much burden on scripters who are making nodes programmatically?

[andrewkaufman]

The explicit call seems a bit too fiddly for scripters to need to remember... if we're serializing it into the .gfr, isn't that basically a postConstructor?

[johnhaddon]

Yeah, I'm not really happy with any of my ideas so far. When you say a postConstructor, what do you mean exactly?

[andrewkaufman]

Doesn't Maya have a concept like that? It's a function that is called right after the constructor when it's safe to call member functions of the derived class. I have no idea how they implemented it, but it's kind of what we're after, isn't it?

[andrewkaufman]

Wait, I guess that doesn't fix the dynamic plug problem... we need setupPlugs() to run after all the dynamic plugs have been added, don't we?

[johnhaddon]

The problem with a post constructor is that coming up with a mechanism for it is just as hard as coming up with a mechanism for what we're discussing anyway, except it would affect /everything/. The only reliable way I can think of would be to have no public constructors, and to force all construction to go through factory methods (so no new MyNode, only MyNode::create() or GraphComponent::create( "MyNode" ). I'm keen that the basics in Gaffer remain more lightweight than that, and in any case I think it's too big a change to be justified by this alone…

And yep, post constructors wouldn't be the right thing for this anyway, if we want dynamic plugs to be an option for dispatchers. The thing certain-third-party-packages might do to deal with that would then be to have an am-i-in-the-course-of-loading? flag that can be queried, and then modify behaviour based on that. But I think having those types of things quickly leads to hard to understand behaviours and questions like "are importing and pasting and duplicating the same as loading?" So I'm keen to stick to more fundamental concepts that are well defined at the library level, like "my parent is changing". That does mean a few awkward situations like this one, but I'd rather that than the even-more-awkward situations I imagine would be introduced by more nebulous concepts.

I actually don't think a postConstructor is one of those nebulous concepts if it's implemented perfectly, but I don't know of a way of doing that without abandoning new MyNode or complicating all constructors so they know whether or not they're the most derived one.

So, I haven't really come up with anything useful there have I? But hopefully I've explained my rationale for preferring a little bit of awkwardness here vs a whole lot of new machinery...

[johnhaddon]

Some further thoughts on this :

• I have completely forgotten why we might want to support dynamic plugs. The best I can come up with is to round-trip Gaffer files through an environment where a particular dispatcher isn't available. But we don't support round-tripping through environments where any other types of shader/plugin are unavailable so I don't see why this should be any different. Do we still have any compelling reasons to want dynamic plugs here?
• I think a postConstructor is now a real possibility, via the following avenue :
  • Add a makeIntrusive() method to IECore, in the spirit of make_shared.
  • Make all RefCounted (and derived classes) constructors private but friends of makeIntrusive(). This is worthwhile in its own right, in that nobody can construct a RefCounted class without correctly assigning to an intrusive_ptr().
  • With all construction going through makeIntrusive() we now have a single place where we can call a post constructor.
  • The situation is a bit trickier in Python, where we want to postpone the postConstructor call until after __init__, but the DependencyNode custom metaclass provides us with a template for how to do that.