HYDRA-2450 : Improve speed of rendering with PRMan render delegate in our viewport unit tests - #482
HYDRA-2450 : Improve speed of rendering with PRMan render delegate in our viewport unit tests#482lanierd-adsk wants to merge 4 commits into
Conversation
lanierd-adsk
changed the title
There was a problem hiding this comment.
Pull request overview
This PR speeds up PRMan-based viewport snapshot tests by capturing directly from Hydra AOVs (instead of playblast) and by making PRMan convergence settling deterministic and early-exiting, addressing both CI runtime and local “black snapshot” failures.
Changes:
- Add Hydra AOV snapshot capture (
hydraSnapshot) and a non-blank image guard based on mean luminance. - Add PRMan test environment setup (fixed sample cap / no adaptive sampling) plus an interactive convergence polling helper.
- Wire PRMan lighting delegate tests to use Hydra writer + PRMan settle function; add standalone unit tests for the new helpers.
Reviewed changes
Copilot reviewed 8 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/testUtils/testRenderManUtils.py | Adds unit tests for PRMan setup/teardown and interactive convergence polling behavior. |
| test/testUtils/testImageUtils.py | New standalone unit tests for imageMeanLuminance() and assertImageNonBlank(). |
| test/testUtils/renderManUtils.py | Introduces PRMan sampling env controls and waitForInteractiveConvergence() polling logic. |
| test/testUtils/mtohUtils.py | Extends assertSnapshotClose wrapper to forward Hydra-writer capture options. |
| test/testUtils/imageUtils.py | Adds Hydra AOV snapshot capture, luminance computation, and blank-snapshot assertion. |
| test/lib/mayaUsd/render/mayaToHydra/testLightingRenderDelegates.py | Switches PRMan snapshots to Hydra writer + PRMan settle function; ensures required plugin/env ordering. |
| test/lib/mayaUsd/render/mayaToHydra/cpp/testWriteFile.py | Reuses the new imageUtils.WriteFile context manager instead of a duplicate implementation. |
| test/lib/mayaUsd/render/mayaToHydra/CMakeLists.txt | Registers the new testImageUtils.py unittest module in CMake. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lanierd-adsk
changed the title
|
|
||
| import maya.cmds as cmds | ||
| import fixturesUtils | ||
| import imageUtils |
There was a problem hiding this comment.
| import imageUtils | |
| from imageUtils import WriteFile |
| # Playblast reads Maya's model-editor framebuffer. Progressive delegates such as | ||
| # PRMan render into Hydra AOVs instead, so playblast can be blank locally even | ||
| # when the viewport looks correct. ImageBufferWriter reads the same Hydra buffer | ||
| # the override presents. Requires the mayaHydraCppTests plugin (TestWriteFile.*). |
There was a problem hiding this comment.
I don't understand this. Does this mean that playblast with RenderMan as the renderer (without Hydra) has never worked?
| # Prefer over snapshot()/playblast for progressive Hydra delegates (e.g. PRMan) | ||
| # that render into AOVs rather than the model-editor framebuffer. |
There was a problem hiding this comment.
Does this mean we need to have renderer-specific capture functions?
| total = 0.0 | ||
| count = 0 | ||
| for r, g, b, a in struct.iter_unpack("4B", buf): | ||
| if a: |
There was a problem hiding this comment.
So any non-zero alpha is treated the same way? But if there is a non-zero alpha in the image, the image is probably meaningful?
| # Usage: | ||
| # self.assertSnapshotClose("Storm_pointLight1.png", 0.1, 7.0) | ||
| # self.assertSnapshotClose("PRMan_pointLight1.png", 0.2, 10.0, | ||
| # useHydraWriter=True, hydraSettleFn=self._settlePrmanBeforeSnapshot) |
There was a problem hiding this comment.
I'm concerned about the renderer-specific parameter.
| hdPrman). Setting the equivalent defaultRenderGlobals.*__convergedSamplesPerPixel | ||
| attribute afterwards does *not* work as a substitute or complement: Maya | ||
| only re-applies render-globals attribute changes to the delegate via the | ||
| "mtohRenderOverride_ApplySetting" AE callback / mayaHydra -updateRenderGlobals | ||
| command, neither of which fires for a plain cmds.setAttr from a script. |
There was a problem hiding this comment.
Why is HdPrman using defaultRenderGlobals settings? We should definitely move away from this. Is this transitional?
| os.environ["HD_PRMAN_DISABLE_ADAPTIVE_SAMPLING"] = "1" | ||
| os.environ["HD_PRMAN_MAX_SAMPLES"] = str(PRMAN_TEST_MAX_SAMPLES) |
There was a problem hiding this comment.
I would hope there is a Hydra v2 render settings way of passing this information to HdPrman. Environment variables are hard to use and control, especially if the property value needs to change.
| # Usage: | ||
| # renderManUtils.waitForInteractiveConvergence() | ||
| # captureRefresh() | ||
| def waitForInteractiveConvergence( |
| Uses mayaHydraTesting(converged=True). Skips when convergenceTimeout is 0 | ||
| or absent (e.g. Storm, which converges immediately). Not called at all for | ||
| PRMan, which is captured via hydraSnapshot/hydraSettleFn instead -- see | ||
| use_hydra_writer in _runLightSnapshots and _settlePrmanBeforeSnapshot. | ||
| If the renderer does not report convergence within the timeout, proceeds anyway. |
There was a problem hiding this comment.
Renderer-specific logic doesn't sound like a lot of fun for future maintenance. I wonder what Pixar does for their own tests.
There was a problem hiding this comment.
Thanks for the work, lots of changes!
I am a bit concerned though with the overall approach for handling renderer convergence, I fear that it is overfitting for the specific task of supporting PRMan, instead of a more streamlined approach that would handle Storm, Arnold and PRMan. It seems like we would end up with a lot of code dedicated to handling renderer convergence, e.g. _settlePrmanBeforeSnapshot, convergenceTimeout, _waitForConvergence, hydraSettleFn, renderManUtils.waitForInteractiveConvergence, mayaHydraTesting(converged=True). This is a lot to keep track of; maybe there is some requirement(s) I am missing, but could we not have just one common path for handling convergence, and that always runs (e.g. in assertSnapshotClose), but just defaults the convergence timeout to 0? That way all renderers are treated the same, and non-converging renderers are essentially just renderers with a convergence of 0s.
Another general, more personal comment : I found many of the comments at the top of functions to be a bit heavy to read personally, I feel that in a lot of cases having them interspersed within the code would help readability (when possible obviously). With the current setup I had to do a first read of each block from start to finish, then read the code, and map the right sections of the code to the right sections of the comments, which didn't flow super well. If each sentence of the comments were right beside the code lines where they are relevant, I think it would make the overall reading flow better. We could also turn this into a rule for AI agents when writing code/adding comments, I'm probably going to do this at least locally for myself for now.
We can discuss further on Slack if you prefer
| assertImageNonBlank(snapImagePath) | ||
| else: | ||
| snapshot(snapImagePath) | ||
|
|
There was a problem hiding this comment.
This is tying the useHydraWriter option to the usage of a settling/convergence function and the non-blank image check, is this something we really want? I feel like all those things should be independent of each other
|
Seeing the number of comments from this PR I think I was going in the wrong direction. I am closing it and will restart from scratch. |
When using the PRMan render delegate in our viewport unit tests, this was adding between 10 and 20 mn to the preflight so this PR reduces the time taken to do a PRMan render delegate rendering in the viewport and also fixes when you try to locally run the test as the result was a black rendering while the viewport was correct.