-
Notifications
You must be signed in to change notification settings - Fork 203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add frame range to Blender command in CueSubmit #1337
Add frame range to Blender command in CueSubmit #1337
Conversation
Extracted from layerRange value in layerData Import regex module
7a29205
to
2009e12
Compare
Line too long
…/OpenCue into blender-framerange
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, there appears to be a TypeError popping up from rqdmachine.py
This looks unrelated to this change. Looks like that part of the code is scanning a file in /proc
to collect machine stats. Probably RQD is not familiar with the format of that file's content. Might need a separate PR.
Meanwhile, Cuebot is throwing a thread-related error.
This also looks unrelated to me. I could be wrong but I think it's nothing to worry about -- some race conditions are expected with Cuebot's task management and it should handle this case fine. @DiegoTavares can probably confirm.
cuesubmit/cuesubmit/Submission.py
Outdated
renderCommand += ' -f {}'.format(frameRange) | ||
elif re.match(r'^\d+-\d+$', frameRange): | ||
startFrame, endFrame = map(int, frameRange.split("-")) | ||
renderCommand += (' -s {startFrame} -e {endFrame} -a' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading this correctly, I think this will result in a layer command with the start/end frames hardcoded. I.e., if the frame range is 1-5
the command will contain -s 1 -e 5 -a
.
Compare this to the code prior to this change, which instead inserts the #IFRAME#
token to act as a placeholder for the frame.
I think instead, we want to use tokens for start frame / end frame:
OpenCue/cuesubmit/cuesubmit/Constants.py
Lines 45 to 54 in 5b9defd
COMMAND_TOKENS = {'#ZFRAME#': 'Current frame with a padding of 4', | |
'#IFRAME#': 'Current frame', | |
'#FRAME_START#': 'First frame of chunk', | |
'#FRAME_END#': 'Last frame of chunk', | |
'#FRAME_CHUNK#': 'Chunk size', | |
'#FRAMESPEC#': 'Full frame range', | |
'#LAYER#': 'Name of the Layer', | |
'#JOB#': 'Name of the Job', | |
'#FRAME#': 'Name of the Frame' | |
} |
These tokens should allow Cuebot to swap in the correct frame numbers for each task based on the chunk size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thread relate error on cuebot is normal, it's related to a race condition that's handled first come first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bcipriano noted.
I was under the impression that the Frame Spec field solely defined the frame range (1-5), hence why I got it directly from that but had no idea about the Chunk Size.
...should allow Cuebot to swap in the correct frame numbers for each task based on the chunk size.
Got it.
To make sure I follow correctly; Frame Spec can be populated with a frame range such as 1-5
, and then Chunk Size can be set as an integer value (eg: 2
) which defines the number of frames within that 1-5
range that will be processed per RQD node, yes?
And the dedicated #tokens can be used to substitute the respective values.
Sorry, I'm not very familiar with the concept of chunk sizes in render farms, hence the confusion 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you've got that right.
To extend the same example, if the frame range is 1-5
and the chunk size is 2
, the set of tasks for that layer should be:
- Frames
1-2
- Frames
3-4
- Frame
5
(don't go past the last frame, despite the chunk size setting)
Cuebot should handle all of this already -- dividing up the frame range into chunks and the replacement of the #FRAME_START#
/ #FRAME_END#
tokens -- so no need to do any of those calculations at this point in the code.
Kind of confusingly, in OpenCue each task is called a "frame". This is because when OpenCue was originally written, it only operated on single frames -- chunk size was always assumed to be 1
. We added the chunk size concept later, but the naming stayed the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bcipriano I've resolved this in ce618a0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, just a few minor requests.
@@ -72,8 +73,14 @@ def buildBlenderCmd(layerData): | |||
renderCommand += ' -o {}'.format(outputPath) | |||
if outputFormat: | |||
renderCommand += ' -F {}'.format(outputFormat) | |||
# The render frame must come after the scene and output | |||
renderCommand += ' -f {frameToken}'.format(frameToken=Constants.FRAME_TOKEN) | |||
if frameRange: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if you could add unit tests to cover both the single-frame and start/end frame cases.
https://github.com/AcademySoftwareFoundation/OpenCue/blob/master/cuesubmit/tests/Submission_tests.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 2d4ba88
@bcipriano however, I'm having a bit of trouble with lines #178 and #153 of Submission_tests.py
The test fails when I set it to blender
but passes when I set it to shell
. This is despite the layerType being set as Blender. Any thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping @bcipriano
Update comment explaining render frame range Blender command
For single frame and multi frame Blender renders
@n-jay Looks like we're getting some python unit test failures here, could you check those out? |
@bcipriano, yes I caught this failure and was hoping for some feedback: #1337 (comment) 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@n-jay Sorry, missed that. See new comment.
cuesubmit/tests/Submission_tests.py
Outdated
'blenderFile': '/path/to/scene.blend', | ||
'outputFormat': 'PNG'}, | ||
'layerRange': '3-9', | ||
'cores': '2' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to add a services
line like the Maya / Nuke tests do?
I think this is causing the job to default to the shell
service, so the assertion on line 153 fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bcipriano noted, didn't catch that one. 😄
Resolved in a17be62
A new set of unit test failures seem to be coming from some unrelated changes.
Unsure, but I think the CueGUI fail is caused by this one: 65b74fe
CC: @DiegoTavares
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that, I'm fixing the pipeline on muster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pipeline fixed by #1360
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted @DiegoTavares, thanks for the update!
d6e859e
into
AcademySoftwareFoundation:master
…ation#1337) * Add frame range to Blender render command Extracted from layerRange value in layerData Import regex module * Fix linting issue Line too long * Add variables for frame range tokens * Update Blender command with tokens for animation frame range * Remove unused import * Add suffix to frame start/end variables * Update comment Update comment explaining render frame range Blender command * Add unit tests For single frame and multi frame Blender renders * Add missing services line to blender unit test layer data
…ation#1337) * Add frame range to Blender render command Extracted from layerRange value in layerData Import regex module * Fix linting issue Line too long * Add variables for frame range tokens * Update Blender command with tokens for animation frame range * Remove unused import * Add suffix to frame start/end variables * Update comment Update comment explaining render frame range Blender command * Add unit tests For single frame and multi frame Blender renders * Add missing services line to blender unit test layer data
Link the Issue(s) this Pull Request is related to.
#1334
#1264
Summarize your change.
Adds frame range to the Blender render command in CueSubmit.
Required for when rendering animations.
Related Blender command linked here.