ShaderTweakProxy

[danieldresser-ie]

I think this should be good for initial review and testing to make sure these are actually the interactions we want. Test coverage is a bit light, I should probably spend a bit of time improving that if everything else looks good.

As for logo, I ended up simplifying it to make it work on the tiny buttons in the Node Editor, and didn't end up too far from @murraystevenson's suggestion, though I'm pointing up instead of right, since the shader being proxied is conceptually somewhere "up" the graph. If we are happy with this icon, I'll need to hack the svg some to get it cleaned up - I used the old inkscape installed at IE, and it inserted a bunch of bogus changes all over the place.

One other thought - currently, there is nothing telling you about the right click interaction on shader plugs ... the discoverability is probably going to be a bit low anyway, but maybe we could come up with somewhere to tell users about that?

[danieldresser-ie]

Tidied things up a bit more, and added a little more testing. The main area I was wondering about testing more I'm actually not sure what I should do: what do we do if you hook up an auto proxy with the wrong type? Currently, if the type isn't convertible, you can get an error from OSL:

WARNING | unable to connect osl(param_c) to image(filename) - types are not compatible (RGB to STRING)

This is a pretty reasonable error, though maybe we should have a more specific error of our own?

What really bugs me though is that if there is no original input on the parameter we're tweaking, then the auto proxy doesn't need to make any connections, and just silently succeeds. This is generally totally correct - if you're inserting something, and there's no network upstream, then you just insert the tweak without anything upstream ( oh, although, see postscript ). But it's very weird if the autoproxy is the wrong type - you're writing to a color, and you're grabbing the input of that color, and connecting it to a string ... which should fail ... but then it succeeds because the color had no input.

I was thinking a bit about testing ahead of time whether the auto proxy type matched the tweak type, and throwing our own descriptive exception pre-emptively instead of letting it get to the shading system if they don't match ... but then I couldn't decide what defines if two types match? Is it if the plug version of one data type would accept an input from the plug version of the other? But we don't have any direct way of checking that without constructing plugs? ( Which we don't have at the point where ShaderTweaks is processing the network ).

My most recent thought is that maybe for the purpose of auto-proxy matching, we don't allow anything like connecting float to color - if the types are not identical, you get an exception. Maybe this is reasonable when we expect people to create auto proxies from the ShaderTweak plugs ... maybe it's too stringent though ... we currently allow you to create auto proxies from any shader input plug, which would make it very easy to create a color auto-proxy while tweaking a vector or something ... might annoy users if we block that, though maybe the clarity is worth it?

Postscript: I just realized another feature I haven't implemented that might be useful - if using an auto proxy to grab the original input of a tweak target, and there is no input node, it would probably be desirable if that instead fetched the original value of the original parameter, and put it on the destination of the auto-proxy on the tweak shader. This would preserve the expectation that hooking a ShaderTweak up to an auto-proxy through a grade shader will have no effect until you adjust the grade ( instead of discarding the parameter value if you apply this network to a parameter with no input node ).

[murraystevenson]

Thanks Daniel! A few usability first-impressions after playing with this a bit.

Should the ShaderTweakProxy node be in the tab-menu? It doesn't appear particularly usable on its own and by nature of it sorting before ShaderTweaks, it means you now need to type a lot more of "ShaderTweaks" to find it in the menu.

Creating a proxy from the Node Editor should ensure the tweaked parameter nodule is exposed when creating a connection, otherwise you need to manually do that as an additional step before you can edit the connection.

I wonder when choosing a node to generate a proxy from it would be better to still show the shader parameters, but filtered to only show parameters with inputs? Seeing the list of bare shaders is not so useful if you don't know ahead of time the name of the shader that you wish to create the proxy from. For example, the below situation with shader names that aren't usefully descriptive, where I'd want to create a tweak based on a proxy of the input of base_color, grade it, and connect the result to emission_color.

[danieldresser-ie]

Should the ShaderTweakProxy node be in the tab-menu? It doesn't appear particularly usable on its own

Oh, oops, that's left over from an earlier iteration, should definitely be removed.

Creating a proxy from the Node Editor should ensure the tweaked parameter nodule is exposed when creating a connection

Sounds reasonable

where I'd want to create a tweak based on a proxy of the input of base_color, grade it, and connect the result to emission_color.

Hmm, are you suggesting new functionality here, where you could create a proxy targetting whatever is connected to a parameter, instead of a specific shader handle? Or just to show the parameters in the dialog even though they can't be selected or interacted with, just because they provide useful information about where the user should actually look? A list of things to select that have children that can't be selected feels like a very non-standard UI, but I can see how it would be useful in this case.

[danieldresser-ie]

Oh, I guess what Murray was suggesting was that you would do the lookup of the input, but bake it when the proxy is created - so you would select the parameter "principled_bsdf.base_color", and it would just create a proxy for "image_texture" based on it currently being connected. Sounds pretty reasonable, though now it feels like this dialogue is getting too specific to store in ShaderUI

[danieldresser-ie]

Added two fix commits, one fixing Murray's first 2 easy comments, and the second mucking about with auto proxy passthrough when there is no input connection. This didn't end up feeling quite as clean as I was hoping ( thinking about types gets complicated when there are potentially 3 different types: the type of the parameter being tweaked, the type of the auto-proxy, and the type of the plug the auto-proxy is connected to ... but I think this is probably still more or less the right direction ).

[danieldresser-ie]

Added a commit where I hacked up the UI to implement the last version of what I discussed with Murray for selecting based on shader parameters.

Noted at the last minute that I seem to have somehow broken connecting to Arnold shaders with their single implicit output, but OSL shaders still seem to be working fine for testing.

[johnhaddon]

Thanks Daniel, and sorry for the delay in getting to this. Haven't been firing on all cylinders this week.

I haven't spent a great deal of time doing practical testing, but I've been through most of the code with the exception of some of the UI stuff. I'll leave the useability stuff to Murray, but I did have one question : do the "From ..." menu items make sense when accessing the menu from a tweak row in the ShaderTweaks NodeEditor? It feels like the context is so specific to that tweak that "Auto" might be the only logical choice there?

[danieldresser-ie]

OK, I think I've just about addressed everything - gotta run now, but might log back on later to reply to comments with specific commits ... there are a lot of little fix commits here to keep track of.

The crash I had encountered yesterday turned out to be because this now makes it possible to make cycles in shader networks ... we probably need to add a new cycle checker to ShaderTweaks to prevent this ... though it probably makes sense to fix the specific crash I found as well: ImageEngine/cortex#1417

[danieldresser-ie]

OK, added some replies to clarify where we're at on your comments. Test failure is trivial, I just need to update tests for an exception change ( or make the formatting of the new exception match the other one ).

[johnhaddon]

OK, added some replies to clarify where we're at on your comments.

Not sure if that means you're done or not, but I've been through and resolved what I can and commented on a few that still seem outstanding. I'll take a closer look when you've got the tests updated and confirmed its ready...

One question I have from a quick bit of testing just now though : when making a proxy in the UI, should we be selecting ithe node automatically? I'm finding that it's not always obvious where it has been created (because it's not using the layout algorithm, the first one i made positioned itself exactly on top of another node. Selecting it would make it clear where it was. But the possible annoyance is that if your NodeEditor isn't pinned then you'll now be editing the proxy and not the ShaderTweaks. Might be worth it though?

One other small thing that predates this PR : I'm always double-clicking or hitting return in the shader dialogue, expecting that to be a shortcut to selecting the shader and hitting the "OK" button. Should be easy I think - pretty sure there's a signal for it that we use in the other path dialogues. Could you maybe squeeze that in?

[danieldresser-ie]

Squashed the previous round of fixes - made two more fix commits, one with the cycle detector, and the other with a bunch of other little notes, since they seem pretty easy to review that way ( and otherwise, you can just look at the final result of this PR ).

As far as I know, that takes care of everything except:

• I found another weird corner case: if I use an autoproxy on a float parameter, and then connect the float output to a child plug of a color, then if there is an input connection, it will get properly transferred ( we just copy the plug name for the connection destination, which is something like "someColor.g" ). But if the float plug we are auto-proxying doesn't have an input, then this means we should transfer the value of the float plug in the input network to the green component of the color parameter, and my parameter copying code doesn't understand the name "someColor.g". Do you think we need to fix this?
• "I'm always double-clicking or hitting return in the shader dialogue, expecting that to be a shortcut to selecting the shader and hitting the "OK" button." I'm confused on this at a couple of levels: firstly, it seems to be working fine in _ShaderParameterDialogue? I can open the dialogue to create a tweak, and double click on parameter, or press enter, and closes the dialogue and makes a tweak. Unless you want selecting a shader to automatically select the first parameter? But anyway, if we want this to work for selecting shaders for tweaks, the issue we run into is that _PathListingWidget.__activated is hardcoded only trigger pathSelectedSignal if the path is a leaf ... does it makes sense to classify shaders as leaves in _ShaderDialogue, even if we are listing their input parameters for help navigating? Or do I need to hack up _PathListingWidget to make this possible?

[johnhaddon]

Thanks Daniel! New comments inline - mostly just UI stuff that I hope we can resolve quickly. I also added to an unresolved conversation from a previous review - I think we can avoid passing ScriptWindow around at all.

I found another weird corner case: if I use an autoproxy on a float parameter, and then connect the float output to a child plug of a color...do you think we need to fix this?

It seems like a pretty esoteric case, and I'm tempted to gamble that it won't happen in practice. What's the failure mode?

"I'm always double-clicking or hitting return in the shader dialogue, expecting that to be a shortcut to selecting the shader and hitting the "OK" button." I'm confused on this at a couple of levels: firstly, it seems to be working fine in _ShaderParameterDialogue?

Sorry, I think I must be either misremembering entirely here, or remembering something that was fixed aleady.

But anyway, if we want this to work for selecting shaders for tweaks, the issue we run into is that _PathListingWidget.__activated is hardcoded only trigger pathSelectedSignal if the path is a leaf ... does it makes sense to classify shaders as leaves in _ShaderDialogue, even if we are listing their input parameters for help navigating?

No, claiming to be a leaf while at the same time having children is a violation of Path's semantics. Double-clicking is a bit overloaded in PathListingWidget because it is also used to change directory when we're in List mode. I guess since we're not in that mode in this case, we could allow double-click to emit pathSelectedSignal() even for non-leaves when we in Tree mode. That's not the documented behaviour of that signal though, so we'd need to look at knock-on effects carefully. Probably best to punt for now, and maybe consider an improvement in 1.5. Sorry for the noise on this one...

[danieldresser-ie]

Added 2 fix commits that address all the UI comments - may be easier to just look at the final state of ShaderUI.

It seems like a pretty esoteric case, and I'm tempted to gamble that it won't happen in practice. What's the failure mode?

Documented the current state of this here: 036b159

As far as I know this is good to go after squashing.

[johnhaddon]

Thanks for the update - LGTM. Will just need a rebase to fix the merge conflict and get Changes.md updated to move things into the new section.

[danieldresser-ie]

Squashed and rebased.

[danieldresser-ie]

Added basic example file - didn't really make sense to create a separate PR when it depends on this one, but if there's anything off about it, it's fine to drop the last commit in order to get this merged, and we can always merge the example later.

[johnhaddon]

I've merged this but since found a problem : the "Create ShaderTweakProxy" menu item appears on all nodules, and then errors. For instance, right-clicking on the output of a Sphere node and navigating into the unwanted submenu gives this :

Traceback (most recent call last):
  File "/home/john/dev/build/gaffer-1.4/python/IECore/curry.py", line 48, in curriedFunction
    return func( *args, **kwds )
  File "/home/john/dev/build/gaffer-1.4/python/Gaffer/WeakMethod.py", line 65, in __call__
    return self.__method( s, *args, **kwArgs )
  File "/home/john/dev/build/gaffer-1.4/python/GafferUI/Menu.py", line 281, in __build
    for path, item in definition.items() :
AttributeError: 'NoneType' object has no attribute 'items'

Hopefully a quick fix @danieldresser-ie? I think we should get it done before we release...