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

Exposing access to the DispatcherWindow for a given script. #1033

Merged
merged 2 commits into from Oct 15, 2014

Conversation

andrewkaufman
Copy link
Contributor

Also added a method to set the dispatcher selection. This is so certain apps can choose what dispatcher to start out with. I wasn't really sure if this should be done in this way, or by adding a static Dispatcher *Dispatcher::defaultDispatcher() or similar to Gaffer::Dispatcher itself.

@johnhaddon
Copy link
Member

Having a method to set the dispatcher in a dispatcher window seems entirely reasonable, but it should be setDispatcher()/getDispatcher() to match the accessors we use everywhere else. We can keep the dispatcher() method for backwards compatibility though.

I'm not so sure about exposing dispatcherWindow(), because it has such weird semantics at the moment. It sounds like "acquire me a dispatcher window for this script", but really it's "get that singleton dispatcher window and reparent it to the window for this script". It's also a bit at odds with _DispatcherWindow.setNodesToDispatch() - it would allow me to parent the window to one particular script but then give it nodes from another. I think we need to figure this out better before making it part of the public API.

I like the idea of Dispatcher::getDefaultDispatcher() and Dispatcher:setDefaultDispatcher() though. It would allow pipeline scripts to not have to think about what the appropriate dispatcher was, making them much more portable, and robust to swapping in another dispatcher in the glorious event that one might find oneself changing renderfarm management software. It would also solve the problem where the dispatcher window starts out with "Local", just hoping that it exists - instead it can just start out with the default.

If you just want to be able to set a default and then leave it alone, I'd plump for the Dispatcher::setDefaultDispatcher() route - it seems useful in general. If you need to monkey with the dispatcher window on the fly then I think we need to give some more thought into how that would look...

@johnhaddon
Copy link
Member

Hmm, I think I spoke too soon. I've said in the past that I don't like the registration of dispatcher instances (rather than types) because there's no effective way for two clients to share a dispatcher instance without resetting all the plugs to the default before changing what they want (and there's certainly no way of sharing a dispatcher instance on different threads for that reason). Having a default dispatcher instance just makes that worse. What do you think?

@andrewkaufman
Copy link
Contributor Author

Yeah, that's why I went for the UI approach initially, because I thought you wouldn't like the defaultDispatcher instance.

About setDispatcher(), that seems like it should take a dispatcher instance rather than a label, but I need a way to update the dropdown menu to the label corresponding to the instance, and I'm not sure how that would work, so I went for selectDispatcher() to avoid thinking about it...

For my current use I just want to set the default and let the window manage the rest. I guess in theory some other UI might want to set the dispatcher itself, say if Jabuka wanted to initiate a farm submission, but be able to use the DispatcherWindow directly, rather than making it's own UI, but that would still just be setting the dispatcher once and letting the window manage it afterwards...

In any case, how do you think we resolve this? The driving goal is that Yuta wants Caribou to startup with the FarmDispatcher pre-selected in the window.

@johnhaddon
Copy link
Member

I don't know if this is a can of worms or not, but what if we :

  • Update Dispatcher::registerDispatcher() to be like every other registry we have, taking a function that returns a new Dispatcher each time, rather than being given an instance.
  • Add Dispatcher::setDefaultDispatcher() and Dispatcher::getDefaultDispatcher(), which are just passed one of the names previously registered with registerDispatcher().

@andrewkaufman
Copy link
Contributor Author

I've forced over with the changes we discussed offline.

@johnhaddon
Copy link
Member

Now we have #1038 in the works, what do you think to returning to my idea of making the dispatcher registry hold creators rather than instances, and have setDefaultDispatcher()/getDefaultDispatcher()? I think it'd be a definite improvement, and now would be the time to do it, before we end up with too much stuff relying on the old way.

@andrewkaufman
Copy link
Contributor Author

I don't mind looking into that change as well, but having played a bit with set/getDispatcher on the window itself, I think I like exposing it there anyways, so we're able to switch the currently selected dispatcher via script, even once the window has been opened.

@andrewkaufman
Copy link
Contributor Author

So to clarify, I'd vote to merge this as is (assuming that last commit looks right to you), and address this registry change in #922, which I've added to the current milestone.

@johnhaddon
Copy link
Member

Agreed - I like it too. What do you think to having acquire() take an ApplicationRoot argument, in the same way that Bookmarks.acquire() and NodeMenu.acquire() do? I think it's probably the right thing to do, so that different apps can coexist in the same process, but with different DispatcherWindows.

Added acquire() and get/setDispatcher() methods. Removed dispatcher(), but that shouldn't be a compatibility break because it wasn't public until now.
@andrewkaufman
Copy link
Contributor Author

I've rebased on master and added a commit for the change you suggested.

@johnhaddon
Copy link
Member

Great - thanks.

johnhaddon added a commit that referenced this pull request Oct 15, 2014
Exposing access to the DispatcherWindow for a given script.
@johnhaddon johnhaddon merged commit d917256 into GafferHQ:master Oct 15, 2014
@andrewkaufman andrewkaufman deleted the exposeDispatcherWindow branch October 15, 2014 17:15
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.

None yet

2 participants