ShufflePlug : Fix specific bug with matching names #5941
Conversation
danieldresser-ie
force-pushed
the
shuffleMatch
branch
from
5bac6b0
to
d7c924b
Compare
include/Gaffer/ShufflePlug.inl
Outdated
| @@ -142,7 +145,7 @@ T ShufflesPlug::shuffleInternal( const T &sourceContainer, const T *extraSources | |||
| if( ! dstPattern.empty() ) | |||
| { | |||
| const std::string dstName = scope.context()->substitute( dstPattern ); | |||
| if( srcName != dstName ) | |||
| if( srcName != dstName || usingExtraSource ) | |||
There was a problem hiding this comment.
I'm wondering why we had this srcName != dstName check in the first place. If we're doing a series of shuffles, any of which might have written to dstName already, it's not like we're detecting a no-op here. As far as I can tell, the only unit test that is affected is ShufflePlugTest.testIgnoreIdentity, which complains that multiple wildcard-matched sources are being shuffled to the same destination if we remove the check. But that seems like a pretty reasonable response to the input data, and it's not like the check would have helped if there were three members in the input data.
Your change is obviously more limited in scope, and I'd be happy to accept it, but I wonder if you might be feeling bold and want to rip out the check entirely? @murraystevenson, do you have any thoughts on this? Perhaps I'm missing an obvious reason why we must keep the check?
There was a problem hiding this comment.
My assumption was that this was just a minor optimization. If you consider a very simple ( and sloppy ) shuffle, like: R = R, G = G, B = __black, then having the check doesn't affect the result, but does mean that two channels get a slightly earlier passthrough ( since there is no shuffle created in the destinationContainer ).
But maybe I'm wrong that the intention was just a tiny optimization - if ShufflePlugTest.testIgnoreIdentity is failing, that implies that someone did think about this behaviour. I don't really like the behaviour though, I guess it's trying to be helpful, but it just seems inconsistent. Currently:
- if we shuffle R,G,B to R, R ends up set to B ( since it was last )
- if we shuffle R,G,B to G, G ends up set to B ( since it was last )
- if we shuffle R,G,B to B, B ends up set to G ( second last, because B was ignored )
I think that behaviour would be a lot easier to explain if we just ditched the check.
So yeah, as far as I can tell, everything is both simpler and better if we remove the check. And I guess this is already targetting main, so there's probably no reason to hold back.
There was a problem hiding this comment.
That gets my vote, unless @murraystevenson or @ericmehl has any objections.
There was a problem hiding this comment.
No objections. Simplicity gets my vote.
danieldresser-ie
force-pushed
the
shuffleMatch
branch
from
d7c924b
to
ccae215
Compare
|
Per conversation, I've completely switched this to just remove the counterproductive conditionals. |
As discussed this morning, you would expect a Shuffle mapping R to R with a missingSourceMode of Black to create a channel R with black in it if it didn't already exist, but due to a bug, this wasn't working.