Skip to content

HYDRA-2096 : Support Hydra PRMan in our viewport unit tests#388

Open
lanierd-adsk wants to merge 48 commits intodevfrom
lanierd/HYDRA-2096
Open

HYDRA-2096 : Support Hydra PRMan in our viewport unit tests#388
lanierd-adsk wants to merge 48 commits intodevfrom
lanierd/HYDRA-2096

Conversation

@lanierd-adsk
Copy link
Copy Markdown
Collaborator

No description provided.

@lanierd-adsk lanierd-adsk self-assigned this Feb 27, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds Hydra PRMan (RenderMan) support to the viewport image-based unit tests and improves test/plugin discovery and convergence handling so progressive delegates can be validated reliably in CI.

Changes:

  • Add a new lighting snapshot test that runs per Hydra render delegate (Storm + PRMan; Arnold currently disabled).
  • Update MayaHydra convergence detection to check per-AOV render buffer convergence.
  • Enhance test configuration to append plugin paths (PXR_PLUGINPATH_NAME) and document how to configure delegate discovery/licensing.

Reviewed changes

Copilot reviewed 8 out of 19 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
test/testUtils/imageUtils.py Improves image-diff failure output with absolute baseline/actual paths for CI artifact linking.
test/testSamples/testLightingRenderDelegates/UVChecker.png Adds/updates a texture asset used by the lighting delegate test scene.
test/lib/mayaUsd/render/mayaToHydra/testLightingRenderDelegates.py New per-delegate lighting snapshot test, including PRMan texture-path handling and convergence waiting.
test/lib/mayaUsd/render/mayaToHydra/CMakeLists.txt Registers the new test script in the mayaToHydra test suite.
lib/mayaHydra/mayaPlugin/renderOverride.cpp Changes convergence logic to evaluate convergence via render buffers/AOVs.
lib/mayaHydra/mayaPlugin/pluginUtils.cpp Makes renderer settings registration resilient when delegate creation fails (e.g., missing GPU context).
lib/mayaHydra/hydraExtensions/adapters/cameraAdapter.cpp Adjusts camera clippingRange handling/typing for Hydra camera params.
doc/build.md Documents how to configure additional delegate plugin paths and PRMan-related test environment variables.
cmake/test.cmake Adds ADDITIONAL_PXR_PLUGINPATH_NAME, inherits plugin path env vars, and wires PRMan-related env vars into tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +994 to +1006
const auto* bufferManager = currentPass->GetRenderBufferManager().get();
if (!bufferManager) {
continue;
}

TfTokenVector renderOutputs = bufferManager->GetRenderOutputs();
if (renderOutputs.empty()) {
renderOutputs = GetAvailableFramePassAovs(i);
}
if (renderOutputs.empty()) {
TF_WARN("RenderOutputs list is empty; assuming converged.");
continue;
}
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new convergence check returns "converged" when a frame pass has no render buffer manager or when no render outputs/AOVs are reported (it just continues and ultimately returns true). This can mark a renderer converged before it has produced any buffers, which is the opposite of what tests and progressive delegates need. Consider treating missing bufferManager / empty renderOutputs as not converged (return false) so snapshots wait until buffers exist and report convergence.

Copilot uses AI. Check for mistakes.
Comment on lines +1004 to +1011
TF_WARN("RenderOutputs list is empty; assuming converged.");
continue;
}

