Refactor runRenderer to use structured types by ashwinbhat · Pull Request #2869 · AcademySoftwareFoundation/MaterialX

ashwinbhat · 2026-04-20T17:34:38Z

Replace the 10-parameter runRenderer signature with RenderSession, RenderItem, and RenderProfileResult value types.

Each call now returns isolated profiling data that the caller accumulates. Test run is now decoupled, so in future, we would be able to introduce some parallelism in these tests if needed.

Minor fixes:

correct ScopedTimer scoping in validate(),
fix a copy-paste timer name in the Slang renderer.

Replace the 10-parameter runRenderer signature with RenderSession, RenderItem, and RenderProfileResult value types. Each call now returns isolated profiling data that the caller accumulates. Test run is now decoupled, so in future, we would be able to introduce some parallelism in these tests if needed. Minor fixes: - correct ScopedTimer scoping in validate(), - fix a copy-paste timer name in the Slang renderer.

Copilot

Pull request overview

Refactors the render-test harness to replace the large runRenderer parameter list with structured value types, enabling per-call isolated profiling that the caller can aggregate (and paving the way for future parallelism).

Changes:

Replaces ShaderRenderTester::runRenderer’s 10-parameter signature with RenderSession, RenderItem, and RenderProfileResult.
Updates all render backends (GLSL/MSL/OSL/MDL/Slang) to return per-call profiling data and consume per-item/session inputs.
Adjusts timer scoping in validate() and removes now-unnecessary explicit logger/profiler destructor behavior.

Reviewed changes

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

Show a summary per file

File	Description
source/MaterialXTest/MaterialXRenderSlang/RenderSlang.cpp	Adapts Slang renderer test to new structured `runRenderer` API and per-call profiling result.
source/MaterialXTest/MaterialXRenderOsl/RenderOsl.cpp	Adapts OSL renderer test to new structured `runRenderer` API and per-call profiling result.
source/MaterialXTest/MaterialXRenderMsl/RenderMsl.mm	Adapts Metal renderer test to new structured `runRenderer` API and per-call profiling result.
source/MaterialXTest/MaterialXRenderMdl/RenderMdl.cpp	Adapts MDL renderer test to new structured `runRenderer` API and per-call profiling result.
source/MaterialXTest/MaterialXRenderGlsl/RenderGlsl.cpp	Adapts GLSL renderer test to new structured `runRenderer` API and per-call profiling result.
source/MaterialXTest/MaterialXRender/RenderUtil.h	Introduces `RenderSession`, `RenderItem`, `RenderProfileResult`, and adds `LanguageProfileTimes::accumulate`.
source/MaterialXTest/MaterialXRender/RenderUtil.cpp	Updates `validate()` to construct session/items, call new API, and accumulate per-call profiling.

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

Copilot · 2026-04-20T17:38:12Z

+    const std::string& shaderName() const
+    {
+        if (_cachedShaderName.empty())
+        {
+            mx::StringMap pathMap;
+            pathMap["/"] = "_";
+            pathMap[":"] = "_";
+            _cachedShaderName = mx::createValidName(
+                mx::replaceSubstrings(element->getNamePath(), pathMap));
+        }
+        return _cachedShaderName;
+    }


RenderItem::shaderName() mutates _cachedShaderName (via mutable) even though RenderItem is otherwise treated as read-only and is passed by const& for future parallel execution. If RenderItem instances are shared across threads later, this becomes a data race. Consider computing and storing the shader name eagerly in the constructor (or storing it as a non-mutable member) so shaderName() remains a pure read-only accessor, or otherwise ensure thread-safe lazy init.

Copilot · 2026-04-20T17:38:12Z

+            RenderItem item { element, docInfo.imageSearchPath, docInfo.outputPath, nullptr };
+            auto result = runRenderer(session, item, *runState.context);
+            profiler.times().languageTimes.accumulate(result.languageTimes);
+            profiler.times().elementsTested += result.elementsTested;
        }


runRenderer now returns a RenderProfileResult with a success flag, but validate() ignores it and always returns true after printing the summary. This makes the new success reporting ineffective and could hide failures if CHECK/REQUIRE behavior changes or if this utility is reused outside Catch2. Consider propagating result.success into an overall allSuccess and returning false (and/or skipping accumulation) when a renderer run fails, or remove the success field if it’s intentionally unused.

Copilot

Pull request overview

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

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

Copilot · 2026-04-20T21:12:39Z

+            RenderItem item { element, docInfo.imageSearchPath, docInfo.outputPath, nullptr };
+            auto result = runRenderer(session, item, *runState.context);
+            profiler.times().languageTimes.accumulate(result.languageTimes);
+            profiler.times().elementsTested += result.elementsTested;
+            if (!result.success)
+                allSucceeded = false;


validate() now returns allSucceeded based on RenderProfileResult::success, but success is only set to false on shader-generation failure. In the renderer implementations, render/compile failures typically result in validated staying false and only CHECK(validated) firing, leaving result.success true—so validate() can still return true despite failures. Consider setting result.success = result.success && validated (or setting it false in the exception paths) so the boolean return value reliably reflects test success.

Copilot · 2026-04-20T21:12:40Z

+    RenderItem(mx::TypedElementPtr elem,
+               mx::FileSearchPath searchPath,
+               mx::FilePath outPath,
+               mx::ImageVec* images = nullptr)
+        : element(std::move(elem)),
+          imageSearchPath(std::move(searchPath)),
+          outputPath(std::move(outPath)),
+          imageVec(images)


