Box UI improvements

[johnhaddon]

This adds proper mechanisms for doing what users were already doing manually - using dot-like nodes to try to keep track of the inputs and outputs to a Box.

Previously, if you were to Box these nodes :

The Box internals would look like this :

Note that there's no indication that the two inputs come from the same place, and no way of editing them. And the outputs aren't represented at all! With this PR, the box internals would now look like this, with the dark BoxIn and BoxOut nodes providing a proper graphical representation of what is going on, and a place where the connections can be edited :

You can still promote from the right click menu in the NodeGraph, but you can also now drop down a BoxIn or BoxOut node from the node menu, and then connect something to the adders on it to promote a plug. Likewise, the Box itself now has adders so that additional inputs and outputs may be added from the outside.

The most contentious part of this is 6426c05 I think - I've included a long commit message there explaining the pros and cons of the approach, and what I think the alternative would be if people are uncomfortable with it. At this point I think I'm happy enough to put it out there and see how it works in practice, knowing that I have an alternative plan. But I'll sleep on it, and am happy to hear all input and have a bash at the alternative if folks think it would clearly be better...

[johnhaddon]

I've pushed three new commits to address the test failures on Travis. IE folks will want to take a look at d3f1393, because it removes a deprecated method which is still in use in the internal codebase. I've checked all the instances of this on GitHub, and they're all trivially replaceable with the equivalent metadata, so I'm hoping this isn't an issue, but do let me know if it is. Here's a search which should find everything which needs to change :

https://github.com/search?p=1&q=registerNodule+user%3AImageEngine&type=Code&utf8=%E2%9C%93

[andrewkaufman]

Yeah, that registerNodule thing seems fine... based on the search results, its seems to be the only way we're going to clean up those old nodes...

[andrewkaufman]

Great work so far! I like the icons too!

I made some comments inline, but here are a few reactions from playing with it:

• Can we add space between existing nodules and the PlugAdder? Ideally the same spacing you get between "in" and "in1" once you've connected to the PlugAdder.
• Deleting "in1" from outside the Box leaves a dangling BoxIO on the inside (and its even still wired up and serializes) which is a bit misleading, and also means copy/pasting the node brings back the outer plug I deleted.
• Promoting an ArrayPlug draws a bit funny. I think I'd have expected to see an output array of CompoundNodules on the BoxIO all wired up to their counterparts.
• Deleting a promoted ArrayPlug from the outside leaves (a) a dangling BoxIO and (b) a broken NodeGadget on the internal node which was promoted (e.g. Switch).
  • Copy/pasting in this state crashes in Gaffer::Switch<Gaffer::ComputeNode>::oppositePlug(Gaffer::Plug const*, unsigned long)
• I can create BoxIn and BoxOut nodes from outside a Box, though I don't seem to be able to do anything with them... can we remove them from the menu? I guess this is a bigger request regarding scope aware menus, so maybe we should leave it for another PR?
• If I box up a graph, promote some plugs, then box up the internals (including the BoxIOs), my promoted plugs all disappear.
  • I think I'd have expected the outermost plugs to be retained, the inner Box to have auto-promoted the BoxIOs it's encapsulating, and wired its newly promoted plugs up to the outermost plugs (using new BoxIOs).
  • Note I again have dangling BoxIOs on the innermost graph, so copy/pasting will get me half-way back to where I expected I guess.
• Using the "MoveTo" menu items from the outside doesn't affect the sided-ness of the BoxIO.
  • It would be nice if the "MoveTo" items were available on the BoxIO as well (and affect the outside plug).
  • Same request for nodule and noodle color.
  • Same request for widget and nodule type
    • Actually, this has me wondering, should we always have BoxIOs even if there is no nodule? It might be nice to have drag/drop access to the widget from inside as well. I guess the inner graph would get cluttered really fast though...
  • I guess this brings the serialization question up again (see inline comments), but should the BoxIO metadata even be persistent? Or should it's NodeGadget read those values directly from the external plug?

And just a note to IE team:

• We've got a handful of nodes which derive from Box that will now have unwanted PlugAdders. We either need to derive from something else, or change back to StandardNodeGadget on our nodes.

[andrewkaufman]

I presume this bit makes the internal plugs serialized? I think that @danieldresser was anticipating taking issue with that, on the grounds these IO nodes should be UI sugar. I suppose he was hoping for an automated re-generation of them based on the existing Box serialization.

[johnhaddon]

Since @danieldresser hasn't entered the fray yet, I'll launch a preemptive strike...

I do actually share some of Daniel's feelings on this, and it's one of the reasons I've avoided getting to this work for so long, and why I spent so long trying a different avenue before settling on this. But despite it not being perfect I do believe that this is the right compromise.

My original approach attempted to avoid any additional nodes at all so that the plug promotion API would remain exactly as it was, and there wouldn't even be a need for "automated re-generation" - I regarded this "pure UI sugar" approach as the perfect solution. But the problem is the users really do want these things to be represented as nodes - they want to select them, position them, delete them, copy/paste them, have them auto-layout etc etc. That means either complicating every single part of the code to deal with these fictional non-node entities, or just compromising and using nodes. So I compromised.

But I don't think then walking halfway back and having the nodes exist or not depending on whether or not they've been viewed is desirable at all - that's not "UI sugar", that's an inconsistent model. Here are just a few of the potential problems :

• We would still need to serialise their position, colour, name etc so we could restore them appropriately when the time came. Where will that be stored if we don't serialise them?
• We would want to see them when viewing References, but we don't want to change the contents of References.
• It would be baffling to drop down a BoxIO node explicitly, then discover it actually doesn't exist any more at a later date, only to find it mysteriously appearing again later.

But maybe you had a different proposal Daniel, and I'm barking up the wrong tree?

P.S. This "UI sugar is implemented as persistent nodes" compromise is actually totally consistent with the Dots and Backdrops we already have, so it's nothing new.

[andrewkaufman]

I know I signed off on this earlier, but it definitely seems funky now I'm thinking about it more... might also be one for @danieldresser to comment on regarding whether BoxIOs are truly part of the graph, or just auto-generated whenever the graph is visualized.

[andrewkaufman]

Also has me wondering, if we stick with this, should we move the "nodule:type" metadata from the UI modules into the core modules? It's not just about "how do I draw this" anymore and has actual meaning to the graph construction.

[johnhaddon]

Hopefully I've convinced you above that BoxIOs definitely shouldn't disappear from the serialisation. But I'm happy to reconsider our options here for promote(), since it is the Achilles heel of this approach. Here's a quick rundown of the possibilities as I see them :

1. The status quo. The downside is a coupling of promote() and the "nodule:type" metadata. The upside is that automated scripts can still produce the graph a user would want to see, provided they're run with the metadata loaded, and those scripts don't need to consider the metadata themselves. It's dirty inside MetadataAlgo, but it avoids other code getting its hands dirty.
2. As above, plus moving the metadata to the core modules. Upside is graph creation is deterministic (same whether the UI modules are loaded or not), downside is the core modules are polluted with UI metadata, and we still need the UI files for the widget-specific stuff, so everything UI related isn't all in one place any more. Plus the temptation to start using UI metadata for other purposes.
3. Explicit requests for BoxIO usage in PlugAlgo, with all decisions made by the UI. Upside is we've removed the coupling. Downside is an additional set<Plug*> plugsThatNeedBoxIO argument in Box::create(), more code in the UI to build that set appropriately (based indirectly on the metadata!), and non-UI-savvy automated scripts never producing BoxIO even though the user would appreciate it. Although this is undoubtedly purer, I'm not convinced it's much of an improvement in real terms, which is why I went with 1.
4. Pure UI sugar. This already failed.
5. BoxIOs appear magically when viewed, disappear when serialised. Ick.
6. A mix of 3 and 5. The UI requests BoxIO nodes explicitly. Automated scripts don't use 'em at all. BoxIO nodes are always serialised. The NodeGraph magically creates BoxIO nodes when first viewing graphs generated by any non-UI-savvy scripts. Upsides are the same as 3, plus a bit of magic. Downsides are getting the magic to work reliably and efficiently (just on first viewing, or when something is promoted naively during viewing?). And what do we do with References? Do we modify them even though they're read-only?
7. What Daniel was really going to suggest, assuming I assumed the wrong thing before.
   • After meeting this became: Always make BoxIOs, but don't have a node gadget for them unless the metadata says we should.

Of those, I only see 1, 3 and 6 as worthy candidates (unless 7 is awesome). 3 would undoubtedly make me feel less dirty, but I'm not sure that feeling of cleanliness comes with any real-world benefits. 6 is a bit of an unknown, and might be solving a problem we don't really have - I'd at least like to have the need for the magic before implementing it. So from my point of view we're back to the options from the commit comment, perhaps with the additional consideration that 3) does at least prepare the way for 6) should we need it later.

[andrewkaufman]

I edited your (7) in place with Daniel's idea from the meeting. I thought it was easier to understand if we had them all written in place...

[johnhaddon]

I'm working on GafferImage stuff today, but unless I hear otherwise from your end, I plan to at least explore option 7 to see what would be involved....

[andrewkaufman]

Oh, ok cool. I thought you were down on 7 after the meeting because of the sheer number of extra BoxIOs it involves (assuming most Boxes have more non-nodule-plugs than nodule-plugs).

[johnhaddon]

Yeah, I do have my concerns over that, but the fact that everything is treated the same and we have a chance to respond to nodule/no-nodule changes on the fly does give it a certain allure. I wonder if someone at that end could do a quick survey to see what the worst ratio of non-nodule to nodule plugs is in a production box, and also the ratio of promoted plugs to non-promoted plugs?

[andrewkaufman]

A Lighting BRP (fancy Box) from a recent show had 65 exposed non-nodule plugs (almost exclusively bools, strings, and floats for what its worth). For this particular example, they were all made from the UIEditor and then connected down inside, but I don't think that most artists work that way. It had 4 nodule plugs (all of which come from the node definition and not through promotion, though we'll likely update that to use the BoxIOs as well). Grepping the file, there were 25094 plug constructors in the gfr in total, 7275 of which have serialized nodule:type metadata. Of those, 378 are GafferUI::StandardNodule, 6891 are '', and 6 are None.

Talking with Lookdev, they thought 10-20 non-nodule plugs was normal for a LookdevBox and rarely promote nodule plugs (just getting the scene in/out pair plus maybe a filter). For their internal lookdev tools (also Box/References), they do expose more nodules (shader inputs), but rarely more than 5-10.

[andrewkaufman]

Any reason its not "nodeGadget:icon" ?

[andrewkaufman]

I couldn't figure out why this claims to be an outdated diff... its still called "icon", right?

[johnhaddon]

Great work so far! I like the icons too!

Haha - thanks. It's upsetting how long it takes me to make an icon, but at least now we have everything in place so someone with actual ability could do the rest.

I made some comments inline, but here are a few reactions from playing with it:

I might not get to dealing with it all today, but I just wanted to reply to a couple of points below. Anything I don't reply to, I'm hoping will be straightforward...

I can create BoxIn and BoxOut nodes from outside a Box, though I don't seem to be able to do anything with them... can we remove them from the menu? I guess this is a bigger request regarding scope aware menus, so maybe we should leave it for another PR?

The only thing you could do would be to make a network you then paste into a Box (at which point they'll promote themselves automatically). I think you're right that removing them would need some sort of scope-aware menu system, so I'd leave it for now - it's pretty harmless.

If I box up a graph, promote some plugs, then box up the internals (including the BoxIOs), my promoted plugs all disappear.
I think I'd have expected the outermost plugs to be retained, the inner Box to have auto-promoted the BoxIOs it's encapsulating, and wired its newly promoted plugs up to the outermost plugs (using new BoxIOs).

Ah yes. So, I think in theory you'd get what you want by us automatically ignoring BoxIOs when boxing, so new BoxIOs are made inside the new Box, and the old ones remain where they are? That seems like the best way of implementing it to me. I've just seen a related problem where if you box up the internal network (but excluding the BoxIOs) it complains that it can't promote an already promoted plug, but I think if I can fix that then I can get you the behaviour you want.

Actually, this has me wondering, should we always have BoxIOs even if there is no nodule? It might be nice to have drag/drop access to the widget from inside as well. I guess the inner graph would get cluttered really fast though...

I think that would be crazy cluttered.

I guess this brings the serialization question up again (see inline comments), but should the BoxIO metadata even be persistent? Or should it's NodeGadget read those values directly from the external plug?

I'll look into this, but I think the short answer will be that it'll be complex. We do have several use cases for auto-metadata-forwarding, but I'd like to attack that at another time if possible - there's already a lot in this one, and a lot of other stuff I need to get to.

[andrewkaufman]

Some unfortunate (but predictable) feedback from users: Existing Boxes should automatically get the BoxIOs created, so we don't have to recreate every single boxed up tool we have. Think that can be accomplished with a compatibility config or something? I'd guess (hope) its fine for References to not get them.

[johnhaddon]

Some unfortunate (but predictable) feedback from users: Existing Boxes should automatically get the BoxIOs created, so we don't have to recreate every single boxed up tool we have. Think that can be accomplished with a compatibility config or something? I'd guess (hope) its fine for References to not get them.

It might be possible using Script::scriptExecutedSignal(), although it would have been easier if that signal took a parent argument - maybe I should change it so it does? Option 6 would take care of it automatically I guess, but noone has been a champion for that so far (what are everyone's leanings by the way?). It would be easy enough to add an "Upgrade my box" tool menu option in the NodeEditor, or maybe for an IE-specific solution to slot that same operation into a Jabuka load?

I'm getting close on my GafferImage work, so I'm going to chug away at that a bit longer and hope to become enlightened on this in the meantime.

[andrewkaufman]

what are everyone's leanings by the way?

Not sure about everyone else, but I was waiting to hear back on your experiments with option 7. If it proved not to be too much of an overhead, I like that best because the BoxIOs are always there, and the UI is just deciding if they're visible.

[andrewkaufman]

It would be easy enough to add an "Upgrade my box" tool menu option in the NodeEditor

I'll float that to users and see how it goes.

[andrewkaufman]

It would be easy enough to add an "Upgrade my box" tool menu option in the NodeEditor

I'll float that to users and see how it goes.

Agreed to add that Upgrade tool as an IE config, and to add a Jabuka verification making sure it got run prior to any new publishes of old boxes.

[danieldresser]

It might be possible using Script::scriptExecutedSignal(), although it would have been easier if that signal took a parent argument - maybe I should change it so it does? Option 6 would take care of it automatically I guess, but noone has been a champion for that so far (what are everyone's leanings by the way?).

The other option that would take care of it automatically is if we added BoxIOs when you added a plug to a box, and then didn't serialize the BoxIOs. I still kind of like the sound of this, but I guess other people didn't like the options for serializing the position of the BoxIO, which I guess would either look like:

__children["Box1"].addChild( GafferImage.ImagePlug( "myOutput", direction = Gaffer.Plug.Direction.Out, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )

# Adding a uiPosition to the BoxIO for myOutput - we haven't explicitly created this node, but we know it will be automatically created when we add the plug above.
__children["Box1"]]["__myOutput_boxIO"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = IECore.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
__children["Box1"]["__myOutput_boxIO"]["__uiPosition"].setValue( IECore.V2f( 42, 42 ) )

or:

__children["Box1"].addChild( GafferImage.ImagePlug( "myOutput", direction = Gaffer.Plug.Direction.Out, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) )
Gaffer.Metadata.registerPlugValue( __children["Box1"]["myOutput"], 'boxIO:uiPositon', IECore.V2f( 42, 42 ) )

The first seems like it would just work without much work, the second is a bit cleaner looking but would take some work? But if we've decided that we do want to automatically add BoxIOs all the time, then this approach would mean that there would be nothing to do to convert old scenes.

[johnhaddon]

I've pushed a series of commits for further discussion. Definitely not ready for merging, but I wanted to get something up for feedback before I disappeared for the weekend. I've actually reverted all the ui-metadata-used-in-non-ui-module magic since I wanted to explore another alternative.

Deleting "in1" from outside the Box leaves a dangling BoxIO on the inside (and its even still wired up and serializes) which is a bit misleading, and also means copy/pasting the node brings back the outer plug I deleted.

Should be fixed in 9f852f8.

If I box up a graph, promote some plugs, then box up the internals (including the BoxIOs), my promoted plugs all disappear.

Should be fixed in bb9aa33

Using the "MoveTo" menu items from the outside doesn't affect the sided-ness of the BoxIO.
It would be nice if the "MoveTo" items were available on the BoxIO as well (and affect the outside plug).
Same request for nodule and noodle color.
Same request for widget and nodule type

I'm actually becoming less sure that this is desirable. I think it's possible that the internal layout might not naturally mirror the external one - that's one of the reasons why users wanted you-can-put-me-anywhere nodes and not at-the-edge-of-the-frame-nodules-that-match-the-external-ordering. I also think for some things it's possible that you might want nodules on the outside but not the inside (and vice-versa). I could probably be persuaded around, but I'm tempted to say we should put this in the wild without the mirroring and see what people say.

Some unfortunate (but predictable) feedback from users: Existing Boxes should automatically get the BoxIOs created, so we don't have to recreate every single boxed up tool we have.

This sent me down the avenue I'm exploring now. I think someone suggested something similar a while back and I wasn't keen, but given the new requirement and the difficulties of all the other options I've decided to give it a go. In 206144c I've added something to the NodeGraph that does a just-in-time addition of BoxIO nodes to upgrade old boxes. The nice thing about this is that it also means that Box::create() (and potentially PlugAlgo::promote()) no longer need any magic - it just does the naive promotion and then the UI fixes it. I do feel a bit funny about this, but it's actually pretty analogous to the way we already do just-in-time layout of nodes that are added to the graph from scripts. The scripts just do what they want without caring about the UI, and then the UI improves the graph readability at the point it is viewed. The edits are a bit more intrusive this time (adding nodes instead of just positioning them), but it does have the advantage of totally disentangling the UI/non-UI stuff.

I haven't got this working with right-click-promote yet, but I wanted to push it up for comment before I went any further. Given that I had to do something about upgrading old boxes anyway, I'm fairly keen on this approach...

[andrewkaufman]

Deleting "in1" from outside the Box leaves a dangling BoxIO on the inside

Should be fixed in 9f852f8.

The fix works for BoxIns but not for BoxOuts. Though you've got testPromotedPlugRemovalDeletesBoxOut so I'm not sure how that could be... nevertheless...

MoveTo / Color metadata 2-way coupling

I'm actually becoming less sure that this is desirable

I'm willing to let it hit the floor and get the user feedback as you suggest. My current stance is that I'll be quite shocked if users don't find it frustrating. But if they don't then I'll owe you a beer. But you'll need to be physically present in Vancouver to receive it.

The edits are a bit more intrusive this time (adding nodes instead of just positioning them), but it does have the advantage of totally disentangling the UI/non-UI stuff.

I guess the downside is that the script that made the graph originally now can't make assumptions about its structure, because it will vary depending if its ever been viewed in the UI. Not sure how I feel about it yet. Though using it in practice its been fine (unless its causing the first issue?).

Can we add space between existing nodules and the PlugAdder? Ideally the same spacing you get between "in" and "in1" once you've connected to the PlugAdder.

Still pending?

Deleting a promoted ArrayPlug from the outside leaves (a) a dangling BoxIO and (b) a broken NodeGadget on the internal node which was promoted (e.g. Switch).

Still pending?

/// - "icon" : string naming an image to be used with ImageGadget

I think this got burried as an "outdated comment" or something, so I never got an answer. Why aren't we calling it "nodeGadget:icon" to match all the other metadata supported by StandardNodeGadget?

[johnhaddon]

Sorry for the lack of progress on this. I felt like I was banging my head a bit so I've taken a bit of a break to get the image catalogue stuff moving. I realise we need to push this through though, so will hopefully come at it fresh tomorrow.

Deleting "in1" from outside the Box leaves a dangling BoxIO on the inside
Should be fixed in 9f852f8.

The fix works for BoxIns but not for BoxOuts. Though you've got testPromotedPlugRemovalDeletesBoxOut so I'm not sure how that could be... nevertheless...

I did get this figured out yesterday - it was down to a different order of parenting than what I'd used in the test cases. Fixed for both sequences of events now (will push up with the rest when it's done).

The edits are a bit more intrusive this time (adding nodes instead of just positioning them), but it does have the advantage of totally disentangling the UI/non-UI stuff.

I guess the downside is that the script that made the graph originally now can't make assumptions about its structure, because it will vary depending if its ever been viewed in the UI. Not sure how I feel about it yet. Though using it in practice its been fine (unless its causing the first issue?).

Yeah, I'm not sure about it either. In terms of the expectations of old scripts, this means the script sees what it expects while running initially (no BoxIOs are made by promote()), but would see something new if it was run again later. I can see arguments for this being preferable to the behaviour changing while running (BoxIOs being made on the fly), but really I'm not going to be totally happy with any of this. I put the "dynamic addition/removal of nodegadgets pr (#2029) together yesterday to explore the UI implications of the other way of doing it, and it seemed like it might work, but I'm still not sure what's best.

Can we add space between existing nodules and the PlugAdder? Ideally the same spacing you get between "in" and "in1" once you've connected to the PlugAdder.
Still pending?

Yes. I took a look at this, and the ideal way to do it would be to have the NoduleLayout support custom widgets in the same way that PlugLayout does. The problem there is the lack of a factory mechanism for arbitrary Gadgets. Not sure what to do yet...

Deleting a promoted ArrayPlug from the outside leaves (a) a dangling BoxIO and (b) a broken NodeGadget on the internal node which was promoted (e.g. Switch).

Still pending?

I think this is fixed by the other "delete from the outside" fix, but we still need to talk about the expectations for ArrayPlug presentation internally. If we expose the individual connections, then it's very easy to remove one, and then break the connection at the ArrayPlug (parent) level, which means that newly added children on the promoted plug will no longer be connected through. I'm wondering if it's be better to switch to a StandardNodule at the point we promote the ArrayPlug, so only the master connection is shown.

/// - "icon" : string naming an image to be used with ImageGadget

I think this got burried as an "outdated comment" or something, so I never got an answer. Why aren't we calling it "nodeGadget:icon" to match all the other metadata supported by StandardNodeGadget?

I was treating it the same as "description" on the grounds that the icon could be used in other spots (the documentation for instance)...

Again, sorry for the slow progress on this one. If you have a chance to look at #2029 that might be handy, and help give me a kick up the bum...

[andrewkaufman]

I'm wondering if it's be better to switch to a StandardNodule at the point we promote the ArrayPlug, so only the master connection is shown.

Yeah, that sounds fine to me.

If you have a chance to look at #2029 that might be handy, and help give me a kick up the bum...

I looked at the code yesterday, but the tests were failing. Looks like they're passing now, so I'll just merge it.

[johnhaddon]

OK, I've rebased this onto the current master, and taken a simpler approach discussed separately with @andrewkaufman. Given that this next release is going to be a big one, and all the "magic" solutions we've discussed have the potential for problems, I've taken a much more conservative approach and just added some BoxIO utility methods that the UI now uses explicitly when promoting and boxing. This seems to give a pretty decent user experience, and should let us get a first release out without worries about unwanted side effects.

The first half of the PR is pretty much unchanged, but since the rebase has shifted everything a bit, here's a guide to how the rest lines up with the review comments from before...

• Can we add space between existing nodules and the PlugAdder? Ideally the same spacing you get between "in" and "in1" once you've connected to the PlugAdder.

Done in 1dc5646 and 28cb5eb - these are functionally the same as before, but now use the NoduleLayout custom gadget mechanism from #2043 to make sure the spacing is as requested.

• Deleting "in1" from outside the Box leaves a dangling BoxIO on the inside (and its even still wired up and serializes) which is a bit misleading, and also means copy/pasting the node brings back the outer plug I deleted.
  The fix works for BoxIns but not for BoxOuts. Though you've got testPromotedPlugRemovalDeletesBoxOut so I'm not sure how that could be... nevertheless...

Both fixed in 1b6c5de.

• Promoting an ArrayPlug draws a bit funny. I think I'd have expected to see an output array of CompoundNodules on the BoxIO all wired up to their counterparts.

Fixed in 31570a3 - we now draw a single connection for the ArrayPlug itself, and hide the child connections because editing them would break the promotion.

• Deleting a promoted ArrayPlug from the outside leaves (a) a dangling BoxIO and (b) a broken NodeGadget on the internal node which was promoted (e.g. Switch).

Fixed in 6970596. The problem was that the UI wasn't actually deleting the ArrayPlug, but was deleting one of its child plugs.

• If I box up a graph, promote some plugs, then box up the internals (including the BoxIOs), my promoted plugs all disappear.

Fixed in 7ebbb17.

Some unfortunate (but predictable) feedback from users: Existing Boxes should automatically get the BoxIOs created, so we don't have to recreate every single boxed up tool we have.

"Upgrade to use BoxIO" tool menu item added in d8e1243. BoxIO::insert( Box *box ) method from 7250bfa also available to allow Jabuka to perform the upgrade automatically if wanted.

Cheers...
John

[johnhaddon]

The other option that would take care of it automatically is if we added BoxIOs when you added a plug to a box, and then didn't serialize the BoxIOs. I still kind of like the sound of this...

Sorry, I realise I never replied to this comment @danieldresser. I've said before though that I don't think this suggested juggling of the serialisation buys us anything, because there will still be BoxIO nodes at any point that matters - whenever a user or a script interacts with the graph. But I also think there's a more fundamental reason this isn't the way to go. BoxIO nodes are actually a primary point of interaction for the user - they create them explicitly from the NodeMenu, and interact with them directly in the NodeGraph and NodeEditor. BoxIO nodes really do exist in their own right, so mangling the serialisation to pretend that they don't exist doesn't make any sense...

Now, like you, I would have liked an approach where there were no BoxIO nodes and everything was just UI, but I did spend a lot of time exploring that avenue and got nowhere, except to realise that users really do want BoxIO nodes to exist...

[andrewkaufman]

Needs a rebase now I've merge #2049

[johnhaddon]

Needs a rebase now I've merge #2049

I'm on it. Should I push over the top, or do you still need to read through all the commit links in my last comments?

[andrewkaufman]

If you do it now, I haven't started reviewing the commits yet.

[johnhaddon]

I won't push yet then, since then all the handy links I've added to the commits won't be of any use. I'll wait for your comments and then push the rebase at the same time as I address them...

[andrewkaufman]

I'm still having trouble with promoted-then-deleted ArrayPlugs, though its a bit different to the old trouble:

• create Box
• dive inside and create Switch
  • setup for scenes and promote array
• dive out and delete promoted array (via rmb on first child)
• dive back in
  • the Switch UI is broken from a user perspective (in that the ArrayPlug retained its StandardNodule)
• re-promote the array and dive back out
  • the promoted UI is broken from a user perspective (in that the promoted ArrayPlug inherited the StandardNodule)

Also, the "UI-only" metadata has definitely crept into the core now (7250bfa, 31570a3) hasn't it? I take it you're feeling ok with that, aside from it causing the issue above?

[johnhaddon]

I've pushed a rebase now, so it's at least mergeable.

I'm still having trouble with promoted-then-deleted ArrayPlugs

Gah - I thought you'd anticipated this is how it would work when we chose this approach.

I could try doing some extra tracking to know whether BoxIO added the metadata and to remove it at the right time, but having spent a bit of time looking at it, I think it's likely to be a bit problematic. Removing it when the BoxIn node (or the promoted) plug is deleted looks doable, but the necessary tracking to remove it if the connection is broken directly in the UI looks heavy without core changes.

I suspect the best approach here is to give CompoundNodule a mode where it automatically switches to represent the parent when the parent has an input, and only splits to show the children when the children have independent inputs. That'd be along the lines of what we've been talking about to enable float->color.x type connections in the NodeGraph. I'm not sure that's a quick "squeeze it in for this release" task though.

Other options would be to temporarily keep the oddball multiple-connections representation we had before, add a "show as array again" option to the right click menu or to leave this as future work.

Also, the "UI-only" metadata has definitely crept into the core now (7250bfa, 31570a3) hasn't it? I take it you're feeling ok with that?

Yep, I'm OK with it. It's the same compromise we've already made for things like Dot::setup() and Switch::setup(). I did consider moving those BoxIO static utilities into GafferUI instead but I didn't see any benefit - they don't introduce a true dependency anyway, and because they're specific to BoxIO the caller is expecting the behaviour they implement.

[andrewkaufman]

I suspect the best approach here is to give CompoundNodule a mode where it automatically switches to represent the parent when the parent has an input, and only splits to show the children when the children have independent inputs. That'd be along the lines of what we've been talking about to enable float->color.x type connections in the NodeGraph.

I like that approach. Lets leave it for later as you say. It was a slightly manufactured problem case anyways, might not come up in production for a while.

[andrewkaufman]

Oh, one last thought. Is there a convenient way to hide the PlugAdders on the Box? We have a few automated tools that create Boxes, and it'd be nice not to have the PlugAdders there. I realize we could just create a Node or SubGraph, and if there is a reason to keep them Boxes, it seems we can do this after creation:

Gaffer.Metadata.registerValue( node, 'noduleLayout:customGadget:addButtonTop:gadgetType', '' )
Gaffer.Metadata.registerValue( node, 'noduleLayout:customGadget:addButtonBottom:gadgetType', '' )
Gaffer.Metadata.registerValue( node, 'noduleLayout:customGadget:addButtonLeft:gadgetType', '' )
Gaffer.Metadata.registerValue( node, 'noduleLayout:customGadget:addButtonRight:gadgetType', '' )

But is there a more concise way to accomplish that? Maybe a single utility function?

[johnhaddon]

Hmm, I'm not super keen on that. If we're talking about nodes that derive from Box when they should be deriving from something else, I don't think we should do anything to encourage it. And if we're talking about scripts that create a sort of "starting point" box that the user then fills in, then I'm a bit confused about what makes the adders undesirable but makes all the other ways of promoting plugs OK?

I guess I'd say that editing metadata is the canonical way of defining UIs, and that given that the use cases above are a bit non-standard, we don't need anything more convenient, or the convenience belongs in internal code until proven otherwise...

Also, I really want this PR to die!

[andrewkaufman]

There was a renderdl test failure on the mac build, but I decided to ignore it...