Skip to content

Refactor runRenderer to use structured types #2869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ashwinbhat wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Refactor runRenderer to use structured types #2869

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
97cae93
Refactor runRenderer to use structured types
ashwinbhat Apr 20, 2026
1904ded
Ensure the return status is correctly reported
ashwinbhat Apr 20, 2026
56aacf1
Address co-pilot comment about shadername generation
ashwinbhat Apr 20, 2026
ad7a4af
Address Copilot review comments
ashwinbhat Apr 21, 2026
1c17ae3
Merge branch 'main' into bhata/testrender_refactor
ashwinbhat Apr 21, 2026
d5e0b55
Minor update based on Copilot feedback
ashwinbhat Apr 21, 2026
ac55117
Store DocumentPtr in RenderItem
ashwinbhat Apr 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 20 additions & 27 deletions source/MaterialXTest/MaterialXRender/RenderUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,24 +57,25 @@ bool ShaderRenderTester::validate(const mx::FilePath optionsFilePath)
// Data search path
runState.searchPath = mx::getDefaultDataSearchPath();

mx::ScopedTimer ioTimer(&profiler.times().ioTime);
mx::FilePathVec files = collectTestFiles(runState);
ioTimer.endTimer();
mx::FilePathVec files;
{
mx::ScopedTimer t(&profiler.times().ioTime);
files = collectTestFiles(runState);
}

// Load in the library dependencies once.
// This will be imported in each test document below.
ioTimer.startTimer();
loadDependentLibraries(runState);
ioTimer.endTimer();
{
mx::ScopedTimer t(&profiler.times().ioTime);
loadDependentLibraries(runState);
}

// Create renderers and generators
initializeGeneratorContext(runState, logger, profiler);

// Map to replace "/" in Element path and ":" in namespaced names with "_".
mx::StringMap pathMap;
pathMap["/"] = "_";
pathMap[":"] = "_";
RenderSession session { runState.options, logger.renderLog() };

bool allSucceeded = true;
for (const mx::FilePath& filename : files)
{
DocumentInfo docInfo = loadAndValidateDocument(filename, runState, logger, profiler);
Expand All @@ -84,17 +85,20 @@ bool ShaderRenderTester::validate(const mx::FilePath optionsFilePath)

for (const auto& element : docInfo.elements)
{
mx::string elementName = mx::createValidName(mx::replaceSubstrings(element->getNamePath(), pathMap));
runRenderer(elementName, element, *runState.context, docInfo.doc, logger.renderLog(), runState.options,
profiler.times(), docInfo.imageSearchPath, docInfo.outputPath, nullptr);
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;
Comment on lines +108 to +113
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
}
Comment on lines +108 to 114
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
}

// Print profiling summary on the normal path.
// All resource cleanup is handled by destructors.
profiler.printSummary(runState.options, logger.profilingLog());

return true;
return allSucceeded;
}

void ShaderRenderTester::loadDependentLibraries(TestRunState& runState)
Expand Down Expand Up @@ -486,11 +490,10 @@ DocumentInfo ShaderRenderTester::loadAndValidateDocument(const mx::FilePath& fil
{
DocumentInfo info;

mx::ScopedTimer ioTimer(&profiler.times().ioTime);

if (_skipFiles.count(filename.getBaseName()) > 0)
return info;

mx::ScopedTimer ioTimer(&profiler.times().ioTime);
info.doc = mx::createDocument();
try
{
Expand Down Expand Up @@ -589,13 +592,6 @@ void TestRunLogger::start(const std::string& target, const GenShaderUtil::TestSu
#endif
}

TestRunLogger::~TestRunLogger()
{
_renderLog.reset();
_validationLog.reset();
_profilingLog.reset();
}

// ---------------------------------------------------------------------------
// TestRunProfiler
// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -646,10 +642,7 @@ void TestRunTracer::start(const std::string& target, const GenShaderUtil::TestSu
}
}

TestRunTracer::~TestRunTracer()
{
_state.reset();
}
TestRunTracer::~TestRunTracer() = default;

#endif // MATERIALX_BUILD_PERFETTO_TRACING

Expand Down
80 changes: 66 additions & 14 deletions source/MaterialXTest/MaterialXRender/RenderUtil.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,19 @@ class LanguageProfileTimes
output << "\tI/O: " << ioTime << " seconds" << std::endl;
output << "\tImage save: " << imageSaveTime << " seconds" << std::endl;
}

void accumulate(const LanguageProfileTimes& other)
{
totalTime += other.totalTime;
setupTime += other.setupTime;
transparencyTime += other.transparencyTime;
generationTime += other.generationTime;
compileTime += other.compileTime;
renderTime += other.renderTime;
ioTime += other.ioTime;
imageSaveTime += other.imageSaveTime;
}

double totalTime = 0.0;
double setupTime = 0.0;
double transparencyTime = 0.0;
Expand Down Expand Up @@ -104,8 +117,6 @@ struct DocumentInfo
class TestRunLogger
{
public:
~TestRunLogger();

void start(const std::string& target, const GenShaderUtil::TestSuiteOptions& options);

std::ostream& renderLog() { return _renderLog ? *_renderLog : std::cout; }
Expand All @@ -124,7 +135,7 @@ class TestRunLogger
class TestRunProfiler
{
public:
~TestRunProfiler() { _totalTimer.reset(); }
~TestRunProfiler() = default;

void start();
void printSummary(const GenShaderUtil::TestSuiteOptions& options,
Expand Down Expand Up @@ -161,6 +172,51 @@ struct TestRunState
std::unique_ptr<mx::GenContext> context;
};

// Read-only test configuration for the entire validate() run.
// Note: the log stream requires synchronization for concurrent use.
struct RenderSession
{
const GenShaderUtil::TestSuiteOptions& testOptions;
std::ostream& log;
};

// Per-element data passed to each runRenderer call.
struct RenderItem
{
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)
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
{
mx::StringMap pathMap;
pathMap["/"] = "_";
pathMap[":"] = "_";
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

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;
}();

Copilot uses AI. Check for mistakes.
shaderName = mx::createValidName(
mx::replaceSubstrings(element->getNamePath(), pathMap));
}

mx::TypedElementPtr element;
mx::FileSearchPath imageSearchPath;
mx::FilePath outputPath;
mx::ImageVec* imageVec = nullptr;
std::string shaderName;

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

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

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; }

Copilot uses AI. Check for mistakes.
};

// Returned by runRenderer — each call produces its own isolated profiling data.
// The caller accumulates results, making future parallelism straightforward.
struct RenderProfileResult
{
LanguageProfileTimes languageTimes;
unsigned int elementsTested = 0;
bool success = true;
};

// Base class used for performing compilation and render tests for a given
// shading language and target.
//
Expand Down Expand Up @@ -209,17 +265,13 @@ class ShaderRenderTester
// Create a renderer for the generated code
virtual void createRenderer(std::ostream& log) = 0;

// Run the renderer
virtual bool runRenderer(const std::string& shaderName,
mx::TypedElementPtr element,
mx::GenContext& context,
mx::DocumentPtr doc,
std::ostream& log,
const GenShaderUtil::TestSuiteOptions& testOptions,
RenderProfileTimes& profileTimes,
const mx::FileSearchPath& imageSearchPath,
const std::string& outputPath = ".",
mx::ImageVec* imageVec = nullptr) = 0;
// Run the renderer.
// GenContext is a separate argument because it is mutable (written per-element)
// and must be cloned per-thread for future parallel execution.
virtual RenderProfileResult runRenderer(
const RenderSession& session,
const RenderItem& item,
mx::GenContext& context) = 0;

// Save an image
virtual bool saveImage(const mx::FilePath&, mx::ConstImagePtr, bool) const { return false; };
Expand Down
68 changes: 32 additions & 36 deletions source/MaterialXTest/MaterialXRenderGlsl/RenderGlsl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,10 @@ class GlslShaderRenderTester : public RenderUtil::ShaderRenderTester