for (const TfToken& aovToken : renderOutputs) {
HdRenderBuffer* buffer = currentPass->GetRenderBuffer(aovToken);
if (!buffer) {
TF_WARN(
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TF_WARN inside the per-frame render loop (empty RenderOutputs / missing render buffer) is likely to spam logs during progressive rendering, especially when buffers are created lazily. Consider downgrading to TF_DEBUG, rate-limiting, or warning once per renderer/AOV to keep CI logs actionable.

Suggested change
TF_WARN("RenderOutputs list is empty; assuming converged.");
continue;
}
for (const TfToken& aovToken : renderOutputs) {
HdRenderBuffer* buffer = currentPass->GetRenderBuffer(aovToken);
if (!buffer) {
TF_WARN(
TF_DEBUG("RenderOutputs list is empty; assuming converged.");
continue;
}
for (const TfToken& aovToken : renderOutputs) {
HdRenderBuffer* buffer = currentPass->GetRenderBuffer(aovToken);
if (!buffer) {
TF_DEBUG(

Copilot uses AI. Check for mistakes.
Comment on lines +80 to +96
def _setTextureSearchPathForPRMan(self):
"""Add the test scene directory to PRMan's texture search path.
The scene uses relative paths (./diffuse.png, ./UVChecker.png) which
PRMan resolves via RMAN_TEXTUREPATH. Without this, textures fail on
build machines where the working directory differs from the scene path.
"""
scenePath = getTestScene("testLightingRenderDelegates", "testLightingRenderDelegates.ma")
sceneDir = os.path.dirname(os.path.abspath(scenePath))
sep = ";" if platform.system() == "Windows" else ":"
existing = os.environ.get("RMAN_TEXTUREPATH", "")
newPath = sceneDir + (sep + existing if existing else "")
os.environ["RMAN_TEXTUREPATH"] = newPath

def loadScene(self):
self._setTextureSearchPathForPRMan()
mayaUtils.openTestScene("testLightingRenderDelegates", "testLightingRenderDelegates.ma")
cmds.refresh(force=True)
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loadScene() unconditionally mutates os.environ["RMAN_TEXTUREPATH"] for the whole test process, even when running the Storm delegate. This can leak state into subsequent tests in the same Maya session. Consider only setting RMAN_TEXTUREPATH when running the PRMan delegate and/or saving/restoring the previous value (e.g. in setUp/tearDown or a context manager).

Copilot uses AI. Check for mistakes.
Comment on lines +174 to +190
def test_EachLight_PerRenderDelegate(self):
"""For each render delegate (Storm, Arnold, PRMan), enable each Maya light one by one and compare snapshots to baseline."""
try:
for delegate in RENDER_DELEGATES:
if not self._delegateRunsOnPlatform(delegate):
continue
self.loadScene()
mayaPlugin = delegate.get("mayaPlugin")
if mayaPlugin:
with PluginLoaded(mayaPlugin):
self.assertTrue(
cmds.pluginInfo(mayaPlugin, query=True, loaded=True),
"{} plugin ({}) failed to load".format(delegate["name"], mayaPlugin),
)
self._runLightSnapshots(delegate)
else:
self._runLightSnapshots(delegate)
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test will hard-fail (not skip) on Windows machines where the HdPrman delegate isn't discoverable (missing PRMAN_DELEGATE_PLUGIN_PATH / license / install). Since _setRenderer() asserts the delegate plugin becomes active, any environment without PRMan configured will fail the whole test suite. Consider checking availability up front (e.g. cmds.mayaHydra(listRenderers=True) contains the delegate plugin/override) and self.skipTest(...) when PRMan isn't installed/configured.

Copilot uses AI. Check for mistakes.
finally:
# Switch back to Storm before teardown to avoid PRMan/mtoa shutdown
# hangs when Maya quits (similar to HYDRA-1242 isolate-select issue).
self._setRenderer(RENDER_DELEGATES[0])
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The finally block always calls _setRenderer(RENDER_DELEGATES[0]). If the test fails earlier (e.g. scene load or renderer switch) this cleanup can throw and mask the original failure. Consider guarding this with a try/except (or checking that a model panel and Storm override are available) so teardown doesn't overwrite the real assertion/error.

Suggested change
self._setRenderer(RENDER_DELEGATES[0])
try:
self._setRenderer(RENDER_DELEGATES[0])
except Exception:
# Best-effort cleanup: avoid masking original test failures
pass

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 19 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1031 to +1032
.Msg("Render output '%s' not found; ignoring.\n", aovToken.GetText());
continue;
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AOV-based convergence fallback ignores missing render buffers (GetRenderBuffer() returns null) and continues, which can incorrectly report convergence if some outputs haven’t been created yet (this file already notes AOVs may not exist yet). Consider treating a missing buffer for a reported render output as “not converged” (or restricting the check to a known-required AOV like HdAovTokens->color) rather than ignoring it.

Suggested change
.Msg("Render output '%s' not found; ignoring.\n", aovToken.GetText());
continue;
.Msg("Render output '%s' not found; assuming not converged.\n",
aovToken.GetText());
return false;

Copilot uses AI. Check for mistakes.
Comment on lines +62 to +72
{
"name": "PRMan",
"plugin": "HdPrmanLoaderRendererPlugin",
"override": "mayaHydraRenderOverride_HdPrmanLoaderRendererPlugin",
"mayaPlugin": None,
"convergenceTimeout": 30, # Wait up to 30s for convergence before snapshot
"platform": "windows", # Image comparison runs on Windows only
},
]


Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test unconditionally includes the PRMan delegate in RENDER_DELEGATES. On Windows machines without HdPrman installed/discoverable via PXR_PLUGINPATH_NAME, _setRenderer() will assert-fail and break the whole suite. Consider checking cmds.mayaHydra(listRenderers=True) (or similar) for the delegate plugin and skipping PRMan when it’s not available, so the test remains runnable outside specialized CI setups.

Suggested change
{
"name": "PRMan",
"plugin": "HdPrmanLoaderRendererPlugin",
"override": "mayaHydraRenderOverride_HdPrmanLoaderRendererPlugin",
"mayaPlugin": None,
"convergenceTimeout": 30, # Wait up to 30s for convergence before snapshot
"platform": "windows", # Image comparison runs on Windows only
},
]
]
# Only include PRMan delegate if its Hydra renderer plugin is available. On machines
# without HdPrman installed or discoverable via PXR_PLUGINPATH_NAME, attempting to
# set this renderer would assert-fail and break the test suite.
try:
_availableHydraRenderers = cmds.mayaHydra(listRenderers=True) or []
except Exception:
_availableHydraRenderers = []
if (
"HdPrmanLoaderRendererPlugin" in _availableHydraRenderers
or "mayaHydraRenderOverride_HdPrmanLoaderRendererPlugin" in _availableHydraRenderers
):
RENDER_DELEGATES.append(
{
"name": "PRMan",
"plugin": "HdPrmanLoaderRendererPlugin",
"override": "mayaHydraRenderOverride_HdPrmanLoaderRendererPlugin",
"mayaPlugin": None,
"convergenceTimeout": 30, # Wait up to 30s for convergence before snapshot
"platform": "windows", # Image comparison runs on Windows only
}
)

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's on purpose.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused. You essentially implemented the suggestion from Copilot. Shouldn't your answer be more like "Yes, that is what is implemented, using the if statement?

class TestLightingRenderDelegates(mtohUtils.MayaHydraBaseTestCase):
# MayaHydraBaseTestCase.setUpClass requirement.
_file = __file__
_requiredPlugins = ["mtoa"]
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_requiredPlugins includes mtoa, but this test currently has the Arnold delegate block commented out and the .ma scene doesn’t require MtoA nodes. Keeping mtoa as a hard requirement will unnecessarily skip/fail this test on setups that only have Storm (and/or PRMan). Consider removing mtoa from _requiredPlugins unless there’s a concrete dependency in the scene/test logic.

Suggested change
_requiredPlugins = ["mtoa"]
_requiredPlugins = []

Copilot uses AI. Check for mistakes.
testMaterialXOnNative.py|depOnPlugins:lookdevx
testArnoldLights.py|depOnPlugins:mtoa
testMayaLightingModes.py|depOnPlugins:mtoa
testLightingRenderDelegates.py|depOnPlugins:mtoa
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is registered with depOnPlugins:mtoa, but the new testLightingRenderDelegates.py doesn’t currently use Arnold and the referenced test scene doesn’t require MtoA. Consider dropping the mtoa dependency so the Storm portion can still run on environments without MtoA installed.

Suggested change
testLightingRenderDelegates.py|depOnPlugins:mtoa
testLightingRenderDelegates.py

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 19 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

return platform.system().lower() == required.lower()

def test_EachLight_PerRenderDelegate(self):
"""For each render delegate (Storm, Arnold, PRMan), enable each Maya light one by one and compare snapshots to baseline."""
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test docstring says it runs for "Storm, Arnold, PRMan", but the Arnold delegate block is currently commented out in RENDER_DELEGATES. Please update the docstring to reflect the delegates that actually run (or re-enable Arnold if that’s intended).

Suggested change
"""For each render delegate (Storm, Arnold, PRMan), enable each Maya light one by one and compare snapshots to baseline."""
"""For each render delegate (Storm and PRMan), enable each Maya light one by one and compare snapshots to baseline."""

Copilot uses AI. Check for mistakes.
Comment thread doc/build.md Outdated

**Arnold (MtoA):** When `MTOA_LOCATION` and `USD_VERSION` are defined, the HdArnold plugin path is added only if `plugInfo.json` is found: `${MTOA_LOCATION}/usd/bundle/<version>` (newer Arnold) is tried first, then `${MTOA_LOCATION}/usd/hydra/<version>` (older).

**RenderMan (PRMan):** For HdPrman tests, set `RMANTREE`, `RENDERMAN_LOCATION` (optional), `PIXAR_LICENSE_FILE` (license server, format: `port@hostname`), and `PRMAN_DELEGATE_PLUGIN_PATH` (path containing HdPrman plugInfo.json). On Windows, `RMANTREE/bin` and `RMANTREE/lib` are added to PATH.
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This RenderMan section implies that setting RMANTREE/PIXAR_LICENSE_FILE (etc.) is sufficient, but the test harness only auto-configures PATH and test ENV from CMake variables (see cmake/test.cmake), not from environment variables. Either clarify that these must be passed to CMake (e.g. -DRMANTREE=...) or add ENV fallbacks so the documentation matches actual behavior.

Suggested change
**RenderMan (PRMan):** For HdPrman tests, set `RMANTREE`, `RENDERMAN_LOCATION` (optional), `PIXAR_LICENSE_FILE` (license server, format: `port@hostname`), and `PRMAN_DELEGATE_PLUGIN_PATH` (path containing HdPrman plugInfo.json). On Windows, `RMANTREE/bin` and `RMANTREE/lib` are added to PATH.
**RenderMan (PRMan):** For HdPrman tests, you must provide the RenderMan locations to CMake as cache variables (not just shell environment variables). Pass `-DRMANTREE=...`, `-DRENDERMAN_LOCATION=...` (optional), `-DPIXAR_LICENSE_FILE=...` (license server, format: `port@hostname`), and `-DPRMAN_DELEGATE_PLUGIN_PATH=...` (path containing HdPrman `plugInfo.json`). On Windows, the test harness will add `${RMANTREE}/bin` and `${RMANTREE}/lib` to `PATH` based on these CMake variables.

Copilot uses AI. Check for mistakes.
Comment thread cmake/test.cmake
Comment on lines +431 to +455
# prman: delegate path (extracted package, not merged into USD) and runtime
# Platform selection driven by .yaml; if vars are set, configure test env.
if(DEFINED PRMAN_DELEGATE_PLUGIN_PATH AND NOT "${PRMAN_DELEGATE_PLUGIN_PATH}" STREQUAL "")
list(APPEND MAYAUSD_VARNAME_${PXR_OVERRIDE_PLUGINPATH_NAME}
"${PRMAN_DELEGATE_PLUGIN_PATH}")
endif()
if(DEFINED RMANTREE AND NOT "${RMANTREE}" STREQUAL "")
list(APPEND ALL_TEST_VARS RMANTREE)
set(MAYAUSD_VARNAME_RMANTREE "${RMANTREE}")
list(APPEND MAYAUSD_VARNAME_PATH "${RMANTREE}/bin")
list(APPEND MAYAUSD_VARNAME_PATH "${RMANTREE}/lib")
endif()
if(DEFINED RENDERMAN_LOCATION AND NOT "${RENDERMAN_LOCATION}" STREQUAL "")
list(APPEND ALL_TEST_VARS RENDERMAN_LOCATION)
set(MAYAUSD_VARNAME_RENDERMAN_LOCATION "${RENDERMAN_LOCATION}")
endif()
if(DEFINED PIXAR_LICENSE_FILE AND NOT "${PIXAR_LICENSE_FILE}" STREQUAL "")
list(APPEND ALL_TEST_VARS PIXAR_LICENSE_FILE)
set(MAYAUSD_VARNAME_PIXAR_LICENSE_FILE "${PIXAR_LICENSE_FILE}")
endif()
foreach(testvar RMANTREE RENDERMAN_LOCATION PIXAR_LICENSE_FILE)
if("${testvar}" IN_LIST ALL_TEST_VARS)
set_property(TEST "${test_name}" APPEND PROPERTY ENVIRONMENT
"${testvar}=${MAYAUSD_VARNAME_${testvar}}")
endif()
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PRMan env setup only triggers when RMANTREE/PIXAR_LICENSE_FILE/PRMAN_DELEGATE_PLUGIN_PATH are provided as CMake variables. If a user/CI job sets RMANTREE (common RenderMan practice) only in the environment, the tests won’t automatically prepend RMANTREE/bin and RMANTREE/lib to PATH (which is typically required on Windows for DLL discovery). Consider adding the same kind of ENV{} fallback used for ADDITIONAL_PXR_PLUGINPATH_NAME, or documenting that these must be passed via -DRMANTREE=... etc.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 19 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +162 to +163
time.sleep(0.5)
cmds.refresh(force=True)
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_waitForConvergence() silently proceeds after the timeout even if the renderer never reports convergence. That can make the snapshot comparison flaky and can also mask a real convergence regression (the test would just capture a non-converged frame). Consider asserting convergence at the end of the loop (or at least logging a clear warning including delegate name + elapsed time) so failures are actionable and deterministic.

Suggested change
time.sleep(0.5)
cmds.refresh(force=True)
time.sleep(0.5)
# If we exit the loop, the renderer never reported convergence within the timeout.
cmds.refresh(force=True)
elapsed = time.time() - start
rendererName = delegate.get("name", rendererPlugin)
self.fail(
"Renderer '{}' ({}) did not report convergence within {:.1f} seconds; "
"snapshot may be invalid.".format(rendererName, rendererPlugin, elapsed)
)

Copilot uses AI. Check for mistakes.
_requiredPlugins = []

IMAGE_DIFF_FAIL_THRESHOLD = 0.1
IMAGE_DIFF_FAIL_PERCENT = 7.0 #Images are non deterministic for shadows even with Storm.
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment style/grammar: add a space after # and consider using "non-deterministic" (hyphenated) for readability in this inline comment.

Suggested change
IMAGE_DIFF_FAIL_PERCENT = 7.0 #Images are non deterministic for shadows even with Storm.
IMAGE_DIFF_FAIL_PERCENT = 7.0 # Images are non-deterministic for shadows even with Storm.

Copilot uses AI. Check for mistakes.
@lanierd-adsk lanierd-adsk removed their assignment Mar 3, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 23 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +69 to 81
def imageDiff(imagePath1, imagePath2, verbose, fail, failpercent, hardfail=None,
warn=None, warnpercent=None, hardwarn=None, perceptual=False):
""" Returns the completed process instance after running idiff or None if
execution of process failed.

imagePath1 -- First image to compare.
imagePath2 -- Second image to compare.
verbose -- If enabled, the image diffing command will be printed to log.
fail -- The threshold for the acceptable difference (relatively to the mean of
the two values) of a pixel for failure.
fail -- The threshold for absolute pixel difference for failure.
failpercent -- The percentage of pixels that can be different before failure.
failrelative -- If set, uses relative difference (scaled by mean of two values). Use 0 for
strict pixel-per-pixel absolute comparison only.
hardfail -- Triggers a failure if any pixels are above this threshold (if the absolute
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imageDiff() docstring mentions a failrelative argument, but the function signature does not accept it and the implementation never sets idiff -failrelative. This makes the docs misleading (and suggests a missing feature). Either remove the failrelative documentation or add a failrelative parameter and pass it through to idiff when provided.

Copilot uses AI. Check for mistakes.
Comment on lines +184 to +188
try:
plugin_path = cmds.pluginInfo(MAYAUSD_PLUGIN_NAME, q=True, path=True)
sys.__stdout__.write("mayaUsdPlugin path: {}\n".format(plugin_path))
except Exception as e:
sys.__stdout__.write("mayaUsdPlugin path: (unavailable) {}\n".format(e))
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In _logUsdEnvironment(), plugin_path is defined inside a try/except, but then later referenced unconditionally in the macOS rpath block (_print_rpaths("mayaUsdPlugin", plugin_path)). If cmds.pluginInfo(..., path=True) throws, plugin_path will be undefined and this will raise NameError, causing the entire rpath dump to be skipped. Initialize plugin_path to None (or "") before the try and guard the later use.

Copilot uses AI. Check for mistakes.
Comment on lines +134 to +135
def _dump_script_editor_history(self, title):
"""Dump Script Editor history to stdout for CI visibility."""
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_dump_script_editor_history(self, title) takes a title argument but never uses it (the header prints only the path). Either include title in the BEGIN/END markers or remove the parameter to avoid confusion for callers passing different titles.

Copilot uses AI. Check for mistakes.
import mayaUtils
import mtohUtils
from testUtils import PluginLoaded, getTestScene
from imageUtils import snapshot
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snapshot is imported but never used in this test module. Removing the unused import avoids confusion about whether snapshots are being taken directly here vs. through assertSnapshotClose().

Suggested change
from imageUtils import snapshot

Copilot uses AI. Check for mistakes.
Comment thread cmake/test.cmake Outdated
Comment on lines +706 to +707
string(REPLACE ":" ";" _path_list "$ENV{${_inherit_var}}")
string(REPLACE ";" ";" _path_list "${_path_list}")
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line string(REPLACE ";" ";" _path_list "${_path_list}") is a no-op and makes it unclear what separators are intended to be handled when inheriting ${_inherit_var} on non-Windows platforms. Consider removing it, or (if the intent is to support both : and ; separators) explicitly normalizing both to a CMake list before iterating.

Suggested change
string(REPLACE ":" ";" _path_list "$ENV{${_inherit_var}}")
string(REPLACE ";" ";" _path_list "${_path_list}")
set(_path_list "$ENV{${_inherit_var}}")
string(REPLACE ":" ";" _path_list "${_path_list}")

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 23 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/testUtils/mtohUtils.py Outdated
Comment on lines +478 to +479
super(MayaHydraBaseTestCase, self).assertSnapshotSilhouetteClose(
refImagePath, fail, failpercent, hardfail, warn, warnpercent, hardwarn, perceptual)
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imageUtils.ImageDiffingTestCase.assertSnapshotSilhouetteClose() now includes a failrelative parameter after hardfail. This wrapper still passes positional arguments, so warn is currently being treated as failrelative and subsequent parameters are shifted. Please forward to the superclass using keyword arguments (and optionally expose failrelative here) to avoid incorrect image-diff thresholds.

Copilot uses AI. Check for mistakes.
Comment thread test/testUtils/mtohUtils.py Outdated
Comment on lines +438 to +440
super(MayaHydraBaseTestCase, self).assertImagesClose(
imagePath1, imagePath2, fail, failpercent, hardfail,
warn, warnpercent, hardwarn, perceptual)
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imageUtils.ImageDiffingTestCase.assertImagesClose() now has an extra failrelative parameter inserted after hardfail. This wrapper forwards args positionally, so warn is currently being passed as failrelative, and the remaining parameters shift as well. This will produce incorrect idiff CLI flags (and may cause unexpected failures). Forward to the superclass using keyword arguments (and optionally expose failrelative on this wrapper) so parameters map correctly.

Copilot uses AI. Check for mistakes.
Comment thread test/testUtils/mtohUtils.py Outdated
Comment on lines +458 to +460
super(MayaHydraBaseTestCase, self).assertSnapshotClose(
refImagePath, fail, failpercent, hardfail,
warn, warnpercent, hardwarn, perceptual, imageVersion=imageVersion)
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imageUtils.ImageDiffingTestCase.assertSnapshotClose() now accepts failrelative after hardfail. This wrapper forwards arguments positionally, so warn is being passed into failrelative and the remaining thresholds are shifted. Call the superclass with keyword arguments (and optionally add a failrelative parameter to this wrapper) to preserve correct behavior.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants