Adding support for array parameters #5954
Conversation
There was a problem hiding this comment.
This isn't really a full review, we probably need @johnhaddon to look through it, but he's been busy, so I thought I'd take an initial look through at least. I don't have concrete suggestions, but I thought it was worth flagging a few questions:
When you create array plugs, you're creating non-dynamic array plugs with the initial element set to nullptr - this doesn't feel to me like it matches the intent of how ArrayPlug is intended to work ... it looks to me like the option to create an ArrayPlug with no first element is intended solely for dynamic ArrayPlugs during deserialization - where the first element is about to be added anyway by the next line of the serialization.
Since you're creating non-dynamic ArrayPlugs, I would think that the intention is that you should find out the element type from Arnold, and pass in a valid element type ( I'm assuming it must be possible to query the element type from Arnold? ).
It's not actually clear to me currently how these plugs would get proper elements set up ... I'm guessing that perhaps this is a minimal set of changes to Gaffer in order to get some of your external custom code working, and you have your own code that actually populates the array values for the shader nodes you're interested in? It totally makes sense to make minimal changes to Gaffer to get your stuff functioning - but I'm thinking that getting the array elements set up properly could make this more useful to other people who could use this functionality.
Anyway, code all looks pretty good to me - seems like we just need to double check it fits with overall intentions for array plugs - hopefully John will be able to take a look sometime soon, I might be missing something.
| { IECore::InternedString(), inputName } | ||
| } ); | ||
| } | ||
| } |
There was a problem hiding this comment.
It's not clear to me what this case is for - could definitely use a comment at least. It doesn't seem related to this PR's main purpose ( arrays, which are handled in the case above ). Maybe this should be obvious, but I can't think offhand of what data types have components, other than compoundNumerics, splines, and arrays?
There was a problem hiding this comment.
This was to allow support connections to the entire array or individual array indices.
There was a problem hiding this comment.
I'm still not quite following here - the conditional on line 615 should be handling arrays?
You're saying that if you make a connection to the entire array, then addParameterComponentConnections is called on a Plug that isn't an ArrayPlug?
There was a problem hiding this comment.
I have finally come back to this to implement John's change, and the reason for this block of code is to handle a float array that has a float plug connected to an index.
There was a problem hiding this comment.
Hmm, I'm still not 100% sure I understand this, but if I'm understanding correctly, you've identified a bug in the existing code, but I don't think this solution is in the right place. addComponentConnections is intended to add connections to the children of a plug, not the plug itself. I think the bug is on line 621 when the children of an array plug are handled just by calling addComponentConnections - this isn't right, because the children of an array might not have components ( ie. a float array ). But we don't exactly have the right code for "add a top level connection if the top level is connected, otherwise add component connections" in a form that's easy to call - if we just called addParameter on line 621, that would sort of do the trick, but it would also be trying to set the parameter value, which we don't want here. We could paste in the relevant code from addParameter, but that's a bit messy. Maybe @johnhaddon has an idea how he'd like this arranged.
There was a problem hiding this comment.
It's not really a bug in current gaffer, it's very specific to array plugs as they have component connections which can either have sub component connections or just regular component connections. eg. ramp['color'][1]['r'] vs ramp['position'][1]. This made the most sense to me as I didn't want to have a bunch of duplicate code looking for components within the array, I would be happy to move it anywhere it would make more sense.
|
Apologies again for the delay. I was talking to John about it this morning. He said that the issues with how array plugs are set up does seem like a problem - but he pointed out that maybe the reason why you haven't done this the way we would expect is that we don't currently have a good way of setting up an array with a type, but with 0 elements. It's possible that in order to really do this properly, we'll need a way to do that ( John is brainstorming on a possibility where array plugs store a prototype that is independent of any element, so that array plugs can always have a proper type, even with 0 elements ). |
That is correct, it was easier to handle adding the child plug types on my internal code when I'm actually constructing the arrays. I am open to ideas to define the array's type with 0 elements so expanding it is simpler. |
I've opened a PR that improves the ArrayPlug so that it can be constructed with zero length while still knowing the type of its array elements, so that Once that is merged I suggest we update this PR to use that functionality. |
|
Thanks John! I'll update this once I get the chance. |
This allows for full support of any Arnold node that requires array data(eg. ramp_float).
Checklist