Shader : Support inline ContextProcessors #6211
Conversation
There was a problem hiding this comment.
As is often the case, I feel like I ought to have more to say about this much code, but I've looked through it fairly carefully, and it all looks good to me.
Just as evidence I've looked through it, one not-very-relevant thought: it feels slightly redundant in some sense to index a hash map with ShaderAndHash, when we assume that MurmurHash can never collide , so in theory you could just append the shader pointer to the hash, and just store the hash. But I don't think there would be any noticeable performance benefit to doing that, and the current code documents it's intent better, so after thinking about it I concluded that the current approach is probably correct.
| source = activeInPlug->source(); | ||
| } | ||
| return { nullptr, nullptr }; | ||
| } |
There was a problem hiding this comment.
Why do we need to return a pair from this function instead of just returning source? I can't see any scenario where result.first is set to anything other than result.second->node(), unless result.second == nullptr, which would be just as easy to check for as checking if the separate first argument is nullptr?
There was a problem hiding this comment.
All the clients of the old sourceShader() had an additional level of nesting to get the Shader and check it wasn't null, and I wanted to get rid of that. I could return a null plug if the shader is null though, and then have the clients static_cast<const Shader *>( plug->node() ) instead of doing their own checks. Maybe that's worth it?
There was a problem hiding this comment.
Yeah, I was thinking of doing the test if the shader is valid and returning a null plug if it isn't valid. I have a pretty strong preference for passing around as little information as possible, and this interface feels a little odd to me - there are basically 3 return values, 2 in a pair, and 1 in reference argument. I would prefer the if there was a single return value ... but it is just aesthetics and not really that important. Kinda just felt like I should comment something after reading this much code.
There was a problem hiding this comment.
I've pushed a fixup which just returns a const Plug * and derives the shader from that in all the clients :
This also adds support for inline Loops and Spreadsheets, and fixes a bug whereby two consecutive Switches were not handled correctly.
Like the new support in Shader, this also supports Loops and Spreadsheet nodes.
johnhaddon
force-pushed
the
contextProcessorsInShaders
branch
from
e903324 to
22ac984
Compare
This allows ContextProcessor, Loop and Spreadsheet nodes to be inserted inline within a shader network, receiving inputs from Shader nodes and passing outputs to Shader nodes. This opens up the possibility for several interesting workflows, one such being an "uber connection" which passes multiple textures through a single colour plug, using a ContextVariables node at the destination to choose which texture to plug in. Along the way, the context variable can be used to apply different grading etc to each texture in a very tidy way. In prototype form at Cinesite, this has already vastly simplified some complex shader networks, leading to much improved render times as well.