Refactor runRenderer to use structured types#2869
Refactor runRenderer to use structured types#2869ashwinbhat wants to merge 3 commits intoAcademySoftwareFoundation:mainfrom
Conversation
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.
There was a problem hiding this comment.
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 withRenderSession,RenderItem, andRenderProfileResult. - 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.
| const std::string& shaderName() const | ||
| { | ||
| if (_cachedShaderName.empty()) | ||
| { | ||
| mx::StringMap pathMap; | ||
| pathMap["/"] = "_"; | ||
| pathMap[":"] = "_"; | ||
| _cachedShaderName = mx::createValidName( | ||
| mx::replaceSubstrings(element->getNamePath(), pathMap)); | ||
| } | ||
| return _cachedShaderName; | ||
| } |
There was a problem hiding this comment.
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.
| 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; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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; |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
| mx::StringMap pathMap; | ||
| pathMap["/"] = "_"; | ||
| pathMap[":"] = "_"; |
There was a problem hiding this comment.
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.
| mx::StringMap pathMap; | |
| pathMap["/"] = "_"; | |
| pathMap[":"] = "_"; | |
| static const mx::StringMap pathMap = []() | |
| { | |
| mx::StringMap substitutions; | |
| substitutions["/"] = "_"; | |
| substitutions[":"] = "_"; | |
| return substitutions; | |
| }(); |
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: