ShaderCompiler: preserve original sampler names colliding with HLSL/MSL keywords - #1796
Conversation
…SL keywords SPIRV-Cross's HLSL/MSL backends rename any shader resource whose name collides with a target-language reserved keyword by prepending '_' (e.g. GLSL samplers 'Texture2D'/'Texture2DArray' become '_Texture2D'/'_Texture2DArray'). Babylon.js and bgfx look samplers up by their original GLSL name, so the renamed uniform never bound and the stage silently sampled nothing (transparent output). AppendSamplers now recovers the original pre-transpile name from the parser's ParsedIR (get_name(id)) -- the Compiler transpiles a private copy of the IR, so the parser retains the original names -- and emits that into the bgfx uniform table and the stage map. The texture register (DecorationBinding) is unchanged; only the app-facing name is fixed. Non-colliding samplers resolve to the identical string and are unaffected. The OpenGL stage-assignment guard is now keyed on the recovered name so the vertex and fragment passes agree on the same identifier. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
There was a problem hiding this comment.
Pull request overview
This PR fixes sampler/uniform binding failures caused by SPIRV-Cross renaming GLSL sampler identifiers that collide with HLSL/MSL reserved keywords (e.g., Texture2D → _Texture2D). It does so by emitting the original (pre-transpile) resource name into the bgfx uniform table and by using that same recovered name when maintaining the shared stage-to-texture-unit map.
Changes:
- Extend
AppendSamplersto accept the parser’s originalspirv_cross::ParsedIRand recover the pre-transpile sampler name viaoriginalIr.get_name(sampler.id). - Emit recovered sampler names into the bgfx shader binary uniform table (falling back to
sampler.namewhen unavailable). - Key the OpenGL stage-assignment guard and stage map entries on the recovered name so vertex/fragment passes agree on the identifier.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Plugins/ShaderCompiler/Source/ShaderCompilerCommon.h | Updates AppendSamplers signature to accept the original ParsedIR for name recovery. |
| Plugins/ShaderCompiler/Source/ShaderCompilerCommon.cpp | Recovers original sampler names from ParsedIR, writes them to the uniform table, and keys stage mapping consistently on the recovered name. |
The shader cache is keyed on a hash of the GLSL source, which this fix does not alter, but the cached payload does change: the bgfx uniform table inside VertexBytes/FragmentBytes now carries the original sampler name, and the keys of UniformStages change with it. Both are serialized by ShaderCacheImpl::Save. Without a version bump, an existing shaderCache.bin keeps deserializing pre-fix entries with the SPIRV-Cross-renamed identifiers, so the bug persists on any machine with a warm cache and gives no indication why. ShaderCacheImpl::Load already returns 0 when the stored version does not match CACHE_VERSION, so bumping to 3 discards stale caches and forces a recompile. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
|
Good catch, and thank you — you're right, and I'd missed it. Bumped to I traced it to confirm the exact failure mode you describe:
Net effect without the bump: on any machine with an existing cache the fix would appear to do nothing, with no diagnostic — which is a worse failure than the original bug, since it looks like the fix simply doesn't work. I added a short comment above the constant recording why 3 exists, so the next person changing the serialized shape has a precedent to follow. Verified: |
Problem
SPIRV-Cross's HLSL and MSL backends rename any shader resource whose name collides with a reserved keyword of the target language, prepending an underscore. Babylon.js generates GLSL samplers named
Texture2DandTexture2DArray, which are HLSL built-in object types, so they are emitted into the bgfx uniform table as_Texture2D/_Texture2DArray.Both bgfx and Babylon.js look samplers up by their original GLSL name, so the renamed uniform never binds. There is no error — the stage just silently samples nothing and renders transparent.
Fix
AppendSamplersrecovers the pre-transpile name from the parser'sParsedIRviaget_name(id). TheCompilertranspiles a private copy of the IR, so the parser still holds the original names. That name is emitted into the bgfx uniform table and the stage map.DecorationBinding) is untouched — only the app-facing name changes.Validation
Built
x64 / Release / D3D11against this branch and ran the full Playground validation suite:No regressions. Because every non-colliding sampler keeps its current name, the change is a no-op for all 300 currently-enabled tests.
Scope
This is intentionally code-only — no
config.jsonchanges. The test this unblocks (MultiRenderTarget with different texture types,#XSNYAU#22) also needs a matching Babylon.js-side fix for the mixed-type MRT write path, so its un-exclusion is deliberately left for a later change once both halves are in. Verified: with only this fix applied, that test still fails, so enabling it here would red the build.