GafferDispatch.FrameMask node #2558
Conversation
python/GafferDispatch/FrameMask.py
Outdated
| def __init__( self, name = "FrameMask" ) : | ||
|
|
||
| GafferDispatch.TaskNode.__init__( self, name ) | ||
| self["mask"] = Gaffer.StringPlug( defaultValue = "1-100x10" ) |
There was a problem hiding this comment.
this seems like an odd default to me.
There was a problem hiding this comment.
It's actually the same as the Dispatcher.frameRange plug, which I think was chosen on the grounds that it documents the syntax somewhat. I guess it does seem stranger here because it's a primary plug rather than being hidden until "Custom Range" is chosen like on the dispatcher. Would you prefer just ""? If so, does that pass through all frames or mask them all? I think I'd be inclined to have it as a pass-through.
There was a problem hiding this comment.
"" and pass-through makes sense to me
There was a problem hiding this comment.
Done in d740e8c - thanks for the suggestion.
| import Gaffer | ||
| import GafferDispatch | ||
|
|
||
| class FrameMask( GafferDispatch.TaskNode ) : |
There was a problem hiding this comment.
Should it be TaskFrameMask to match TaskContextVariables? I could see a future where we have SceneFrameMask and ImageFrameMask as well.
There was a problem hiding this comment.
I find it really hard to imagine SceneFrameMask and ImageFrameMask for some reason, and I'm not aware of anyone every having asked for it. Perhaps this is because in many cases the "enabled" plug can be used for a similar purpose?
I suggest we stick with the simpler FrameMask for now, knowing that we can easily use a compatibility config if we need to in the future.
There was a problem hiding this comment.
I find it really hard to imagine SceneFrameMask and ImageFrameMask for some reason, and I'm not aware of anyone every having asked for it.
See the IE published Box named "frameHold" which is essentially an ImageFrameMask. I coudn't find a published version for scenes, but I've definitely sent people the "SceneContextVariables with a variable called 'frame' and an expression driving the value" example many times.
There was a problem hiding this comment.
Network issues are preventing me checking those out, but holding a frame and masking it are different things aren't they? Holding means the image/scene still exists, but is taking it's data from a different upstream frame. Masking means the task no longer exists at all. I'm going to stick to my guns on this one, with the fallback that I'm perfectly happy to rename the node if the need arises in the future...
I've definitely sent people the "SceneContextVariables with a variable called 'frame' and an expression driving the value" example many times.
Any reason you're not sending them a SceneTimeWarp?
There was a problem hiding this comment.
holding a frame and masking it are different things aren't they? Holding means the image/scene still exists, but is taking it's data from a different upstream frame.
Yes, but inverting your expression turns the mask into a hold, right? I guess its better described as a frame clamp? The use case here is to dispatch an enormous range (0-3000) and use those boxes to make sure certain branches don't render outside a start/stop frame range. So they're defining the frames to keep rather than the frames to skip.
Any reason you're not sending them a SceneTimeWarp?
I guess because its easier to tell someone "make an expression that drives the frame however you want" than to do the same for speed and offset.
There was a problem hiding this comment.
Yes, but inverting your expression turns the mask into a hold, right? I guess its better described as a frame clamp?
Sorry, I'm not following, maybe just because I've been unable to see the frameHold example. To me, "hold" means a particular upstream frame is substituted in for some downstream frames. For instance, all frames greater than 10 might read frame 10 instead, so frame 10 is repeated. And "mask" means to ignore the tasks for some frames entirely. If you invert the mask, you're just ignoring a different set of frames, not holding anything. Nothing is repeated.
The use case here is to dispatch an enormous range (0-3000) and use those boxes to make sure certain branches don't render outside a start/stop frame range. So they're defining the frames to keep rather than the frames to skip.
Yeah, that's why the FrameMask node accepts the list of frames to keep.
There was a problem hiding this comment.
I think I'm mixing things up... lets just end this discussion, its gotten maddening.
| Any frames not included here will be ignored, regardless | ||
| of the dispatcher's frame range. | ||
|
|
||
| > Note : This can only remove frames. To add frames, edit the |
There was a problem hiding this comment.
Valid note, but perhaps a sign that the node is a bit more limiting than it could be. Is there a case for TaskTimeWarp or something along those lines?
There was a problem hiding this comment.
A TaskTimeWarp wouldn't add frames either - it would offset or multiply them. It's not impossible to add frames, but you'd have to declare them as preTasks of something, which would make for pretty odd looking job structures in something like Tractor. A FrameMask is a simple concept intended for a specific purpose that has come up multiple times, and I don't think additional features are needed.
If we truly do want to add frames (and that's not what was asked for here), then I think the way to do it is to have some sort of auto-discovery mechanism whereby the Dispatcher gets the valid frame ranges automatically by examining the TaskNodes. But even then, what would adding frames achieve? If an upstream task says it is only valid for a particular range, then logically we can only mask from that range anyway, not add to it.
There was a problem hiding this comment.
A TaskTimeWarp wouldn't add frames either - it would offset or multiply them.
Good point. Still, offsetting the range might be appropriate in same cases, like a frame 1-100 geometry (or vdb) cache, followed by several renders in the real shot range.
There was a problem hiding this comment.
Definitely. I'm not trying to say that TaskTimeWarp isn't a valid concept. I'm just saying that it's a different concept to a FrameMask, and would be implemented as a different node.
python/GafferDispatch/FrameMask.py
Outdated
| def preTasks( self, context ) : | ||
|
|
||
| frames = IECore.FrameList.parse( self["mask"].getValue() ).asList() | ||
| if context.getFrame() in frames : |
There was a problem hiding this comment.
I guess the behaviour here is that an empty mask acts as a blocker. Should empty be equivalent of pass-through (eg disabled) instead? I'm not sure which is better to be honest.
There was a problem hiding this comment.
Good point. I think I'm leaning to making the default value be "", and make this be a pass-through. Agreed?
There was a problem hiding this comment.
Yep, pass-through sounds good
|
If you fixup the failing test we can merge. Feel free to squash first. |
Fingers crossed, should be good to go now. Thanks for the review - the default behaviour is a definite improvement over what I first had. |
Needed by Alex, so I thought I'd squeeze it in quickly to finish off the day.