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 1 commit
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
42 changes: 16 additions & 26 deletions source/MaterialXTest/MaterialXRender/RenderUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,23 +57,23 @@ 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() };

for (const mx::FilePath& filename : files)
{
Expand All @@ -84,9 +84,10 @@ 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;
}
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.
}

Expand Down Expand Up @@ -486,11 +487,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 +589,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 +639,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
88 changes: 74 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,59 @@ 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) {}

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

const std::string& shaderName() const
{
if (_cachedShaderName.empty())
{
mx::StringMap pathMap;
pathMap["/"] = "_";
pathMap[":"] = "_";
_cachedShaderName = mx::createValidName(
mx::replaceSubstrings(element->getNamePath(), pathMap));
}
return _cachedShaderName;
}
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::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 uses AI. Check for mistakes.

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

private:
mutable std::string _cachedShaderName;
};

// 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 +273,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
⚡