void createRenderer(std::ostream& log) override;

bool runRenderer(const std::string& shaderName,
mx::TypedElementPtr element,
mx::GenContext& context,
mx::DocumentPtr doc,
std::ostream& log,
const GenShaderUtil::TestSuiteOptions& testOptions,
RenderUtil::RenderProfileTimes& profileTimes,
const mx::FileSearchPath& imageSearchPath,
const std::string& outputPath = ".",
mx::ImageVec* imageVec = nullptr) override;
RenderUtil::RenderProfileResult runRenderer(
const RenderUtil::RenderSession& session,
const RenderUtil::RenderItem& item,
mx::GenContext& context) override;

bool saveImage(const mx::FilePath& filePath, mx::ConstImagePtr image, bool verticalFlip) const override;

Expand Down Expand Up @@ -151,22 +145,23 @@ bool GlslShaderRenderTester::saveImage(const mx::FilePath& filePath, mx::ConstIm
return _renderer->getImageHandler()->saveImage(filePath, image, verticalFlip);
}

bool GlslShaderRenderTester::runRenderer(const std::string& shaderName,
mx::TypedElementPtr element,
mx::GenContext& context,
mx::DocumentPtr doc,
std::ostream& log,
const GenShaderUtil::TestSuiteOptions& testOptions,
RenderUtil::RenderProfileTimes& profileTimes,
const mx::FileSearchPath& imageSearchPath,
const std::string& outputPath,
mx::ImageVec* imageVec)
RenderUtil::RenderProfileResult GlslShaderRenderTester::runRenderer(
const RenderUtil::RenderSession& session,
const RenderUtil::RenderItem& item,
mx::GenContext& context)
{
RenderUtil::RenderProfileResult result;
const std::string& shaderName = item.shaderName;
mx::DocumentPtr doc = item.doc();
mx::TypedElementPtr element = item.element;
const GenShaderUtil::TestSuiteOptions& testOptions = session.testOptions;
std::ostream& log = session.log;

MX_TRACE_FUNCTION(mx::Tracing::Category::Render);
MX_TRACE_SCOPE(mx::Tracing::Category::Material, shaderName.c_str());
std::cout << "Validating GLSL rendering for: " << doc->getSourceUri() << std::endl;

mx::ScopedTimer totalGLSLTime(&profileTimes.languageTimes.totalTime);
mx::ScopedTimer totalGLSLTime(&result.languageTimes.totalTime);

const mx::ShaderGenerator& shadergen = context.getShaderGenerator();
mx::FileSearchPath searchPath = mx::getDefaultDataSearchPath();
Expand All @@ -180,13 +175,13 @@ bool GlslShaderRenderTester::runRenderer(const std::string& shaderName,

for (auto options : optionsList)
{
profileTimes.elementsTested++;
result.elementsTested++;

mx::FilePath outputFilePath = outputPath;
mx::FilePath outputFilePath = item.outputPath;

// Note: mkdir will fail if the directory already exists which is ok.
{
mx::ScopedTimer ioDir(&profileTimes.languageTimes.ioTime);
mx::ScopedTimer ioDir(&result.languageTimes.ioTime);
outputFilePath.createDirectory(true);

// Use separate directory for reduced output
Expand All @@ -201,11 +196,11 @@ bool GlslShaderRenderTester::runRenderer(const std::string& shaderName,
mx::ShaderPtr shader;
try
{
mx::ScopedTimer transpTimer(&profileTimes.languageTimes.transparencyTime);
mx::ScopedTimer transpTimer(&result.languageTimes.transparencyTime);
options.hwTransparency = mx::isTransparentSurface(element, shadergen.getTarget());
transpTimer.endTimer();

mx::ScopedTimer generationTimer(&profileTimes.languageTimes.generationTime);
mx::ScopedTimer generationTimer(&result.languageTimes.generationTime);
MX_TRACE_SCOPE(mx::Tracing::Category::ShaderGen, "GenerateShader");
mx::GenOptions& contextOptions = context.getOptions();
contextOptions = options;
Expand All @@ -223,7 +218,8 @@ bool GlslShaderRenderTester::runRenderer(const std::string& shaderName,
if (shader == nullptr)
{
log << ">> Failed to generate shader\n";
return false;
result.success = false;
return result;
}
const std::string& vertexSourceCode = shader->getSourceCode(mx::Stage::VERTEX);
const std::string& pixelSourceCode = shader->getSourceCode(mx::Stage::PIXEL);
Expand All @@ -233,7 +229,7 @@ bool GlslShaderRenderTester::runRenderer(const std::string& shaderName,
if (testOptions.dumpGeneratedCode)
{
MX_TRACE_SCOPE(mx::Tracing::Category::Render, "DumpGeneratedCode");
mx::ScopedTimer dumpTimer(&profileTimes.languageTimes.ioTime);
mx::ScopedTimer dumpTimer(&result.languageTimes.ioTime);
std::ofstream file;
file.open(shaderPath + "_vs.glsl");
file << vertexSourceCode;
Expand Down Expand Up @@ -285,15 +281,15 @@ bool GlslShaderRenderTester::runRenderer(const std::string& shaderName,

{
MX_TRACE_SCOPE(mx::Tracing::Category::Render, "CompileShader");
mx::ScopedTimer compileTimer(&profileTimes.languageTimes.compileTime);
mx::ScopedTimer compileTimer(&result.languageTimes.compileTime);
_renderer->createProgram(shader);
_renderer->validateInputs();
}

if (testOptions.dumpUniformsAndAttributes)
{
MX_TRACE_SCOPE(mx::Tracing::Category::Render, "DumpUniformsAndAttributes");
mx::ScopedTimer printTimer(&profileTimes.languageTimes.ioTime);
mx::ScopedTimer printTimer(&result.languageTimes.ioTime);
log << "* Uniform:" << std::endl;
program->printUniforms(log);
log << "* Attributes:" << std::endl;
Expand Down Expand Up @@ -353,8 +349,8 @@ bool GlslShaderRenderTester::runRenderer(const std::string& shaderName,

{
MX_TRACE_SCOPE(mx::Tracing::Category::Render, "RenderMaterial");
mx::ScopedTimer renderTimer(&profileTimes.languageTimes.renderTime);
_renderer->getImageHandler()->setSearchPath(imageSearchPath);
mx::ScopedTimer renderTimer(&result.languageTimes.renderTime);
_renderer->getImageHandler()->setSearchPath(item.imageSearchPath);
unsigned int width = (unsigned int) testOptions.renderSize[0] * supersampleFactor;
unsigned int height = (unsigned int) testOptions.renderSize[1] * supersampleFactor;
_renderer->setSize(width, height);
Expand All @@ -364,7 +360,7 @@ bool GlslShaderRenderTester::runRenderer(const std::string& shaderName,

{
MX_TRACE_SCOPE(mx::Tracing::Category::Render, "CaptureAndSaveImage");
mx::ScopedTimer ioTimer(&profileTimes.languageTimes.imageSaveTime);
mx::ScopedTimer ioTimer(&result.languageTimes.imageSaveTime);
std::string fileName = shaderPath + "_glsl.png";
mx::ImagePtr image = _renderer->captureImage();
if (image)
Expand All @@ -374,9 +370,9 @@ bool GlslShaderRenderTester::runRenderer(const std::string& shaderName,
image = image->applyBoxDownsample(supersampleFactor);
}
_renderer->getImageHandler()->saveImage(fileName, image, true);
if (imageVec)
if (item.imageVec)
{
imageVec->push_back(image);
item.imageVec->push_back(image);
}
}
}
Expand Down Expand Up @@ -409,7 +405,7 @@ bool GlslShaderRenderTester::runRenderer(const std::string& shaderName,
CHECK(validated);
}
}
return true;
return result;
}

TEST_CASE("Render: GLSL TestSuite", "[renderglsl]")
Expand Down
Loading
⚡