Refactor runRenderer to use structured types#2869
Refactor runRenderer to use structured types#2869jstone-lucasfilm merged 11 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.
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.
- Set CATCH status - evaluate shadername in RenderItem constructor.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jstone-lucasfilm
left a comment
jstone-lucasfilm
left a comment
There was a problem hiding this comment.
I really like this proposal overall, @ashwinbhat, and I've written up two notes on potential improvements before we merge this work.
| mx::DocumentPtr document; | ||
| mx::FileSearchPath imageSearchPath; | ||
| mx::FilePath outputPath; | ||
| mx::ImageVec* imageVec = nullptr; |
There was a problem hiding this comment.
Would this variable be better as an ImageVec images member of RenderProfileResult, rather than a pointer-to-vector member of RenderItem?
That would allow function arguments of type const RenderItem& to remain truly constant, without making this vector a silently-mutable data member of RenderItem.
| mx::ImageVec* imageVec = nullptr; | ||
| std::string shaderName; | ||
|
|
||
| mx::DocumentPtr doc() const { return document; } |
There was a problem hiding this comment.
Now that document is a public member of RenderItem, do we still need this additional doc accessor?
jstone-lucasfilm
left a comment
jstone-lucasfilm
left a comment
There was a problem hiding this comment.
Thanks for this improvement, @ashwinbhat, and this looks good to me!
jstone-lucasfilm
merged commit e8c12a2
into
AcademySoftwareFoundation:main
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: