Shuffle improvements

[johnhaddon]

As a side-benefit of a spare-time project I've got going, this improves the channel Shuffle node to use the same standard ShufflesPlug that ShuffleAttributes and ShufflePrimitiveVariables uses. With the following features :

• Arbitrary numbers of shuffles in the UI.
• Enable/disable for individual shuffles.
• Wildcards to match multiple source channels, and substitutions and expressions to map them to destination channels.
• Optional deletion of the source channel.
• Optional preservation of pre-existing destination channels.
• Control over behaviour when a source doesn't exist - Ignore, Error or Black.

This was all going swimmingly until 6dea107, where I realised the special __black and __white "virtual" source channels we provide could now be matched by wildcards. In that commit I've made a "fix" that is technically a bit sketchy, but has the one virtue of not complicating the ShufflesPlug API any further. I don't think that should be the final solution, but I'm opening the PR to provide a basis for discussion. How should we improve the ShufflesPlug API to do this properly?

1. Add a const T &fallbackContainer argument for values we want to provide exact matches but be ignored by wildcard matches. We'd need this in both shuffle() and shuffleWithDefaultSource() though.
2. Something a bit more like TweaksPlug, with a GetDataFunctor and SetDataFunctor? This could be quite nice and flexible, but how would we extend it to support wildcard matches?
3. Something else?

Thoughts welcome!

[danieldresser-ie]

I've read through this in moderate detail, and I think I pretty well understand it, and it all looks pretty good.

It will definitely be a great change, I am frequently annoyed by not having these features on Shuffle in my own testing.

I don't have any clear ideas on the virtual channel sources, though I can throw one more option on the pile:
We already have a lot of unused surface area in ShufflesPlug::shuffle(WithDefault) because we pass in the list of input channels as a map ... I guess in theory this would allow chaining multiple operations on the same map structure, but we don't seem to be using that ... this seems to be mostly because it allows the input to define the type of the output ( which does need to be a map ). Currently, it looks like we always set up the input mapping as an identity map ( ie. m_mapping[channelName] = channelName; for all channels ). So it would be an option to use the value part of these key/value pairs. For example, m_mapping['__black'] = "" or m_mapping['__black'] = ShufflePlug::explicitToken would indicate that this key should only be used by explicit reference, not with wildcards. Pretty low discoverability, but not that ugly, and I think it would be trivial to implement? It would just be the switch from IECore::StringAlgo::match( srcName, srcPattern ) to srcValue != explicitToken && IECore::StringAlgo::match( srcName, srcPattern ). I guess you'd also need to decide whether whatever this feature is named also implied that it shouldn't be added to the destinationContainer in ShufflesPlug, or whether you stick with rejecting these when building m_outChannelNames like was happening before 6dea107

It's totally reasonable if you want to add a more discoverable option, but if you want a minimal change that would get GafferImage::Shuffle working, I think this would work.

[johnhaddon]

it would be an option to use the value part of these key/value pairs. For example, m_mapping['__black'] = "" or m_mapping['__black'] = ShufflePlug::explicitToken would indicate that this key should only be used by explicit reference, not with wildcards.

We're already using the value part to tell us which input channel to map to the output (key part) though. And in the general case (ShuffleAttributes etc), the value part isn't even a string. Something similar would be possible by augmenting the key part with a special suffix instead, something like __white:ExplicitToken, but then we have to do string munging to get the key for a lookup.

I've pushed an alternative approach that I think I'm reasonably happy with in the end : replacing shuffleWithDefaultSource() with shuffleWithExtraSources(). That's in the first two fixup commits (the third one is just a trivial whitespace fix).

[johnhaddon]

Pushed a version with all the fixups squashed in.