feat: Add a render queue submission feature for different camera options#233
feat: Add a render queue submission feature for different camera options#233anika-suman-amazon wants to merge 10 commits intoaws-deadline:mainlinefrom
Conversation
…ased Signed-off-by: Anika Suman <211736242+anika-suman-amazon@users.noreply.github.com>
…-deadline#225) Signed-off-by: Anika Suman <211736242+anika-suman-amazon@users.noreply.github.com> docs: Updated CHANGELOG.md to include reason for 0.4.1 not being released (aws-deadline#226) Signed-off-by: Anika Suman <211736242+anika-suman-amazon@users.noreply.github.com>
Signed-off-by: client-software-ci <129794699+client-software-ci@users.noreply.github.com>
Signed-off-by: Anika Suman <211736242+anika-suman-amazon@users.noreply.github.com>
Signed-off-by: Anika Suman <211736242+anika-suman-amazon@users.noreply.github.com>
Signed-off-by: Anika Suman <211736242+anika-suman-amazon@users.noreply.github.com>
Signed-off-by: Anika Suman <211736242+anika-suman-amazon@users.noreply.github.com>
anika-suman-amazon
requested review from
crowecawcaw,
joel-wong-aws and
karthikbekalp
Signed-off-by: Anika Suman <211736242+anika-suman-amazon@users.noreply.github.com>
anika-suman-amazon
force-pushed
the
render-queue-submission
branch
from
7ec463e to
90e2044
Compare
| output_dir = os.path.dirname(output_path) | ||
| print(f"Requested output directory: {output_dir}") |
There was a problem hiding this comment.
left over debug statement?
| camera = data.get("camera", lux.getCamera()) | ||
|
|
||
| print(f"Setting camera to: {camera}") | ||
| print(f"lux.getCamera() result: {lux.getCamera()}") |
There was a problem hiding this comment.
Leftover debug statement?
| "groupLabel": "KeyShot Settings", | ||
| }, | ||
| "description": f"The render output path {label}.", | ||
| "default": "{{Param.OutputFilePath}}", |
There was a problem hiding this comment.
This is a reference to a different output job parameter, which isn't valid. We shouldn't include a default value if there is no reasonable default.
|
|
||
| render_selections = lux.getInputDialog( | ||
| title="Select Cameras to Render", | ||
| values=render_dialog_items, |
There was a problem hiding this comment.
Why did we choose to put this in a separate dialog, rather than the same dialog? (I recall there was a reason for it, but if we can document that in code, it would be helpful for future developers)
| active_camera = lux.getCamera() | ||
|
|
||
| if len(cameras) == 1: | ||
| file_selections["selected_cameras"] = [cameras[0]] |
There was a problem hiding this comment.
Can we make "selected_cameras" a shared constant?
| break | ||
|
|
||
| if not param_found: | ||
| settings.parameter_values.append({"name": param_name, "value": camera_output_path}) |
There was a problem hiding this comment.
It could also be helpful to save the camera_output_path as the default value for the param_name job parameters here.
| "filename": "init-data.yaml", | ||
| "type": "TEXT", | ||
| "data": "scene_file: '{{Param.KeyShotFile}}'\noutput_file_path: '{{Param.OutputFilePath}}'\noutput_format: 'RENDER_OUTPUT_{{Param.OutputFormat}}'\noverride_render_device: {{Param.OverrideRenderDevice}}\nrender_device: '{{Param.RenderDevice}}'\nrender_options: {add_to_queue: false, advanced_samples: 0, background_rendering: false, engine_anti_aliasing: 1, engine_caustics_quality: 0.0, engine_dof_quality: 3.0, engine_global_illumination: 0.0, engine_global_illumination_cache: true, engine_indirect_bounces: 1, engine_pixel_blur: 1.5, engine_ray_bounces: 6, engine_shadow_quality: 1.0, engine_sharp_shadows: true, engine_sharper_texture_filtering: false, engine_threads: 14, output_alpha_channel: false, output_ambient_occlusion_pass: false, output_caustics_pass: false, output_clown_pass: false, output_depth_pass: false, output_diffuse_pass: false, output_direct_lighting_pass: false, output_indirect_lighting_pass: false, output_labels_pass: false, output_normals_pass: false, output_raw_pass: false, output_reflection_pass: false, output_refraction_pass: false, output_render_layers: false, output_shadow_pass: false, progressive_max_samples: 0, progressive_max_time: 0.0, region: null, render_mode: 2, send_to_network: false, __VERSION: 5}\n" | ||
| "data": "scene_file: '{{Param.KeyShotFile}}'\noutput_file_path: '{{Param.OutputFilePath}}'\noutput_format: 'RENDER_OUTPUT_{{Param.OutputFormat}}'\noverride_render_device: {{Param.OverrideRenderDevice}}\nrender_device: '{{Param.RenderDevice}}'\ncamera: '{{Param.Camera}}'\nrender_options: {add_to_queue: false, advanced_samples: 0, background_rendering: false, engine_anti_aliasing: 1, engine_caustics_quality: 0.0, engine_dof_quality: 3.0, engine_global_illumination: 0.0, engine_global_illumination_cache: true, engine_indirect_bounces: 1, engine_pixel_blur: 1.5, engine_ray_bounces: 6, engine_shadow_quality: 1.0, engine_sharp_shadows: true, engine_sharper_texture_filtering: false, engine_threads: 14, output_alpha_channel: false, output_ambient_occlusion_pass: false, output_caustics_pass: false, output_clown_pass: false, output_depth_pass: false, output_diffuse_pass: false, output_direct_lighting_pass: false, output_indirect_lighting_pass: false, output_labels_pass: false, output_normals_pass: false, output_raw_pass: false, output_reflection_pass: false, output_refraction_pass: false, output_render_layers: false, output_shadow_pass: false, progressive_max_samples: 0, progressive_max_time: 0.0, region: null, render_mode: 2, send_to_network: false, __VERSION: 5}\n" |
There was a problem hiding this comment.
Out of scope for this PR, but we should add an integration test where there are multiple cameras to test the new flow.
| job_template = submitter.construct_job_template(filename) | ||
|
|
||
| assert job_template["name"] == filename | ||
| assert job_template["name"] == filename |
There was a problem hiding this comment.
supernit: can we leave the assert unindented? The job_template also exists outside of the mock and the output could theoretically be affected by the mock (albeit not in this case), so I think it's better to leave it outside the mock.
Similar note for the asserts below.
|
|
||
|
|
||
| def test_construct_job_template_timeout_values(): | ||
| with mock.patch.object(submitter.lux, "getCameras", return_value=[]): |
There was a problem hiding this comment.
Is an empty list for getCameras possible? The test data should replicate some realistic data
Also, we should probably add this as an autoused fixture def mock_lux_get_cameras():
| mock.patch.object(submitter.lux, "getCameras", return_value=[]), | ||
| ): | ||
| submitter.lux.RENDER_ENGINE_PRODUCT_GPU = RENDER_ENGINE_PRODUCT_GPU | ||
| submitter.lux.RENDER_ENGINE_INTERIOR_GPU = RENDER_ENGINE_INTERIOR_GPU |
There was a problem hiding this comment.
We either need an integration or unit test for the new flow with multiple cameras :)
|
This is looking really good! I've added a bunch of comments but they are mostly around better handling edge cases and remove some redundant logic. This is a great step towards addressing #191 :) |
Signed-off-by: Anika Suman <211736242+anika-suman-amazon@users.noreply.github.com>
anika-suman-amazon
force-pushed
the
render-queue-submission
branch
from
2ea849e to
960d3e1
Compare
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| # def test_get_camera_safe_name(): | ||
| # assert submitter.get_camera_safe_name("Camera 1") == "Camera_1" | ||
| # assert submitter.get_camera_safe_name("Camera/Test") == "Camera_Test" | ||
| # assert submitter.get_camera_safe_name("Camera & Light") == "Camera_and_Light" | ||
| # assert submitter.get_camera_safe_name("Camera-View") == "Camera_View" |
Check notice
Code scanning / CodeQL
Commented-out code Note test
4 participants
Fixes (partially): #191
What was the problem/requirement? (What/Why)
Right now, the Deadline Cloud integration with KeyShot does not allow the user to submit multiple scenes at once, with different combinations of cameras/viewpoints, colorways, model sets, lighting options, etc., reducing efficiency as they instead have to submit one at a time. This update allows the user to render multiple cameras at once. The other settings are more difficult to access with the KeyShot API available to us, so we would need to contact KeyShot to proceed with adding the other options to make this feature even more robust.
What was the solution? (How)
I added a new dialog box to allow the user to select which cameras out of all available camera views the user wants to render. Then, I made it so each camera option added a new step to the job template so that all of them could be rendered with one job.
What is the impact of this change?
Increases efficiency and ease for customers, allowing them to render multiple camera viewpoints at once.
How was this change tested?
I tested against the unit tests, adjusting where needed, and I also tested with the integration tests and by creating a Conda package with my changes.
Was this change documented?
N/A
Is this a breaking change?
Yes, changes have been made to the adaptor.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.