feat: add Arnold AOV support - #266
Conversation
leongdl
left a comment
There was a problem hiding this comment.
Thanks for this contribution — it fixes two real blockers for Arnold animation jobs (per-frame AOV filename collisions and the scene-baked outputPath), and the quality bar here is high: the docstrings explaining why Arnold diverges from the V-Ray render-element path are exactly what a future maintainer needs, _iter_drivers cleanly quarantines the defensive pymxs access pattern, the cache-once-across-frames test is the right regression test, and the verification story (OpenJD runner + real Deadline Cloud submission) is stronger than most PRs.
I traced the three flows (scene load → _apply_path_mapping; per-frame start_render → _configure_renderer_output; session end → cleanup_render_elements → restore) and have three changes to request plus one recommendation.
Requested changes
1. Asymmetric pymxs undefined handling can leak the literal string "undefined" into filenames and the restore cache
_apply_path_mapping explicitly normalizes pymxs's "undefined" stringification, but the suffix read in _configure_renderer_output uses str(drv.filenameSuffix or ""). pymxs undefined is not guaranteed falsy across MAXtoA versions — if it's truthy, a driver with an unset suffix produces files named frame0001_undefined.exr, and worse, "undefined" gets cached and written back into the scene at cleanup. Suggest a small shared helper (e.g. _normalize_mxs_str(value) -> str that returns "" for None/"undefined") used in both places.
2. Two empty-suffix drivers still collide with each other
The _AOV safety tag correctly distinguishes empty-suffix AOV output from the beauty pass, but two drivers that both have empty suffixes compose to the same <output_name>_AOV.<ext> and overwrite each other — the exact failure mode this PR exists to fix, just one level down. Suggest including the driver index in the fallback: f"{output_name}_AOV{i}".
3. Misleading log: the _apply_path_mapping remap is always clobbered before any render
Every render path goes through start_render → _configure_renderer_output, which unconditionally sets aov_mgr.outputPath = output_dir (and start_render raises earlier if output_dir is unset). So the remapped value never survives to a render — yet the code logs "Remapped Arnold AOV outputPath: X -> Y", which will send anyone debugging output paths from worker logs chasing a value that was overwritten. Either drop the outputPath remap from _apply_path_mapping, or keep it as an explicit documented fallback and reword the log so it can't be mistaken for the effective render-time path (e.g. "...(will be overridden by job output directory at render time)").
Recommended (non-blocking)
- The PR description mentions tests for the
"undefined"and unchanged-path remap cases, but I don't see them in the diff — adding them would also pin the fix for item 1. - Test helper
_make_handler_with_aov_managerstarts thertpatcher before returning it; if anything raises betweenstart()and the caller'stry(e.g. handler construction),rtstays patched for the rest of the test session. A pytest fixture or@contextmanageryielding(handler, aov_mgr, drivers)fixes that and deletes all eighttry/finally patcher.stop()blocks.
Edge cases I checked and cleared, so you don't need to re-investigate: non-Arnold renderer active at map time (caught, warns, no-ops), multi-frame suffix compounding (prevented by cache-once, tested), restore raising mid-loop (finally clears the cache), and output_dir creation ordering (base creates it after the hook, before rt.render).
Happy to approve once items 1–3 are addressed — they're all small, same-file changes.
🤖 AI-assisted review 🤖
|
Summary. (me trying to understand the problem) PR #266 fixes two real blockers for Arnold animation jobs: per-frame AOV filename collisions (every frame overwrote the last, since Arnold names AOV files purely from Call-stack traces Flow 1 — scene load / path mapping:
Flow 2 — per frame:
Flow 3 — session end:
|
355a170 to
f0523a5
Compare
f0523a5 to
f9d7eac
Compare
181968c to
64ded63
Compare
f7e9dbe to
1cacf1a
Compare
Wire Arnold AOVs through a dedicated render handler so per-frame AOV filenames are unique and AOV_Manager.outputPath is remapped to the worker's output directory. Arnold AOVs are managed by renderer.AOV_Manager (not the standard render element manager), so the handler drives the manager directly.
- Per frame, force AOV_Manager.outputPath to the job output directory and inject the resolved output_name into each driver's filenameSuffix so frames no longer overwrite each other.
- Capture original suffixes once (positionally) and restore them at session end; empty-suffix drivers fall back to _AOV<index> to avoid collisions.
- Normalize pymxs None/'undefined' via _normalize_mxs_str so the sentinel never leaks into filenames or the restore cache.
- check_renderer matches on startswith('Arnold'); _configure_renderer_output returns False so the framework still writes the beauty pass.
- Add unit tests for suffix composition, per-frame non-compounding, empty-suffix fallback, undefined normalization, path-mapping no-ops, and suffix restore.
Signed-off-by: Zain Ali <55154081+ZainAallii@users.noreply.github.com>
1cacf1a to
059e23c
Compare
| self.log_to_console( | ||
| f"Warning: failed to restore Arnold AOV_Manager.outputPath: {exc}" | ||
| ) | ||
| for i, drv in enumerate(aov_mgr.drivers): |
There was a problem hiding this comment.
aov_mgr.drivers is the one pymxs access in this method not wrapped in an except. If it's throws here, the finally clears the cache but the exception escapes before super().cleanup_render_elements(data) runs — so the standard render-element restore silently gets skipped
| ``_restore_arnold_aov_drivers`` so its name reflects what it actually | ||
| does. | ||
| """ | ||
| self._restore_arnold_aov_drivers() |
There was a problem hiding this comment.
this cleanup hook only gets enqueued when enabled_modify_render_elements is true (see on_cleanup in adaptor.py). So for a typical Arnold AOV job, this restore never actually runs, is this what we expected?
| @@ -1,10 +1,11 @@ | |||
| """ | |||
There was a problem hiding this comment.
The description and the call-stack comment above both mention an _apply_path_mapping override that isn't actually in the diff anymore?
None of the test touch path remapping (the "undefined" one is about suffix normalization). Mind adding some?
|
Re: the cleanup-hook gating question (discussion) — I traced this through That said, I consider this non-blocking. The gated cleanup means the restore machinery is dead, not wrong: on a worker the scene copy is discarded at session end anyway, so the missing restore has zero production impact today. Its only cost is misleading docstrings that promise a restore guarantee the common path doesn't deliver. Suggested easy follow-up (either option, both adaptor-only):
Fine to land this PR without it and track as a follow-up issue. |
Wire Arnold AOVs through the render handler so per-frame AOV filenames are unique and AOV_Manager.outputPath is remapped to the worker's output directory. Arnold AOVs are managed by renderer.AOV_Manager (not the standard render element manager), so the handler drives the manager directly.
What was the problem/requirement? (What/Why)
Arnold AOVs did not render correctly for animations through the adaptor, for two reasons:
AOV_Managerdriver, and each file's basename comes from that driver'sfilenameSuffix(e.g.AOVs.exr). There is no per-frame token in the name, so every frame overwrote the previous frame's AOV output — a hard blocker for animation.AOV_Manager.outputPathis baked into the scene at author time and points at the artist's local filesystem, which isn't writable on a Deadline Cloud worker.Unlike V-Ray render elements, Arnold AOVs are managed by
renderer.AOV_Manager(anArnoldAOVsManagerreference target), not by 3ds Max's standardRenderElementManager, so this required driving the AOV manager directly from the handler.What was the solution? (How)
_apply_path_mappingremapsAOV_Manager.outputPaththrough the adaptor'smap_pathafter scene load._configure_renderer_outputsetsAOV_Manager.outputPathto the job's output directory and rewrites each driver'sfilenameSuffixto<output_name>_<original_suffix>, giving every frame a unique per-driver filename. If a driver's suffix is empty,_AOVis used as a safety tag so the AOV file doesn't collide with the beauty pass._restore_arnold_aov_drivers(invoked from the existingcleanup_render_elementsframework hook). Atry/finallyguarantees the cache is cleared even if a restore raises. The cache also prevents suffix compounding across frames within a session.check_rendererusesstartswith("Arnold")for version robustness.except Exceptionlogs a warning; no silent failures._configure_renderer_outputreturnsFalse, so the framework's existingrt.render(...)call still writes the main output. (Contrast withVrayHandler, which returnsTruein raw-output modes because V-Ray writes the beauty pass into its own container.)What is the impact of this change?
How was this change tested?
test_arnold_aov.pycovering path remapping (including"undefined"and unchanged paths), per-frame suffix mutation, the empty-suffix_AOVfallback, once-only caching / no compounding across frames, cleanup restoration, and the always-clear-cache path. Plus a versionedcheck_renderertest case intest_arnold_handler.py.hatch run test: full suite passes.globe_rotation.max(2 AOV drivers) across multiple frames. Per-frame logs confirmoutputPathremapping and per-driverfilenameSuffixmutation with no compounding; all frames exited 0 and uploaded to S3.Was this change documented?
Inline docstrings on the class (explaining the divergence from V-Ray's render-element path) and on each helper method. No user-facing docs required.
Did you modify schema files?
If yes, Did you bump the
integration_data_interface_versioninsrc/deadline/max_adaptor/MaxAdaptor/adaptor.pyand update the test constants?See the "Adaptor Schema Files" section in DEVELOPMENT.md for details.
Is this a breaking change?
No. The beauty pass is unchanged and non-AOV jobs are unaffected. Existing AOV jobs previously produced only the last frame's output; they now correctly produce one file per frame per driver.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.