RenderUtil.h uses std::move in the RenderItem constructor but doesn't include <utility>, relying on transitive includes. Please add the explicit include to avoid non-portable compilation dependencies.

Copilot · 2026-04-20T21:12:40Z

+        mx::StringMap pathMap;
+        pathMap["/"] = "_";
+        pathMap[":"] = "_";


RenderItem rebuilds a mx::StringMap for the same two substitutions every time it's constructed. Since this runs once per renderable element, consider making the map static const (or using a small helper that avoids allocating a map) to reduce per-element overhead.

Suggested change

mx::StringMap pathMap;

pathMap["/"] = "_";

pathMap[":"] = "_";

static const mx::StringMap pathMap = []()

{

mx::StringMap substitutions;

substitutions["/"] = "_";

substitutions[":"] = "_";

return substitutions;

}();

- Set CATCH status - evaluate shadername in RenderItem constructor.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

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

Copilot · 2026-04-21T17:07:32Z


                // Set MDL search path for the module itself.
-                command += " --mdl_path \"" + outputPath + "\"";
+                command += " --mdl_path \"" + item.outputPath.asString() + "\"";


For reduced shader interface runs, the generated MDL module is written under outputFilePath (which appends reduced/), but the --mdl_path added for locating the module uses item.outputPath (the non-reduced parent). This can make testrender fail to find the module when options.shaderInterfaceType == SHADER_INTERFACE_REDUCED. Use the actual directory containing the generated module (e.g., outputFilePath) for this --mdl_path entry (and keep the parent path only if you intentionally want both).

Suggested change

command += " --mdl_path \"" + item.outputPath.asString() + "\"";

command += " --mdl_path \"" + outputFilePath.asString() + "\"";

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

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

Copilot · 2026-04-22T14:57:35Z

+    mx::FileSearchPath imageSearchPath;
+    mx::FilePath outputPath;
+    mx::ImageVec* imageVec = nullptr;
+    std::string shaderName;
+
+    mx::DocumentPtr doc() const { return element->getDocument(); }


RenderItem::doc() derives the document from element->getDocument(). In MaterialX, Element::getDocument() calls getRoot(), which throws ExceptionOrphanedElement if the element becomes orphaned (e.g., if the owning DocumentPtr has been destroyed). Since this refactor’s stated goal is to make per-element work items suitable for future parallelism/decoupling, RenderItem should own/retain the mx::DocumentPtr (or otherwise guarantee document lifetime) rather than reconstructing it from the element each time.

Suggested change

mx::FileSearchPath imageSearchPath;

mx::FilePath outputPath;

mx::ImageVec* imageVec = nullptr;

std::string shaderName;

mx::DocumentPtr doc() const { return element->getDocument(); }

mx::DocumentPtr document = element ? element->getDocument() : nullptr;

mx::FileSearchPath imageSearchPath;

mx::FilePath outputPath;

mx::ImageVec* imageVec = nullptr;

std::string shaderName;

mx::DocumentPtr doc() const { return document; }

ashwinbhat requested review from Copilot, jstone-lucasfilm and kwokcb April 20, 2026 17:34

ashwinbhat self-assigned this Apr 20, 2026

Copilot started reviewing on behalf of ashwinbhat April 20, 2026 17:35 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

ashwinbhat added 2 commits April 20, 2026 14:04

Ensure the return status is correctly reported

1904ded

Address co-pilot comment about shadername generation

56aacf1

ashwinbhat requested a review from Copilot April 20, 2026 21:08

Copilot started reviewing on behalf of ashwinbhat April 20, 2026 21:08 View session

Copilot AI reviewed Apr 20, 2026

View reviewed changes

ashwinbhat and others added 2 commits April 21, 2026 08:50

Address Copilot review comments

ad7a4af

- Set CATCH status - evaluate shadername in RenderItem constructor.

Merge branch 'main' into bhata/testrender_refactor

1c17ae3

ashwinbhat requested a review from Copilot April 21, 2026 17:03

Copilot started reviewing on behalf of ashwinbhat April 21, 2026 17:04 View session

Copilot AI reviewed Apr 21, 2026

View reviewed changes

Minor update based on Copilot feedback

d5e0b55

ashwinbhat requested a review from Copilot April 22, 2026 14:51

Copilot started reviewing on behalf of ashwinbhat April 22, 2026 14:52 View session

Copilot AI reviewed Apr 22, 2026

View reviewed changes

Store DocumentPtr in RenderItem

ac55117

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor runRenderer to use structured types#2869

Refactor runRenderer to use structured types#2869
ashwinbhat wants to merge 7 commits intoAcademySoftwareFoundation:mainfrom
autodesk-forks:bhata/testrender_refactor

ashwinbhat commented Apr 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI Apr 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 21, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-        mx::StringMap pathMap;
-        pathMap["/"] = "_";
-        pathMap[":"] = "_";
+        static const mx::StringMap pathMap = []()
+        {
+            mx::StringMap substitutions;
+            substitutions["/"] = "_";
+            substitutions[":"] = "_";
+            return substitutions;
+        }();

	command += " --mdl_path \"" + item.outputPath.asString() + "\"";
	command += " --mdl_path \"" + outputFilePath.asString() + "\"";

Conversation

ashwinbhat commented Apr 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants