Skip to content

ShaderCompiler: preserve original sampler names colliding with HLSL/MSL keywords #1796

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
bkaradzic-microsoft wants to merge 1 commit into
base: master
Choose a base branch
from
+20 −10
Open

ShaderCompiler: preserve original sampler names colliding with HLSL/MSL keywords #1796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 19 additions & 9 deletions Plugins/ShaderCompiler/Source/ShaderCompilerCommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,12 +90,21 @@ namespace Babylon::ShaderCompilerCommon
}
}

void AppendSamplers(std::vector<uint8_t>& bytes, const spirv_cross::Compiler& compiler, const spirv_cross::SmallVector<spirv_cross::Resource>& samplers, std::map<std::string, uint8_t>& stages)
void AppendSamplers(std::vector<uint8_t>& bytes, const spirv_cross::Compiler& compiler, const spirv_cross::ParsedIR& originalIr, const spirv_cross::SmallVector<spirv_cross::Resource>& samplers, std::map<std::string, uint8_t>& stages)
{
for (const spirv_cross::Resource& sampler : samplers)
{
AppendBytes(bytes, static_cast<uint8_t>(sampler.name.size()));
AppendBytes(bytes, sampler.name);
// SPIRV-Cross's HLSL/MSL backends rename resources whose name collides with a reserved
// keyword of the target language (e.g. a GLSL sampler named "Texture2D" or "Texture2DArray"
// becomes "_Texture2D" because those are HLSL built-in object types). bgfx's uniform table
// and Babylon.js look samplers up by their original GLSL name, so the renamed identifier would
// never bind (the sampler silently samples nothing). Recover the pre-transpile name from the
// parser's ParsedIR (the Compiler transpiles a private copy, leaving the parser's names intact).
const std::string& originalName = originalIr.get_name(sampler.id);
const std::string& name = originalName.empty() ? sampler.name : originalName;

AppendBytes(bytes, static_cast<uint8_t>(name.size()));
AppendBytes(bytes, name);
AppendBytes(bytes, static_cast<uint8_t>(bgfx::UniformType::Sampler | BGFX_UNIFORM_SAMPLERBIT));

// TODO : These values (num, regIndex, regCount) are only used by Vulkan and should be set for that API
Expand All @@ -113,13 +122,14 @@ namespace Babylon::ShaderCompilerCommon
// That produced multiple sampler2D uniforms and a samplerCube all pointing at one unit,
// which GLES/ANGLE rejects at draw time with GL_INVALID_OPERATION (D3D11/Metal don't
// validate this, so the bug was GL-only). Each sampler now gets its own distinct unit,
// mirroring WebGL's Effect._bindSamplerUniformToChannel.
if (stages.find(sampler.name) == stages.end())
// mirroring WebGL's Effect._bindSamplerUniformToChannel. Keyed on the recovered
// pre-transpile name (see above) so both stages agree on the same identifier.
if (stages.find(name) == stages.end())
{
stages[sampler.name] = static_cast<uint8_t>(stages.size());
stages[name] = static_cast<uint8_t>(stages.size());
}
#else
stages[sampler.name] = static_cast<uint8_t>(compiler.get_decoration(sampler.id, spv::DecorationBinding));
stages[name] = static_cast<uint8_t>(compiler.get_decoration(sampler.id, spv::DecorationBinding));
#endif
}
}
Expand Down Expand Up @@ -298,7 +308,7 @@ namespace Babylon::ShaderCompilerCommon

AppendBytes(vertexBytes, static_cast<uint16_t>(numUniforms));
AppendUniformBuffer(vertexBytes, uniformsInfo, false);
AppendSamplers(vertexBytes, compiler, samplers, bgfxShaderInfo.UniformStages);
AppendSamplers(vertexBytes, compiler, vertexShaderInfo.Parser->get_parsed_ir(), samplers, bgfxShaderInfo.UniformStages);

AppendBytes(vertexBytes, static_cast<uint32_t>(vertexShaderInfo.Bytes.size()));
AppendBytes(vertexBytes, vertexShaderInfo.Bytes);
Expand Down Expand Up @@ -359,7 +369,7 @@ namespace Babylon::ShaderCompilerCommon

AppendBytes(fragmentBytes, static_cast<uint16_t>(numUniforms));
AppendUniformBuffer(fragmentBytes, uniformsInfo, true);
AppendSamplers(fragmentBytes, compiler, samplers, bgfxShaderInfo.UniformStages);
AppendSamplers(fragmentBytes, compiler, fragmentShaderInfo.Parser->get_parsed_ir(), samplers, bgfxShaderInfo.UniformStages);

AppendBytes(fragmentBytes, static_cast<uint32_t>(fragmentShaderInfo.Bytes.size()));
AppendBytes(fragmentBytes, fragmentShaderInfo.Bytes);
Expand Down
2 changes: 1 addition & 1 deletion Plugins/ShaderCompiler/Source/ShaderCompilerCommon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ namespace Babylon::ShaderCompilerCommon
};

void AppendUniformBuffer(std::vector<uint8_t>& bytes, const NonSamplerUniformsInfo& uniformBuffer, bool isFragment);
void AppendSamplers(std::vector<uint8_t>& bytes, const spirv_cross::Compiler& compiler, const spirv_cross::SmallVector<spirv_cross::Resource>& samplers, std::map<std::string, uint8_t>& stages);
void AppendSamplers(std::vector<uint8_t>& bytes, const spirv_cross::Compiler& compiler, const spirv_cross::ParsedIR& originalIr, const spirv_cross::SmallVector<spirv_cross::Resource>& samplers, std::map<std::string, uint8_t>& stages);
NonSamplerUniformsInfo CollectNonSamplerUniforms(spirv_cross::Parser& parser, const spirv_cross::Compiler& compiler);

/// Assigns unique descriptor bindings to every uniform buffer in the shader so that
Expand Down