From ddaab5a32a3bd3e16687d66221e1b9b4bbfd6134 Mon Sep 17 00:00:00 2001 From: Jorijn van der Graaf Date: Tue, 6 Oct 2026 16:50:41 +0200 Subject: [PATCH] feat(shader): per-entry defines and a named output variant Crafter.Graphics has to ship each of its compute shaders in two builds of one source: one that indexes VK_EXT_descriptor_heap and one that reads the same slots from a descriptor set, for drivers without the extension. A Shader was a path, an entry point and a stage, and its output name was derived from the path alone, so one source could only be compiled once. Shader gains `defines`, spelled like the compiler's -D ("NAME" is defined to 1, "NAME=VALUE" to VALUE) and handed to glslang as a preamble, and `variant`, which names the output: "ui.comp.glsl" with variant "sets" becomes "ui.comp.sets.spv". OutputName() is the one place that name is formed; Check, Compile and the forwarding of a dependency's shaders into the consumer's bin dir all use it. Two fields make one mistake easy: defines set, variant forgotten. Both entries then compiled in parallel into the same file, the build reported success with whichever finished last, and Check() called both up to date from then on. The same has always been true of a.vert next to a.frag. Build() now refuses a configuration in which two entries share an output name and says which. A define without a name, or one spanning lines, and a variant with a path separator are rejected by name. Left to glslang the first two surface as "-1:" with no file, and the last as a failed open after the whole compile. Not addressed: Check() still compares only the source's mtime, so a change to the defines, or to an included file, does not recompile (#35). Co-Authored-By: Claude Fable 5.1 --- implementations/Crafter.Build-Clang.cpp | 22 +++++++++++-- implementations/Crafter.Build-Shader.cpp | 41 ++++++++++++++++++++++-- interfaces/Crafter.Build-Shader.cppm | 18 +++++++++++ 3 files changed, 77 insertions(+), 4 deletions(-) diff --git a/implementations/Crafter.Build-Clang.cpp b/implementations/Crafter.Build-Clang.cpp index d2aee22..05fa4b8 100644 --- a/implementations/Crafter.Build-Clang.cpp +++ b/implementations/Crafter.Build-Clang.cpp @@ -502,10 +502,28 @@ BuildResult Crafter::Build(Configuration& config, std::unordered_map buildCancelled{false}; + // Two entries with the same output name would compile in parallel into + // one file: whichever finished last won, the build reported success, and + // Check() then called both up to date. The usual way in is one source + // registered twice with different defines and no variant; a.vert next to + // a.frag is another. + { + std::unordered_map outputs; + for (const Shader& shader : config.shaders) { + const auto [existing, inserted] = outputs.emplace(shader.OutputName().string(), &shader); + if (inserted) continue; + buildError = std::format("shaders {} and {} both compile to {}: give one of them a distinct Shader::variant", existing->second->path.string(), shader.path.string(), existing->first); + buildCancelled.store(true); + break; + } + } for (const Shader& shader : config.shaders) { + if (buildCancelled.load(std::memory_order_relaxed)) break; if (shader.Check(outputDir)) continue; threads.emplace_back([&shader, &outputDir, &shaderIncludeDirs, &buildError, &buildCancelled]() { - Progress::Task task(std::format("Compiling shader {}", shader.path.filename().string())); + // A source registered more than once shows up once per variant; + // name the variant so the lines can be told apart. + Progress::Task task(shader.variant.empty() ? std::format("Compiling shader {}", shader.path.filename().string()) : std::format("Compiling shader {} ({})", shader.path.filename().string(), shader.variant)); if (buildCancelled.load(std::memory_order_relaxed)) return; std::string result = shader.Compile(outputDir, shaderIncludeDirs); @@ -878,7 +896,7 @@ BuildResult Crafter::Build(Configuration& config, std::unordered_mapBinDir(); for (const Shader& shader : dep->shaders) { - fs::path src = depBinDir / shader.path.filename().replace_extension("spv"); + fs::path src = depBinDir / shader.OutputName(); if (!fs::exists(src)) continue; copyTree(src, outputDir / src.filename()); } diff --git a/implementations/Crafter.Build-Shader.cpp b/implementations/Crafter.Build-Shader.cpp index e251c50..722e635 100644 --- a/implementations/Crafter.Build-Shader.cpp +++ b/implementations/Crafter.Build-Shader.cpp @@ -36,9 +36,17 @@ namespace { namespace Crafter { Shader::Shader(fs::path&& path, std::string&& entrypoint, ShaderType type) : path(std::move(path)), entrypoint(std::move(entrypoint)), type(type) { + } + fs::path Shader::OutputName() const { + // replace_extension swaps only the last extension, so a stage suffix + // survives: "ui.comp.glsl" becomes "ui.comp.spv", or "ui.comp.sets.spv" + // for the variant "sets". + fs::path name = path.filename(); + name.replace_extension(variant.empty() ? std::string("spv") : std::format("{}.spv", variant)); + return name; } bool Shader::Check(const fs::path& outputDir) const { - fs::path spv = outputDir / path.filename().replace_extension("spv"); + fs::path spv = outputDir / OutputName(); return fs::exists(spv) && fs::last_write_time(path) < fs::last_write_time(spv); } std::string Shader::Compile(const fs::path& outputDir, std::span includeDirs) const { @@ -52,6 +60,9 @@ namespace Crafter { auto fail = [&](std::string_view stage, std::string log) { glslang::FinalizeProcess(); std::string out = path.string(); + // Several variants of one source compile in parallel; without the + // variant the message would not say which of them failed. + if (!variant.empty()) out += std::format(" ({})", variant); out += ": "; out += stage; if (!log.empty()) { @@ -75,8 +86,34 @@ namespace Crafter { const char* shaderSource = src.data(); const std::int32_t shaderSourceLen = static_cast(src.size()); + // A variant is one component of a file name. Caught here, because a + // separator would otherwise surface as "failed to open SPIR-V output" + // after the whole compile. + if (variant.find_first_of("/\\") != std::string::npos) { + return fail(std::format("shader variant '{}' must not contain a path separator", variant), {}); + } + + // One #define line per entry, split like the compiler's -D: "NAME" + // defines it to 1, "NAME=VALUE" to VALUE. glslang places the preamble + // ahead of the whole source (it finds #version by scanning first), so + // line numbers in the user's file are unaffected. It keeps the + // pointer it is handed instead of copying, so the string is declared + // here to outlive parse() below. + std::string preamble; + for (const std::string& define : defines) { + const std::size_t eq = define.find('='); + const std::string name = define.substr(0, eq); + // An error inside the preamble is reported as "-1:" with no + // file and no macro name, so reject the malformed ones by name. + if (name.empty() || define.find_first_of("\r\n") != std::string::npos) { + return fail(std::format("invalid shader define '{}': expected NAME or NAME=VALUE on one line", define), {}); + } + preamble += std::format("#define {} {}\n", name, eq == std::string::npos ? std::string("1") : define.substr(eq + 1)); + } + glslang::TShader shader(glslangType); shader.setStringsWithLengthsAndNames(&shaderSource, &shaderSourceLen, fileNameList, 1); + if (!preamble.empty()) shader.setPreamble(preamble.c_str()); shader.setEntryPoint(entrypoint.c_str()); shader.setSourceEntryPoint(entrypoint.c_str()); shader.setEnvTarget(glslang::EShTargetSpv, glslang::EShTargetSpv_1_4); @@ -110,7 +147,7 @@ namespace Crafter { glslang::GlslangToSpv(*intermediate, spirv, &logger); std::string spvLog = logger.getAllMessages(); - fs::path filename = path.filename().replace_extension("spv"); + fs::path filename = OutputName(); std::ofstream file(outputDir/filename, std::ios::binary); if (!file) { return fail("failed to open SPIR-V output", (outputDir/filename).string()); diff --git a/interfaces/Crafter.Build-Shader.cppm b/interfaces/Crafter.Build-Shader.cppm index 4b4d841..b059cfb 100644 --- a/interfaces/Crafter.Build-Shader.cppm +++ b/interfaces/Crafter.Build-Shader.cppm @@ -28,7 +28,25 @@ namespace Crafter { fs::path path; std::string entrypoint; ShaderType type; + // Preprocessor macros for this compile, spelled like the compiler's -D: + // "NAME" (defined to 1) or "NAME=VALUE". They are placed ahead of the + // source, so one file can be registered more than once and take a + // different #ifdef branch each time. Note the spelling differs from + // Configuration::defines, which is a list of {name, value} pairs. + std::vector defines; + // Names the output when one source is compiled more than once: empty + // gives ".spv", "sets" gives ".sets.spv". One file-name + // component: no path separators. Every entry of a configuration must + // end up with its own OutputName(); Build() fails if two collide, so + // each extra compile of a source needs its own variant. Check() + // compares only the source's mtime: changing defines without touching + // the source leaves the old output in place. + std::string variant; Shader(fs::path&& path, std::string&& entrypoint, ShaderType type); + // File name of the compiled SPIR-V, relative to the output directory. + // The single source of truth for it: Check, Compile and the artifact + // forwarding in Build() all go through here. + fs::path OutputName() const; bool Check(const fs::path& outputDir) const; std::string Compile(const fs::path& outputDir, std::span includeDirs) const; };