Test Shader compilation with WEBMIN flag#1638
Conversation
CedricGuillemet
changed the title
…nto TestShaderCompilation # Conflicts: # Dependencies/CMakeLists.txt
…nto TestShaderCompilation # Conflicts: # Apps/UnitTests/CMakeLists.txt
…nto TestShaderCompilation
…nto TestShaderCompilation # Conflicts: # Apps/UnitTests/CMakeLists.txt
…raversers. UniformTypeChangeTraverser and MoveNonSamplerUniformsIntoStruct only excluded samplers and matrices, so they happily visited the symbols representing whole `layout(std140) uniform` blocks (EbtBlock) and `uniform Foo foo;` struct instances (EbtStruct), rewriting them to vec4. Once that happened, spirv-cross emitted unnamed placeholder uniforms (e.g. `uniform vec4 _1195;`) and the matrix multiplies collapsed to scalar nonsense, then bgfx asserted on the empty uniform name (`Identifier can't be empty.`). Restrict both traversers to scalar/vector basic types (Float/Int/Uint/ Bool) so blocks and structs are left intact. Also throw early in CollectNonSamplerUniforms if a uniform name is empty, so any future regression surfaces in the compiler instead of as a deep bgfx assert. Add a TODO in CMakeLists.txt to point SPIRV-Cross back to the upstream BabylonJS fork once the corresponding PR there is merged.
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive shader cross-compilation unit test and tightens shader AST traversers to avoid incorrectly rewriting UBO blocks and struct uniforms, preventing empty/placeholder uniform identifiers from reaching bgfx.
Changes:
- Restrict uniform traversers to basic scalar/vector types to avoid visiting
EbtBlock/EbtStructuniforms. - Add a UnitTests scenario that compiles a large WebGL2 GLSL shader to exercise SPIRV-Cross with the WEBMIN flag.
- Update the fetched SPIRV-Cross dependency to a specific fork/commit and add an early compiler-side guard for empty uniform names.
Reviewed changes
Copilot reviewed 7 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Plugins/ShaderCompiler/Source/ShaderCompilerTraversers.cpp | Limits traversers to avoid rewriting UBO blocks/struct uniforms. |
| Plugins/ShaderCompiler/Source/ShaderCompilerCommon.cpp | Adds early failure when a reflected uniform name is empty. |
| CMakeLists.txt | Changes SPIRV-Cross FetchContent repository/commit. |
| Apps/UnitTests/Source/Tests.Shaders.Cross.cpp | Adds a new runtime-backed shader compilation unit test. |
| Apps/UnitTests/JavaScript/webpack.config.js | Adds a new webpack entry for the shader cross test. |
| Apps/UnitTests/JavaScript/src/tests.shaders.cross.ts | New “comprehensive GLSL” test script. |
| Apps/UnitTests/JavaScript/dist/tests.shaders.cross.js | Built webpack output for the new test script. |
| Apps/UnitTests/JavaScript/dist/tests.shaderCache.basicScene.js | Updated built output (webpack bootstrap changes). |
| Apps/UnitTests/CMakeLists.txt | Includes new JS asset and C++ test source in UnitTests target. |
| // We only care about loose scalar/vector uniforms. Excluding matrices, samplers, | ||
| // UBO blocks (EbtBlock) and uniform struct instances (EbtStruct) prevents the | ||
| // traverser from rewriting whole blocks/structs to vec4, which destroys their | ||
| // member layout and member names. | ||
| const auto basic = type.getBasicType(); | ||
| if (type.getQualifier().isUniformOrBuffer() | ||
| && !type.isMatrix() | ||
| && (basic == EbtFloat || basic == EbtInt || basic == EbtUint || basic == EbtBool)) | ||
| { |
There was a problem hiding this comment.
[Responded by Copilot on behalf of @bghgary]
The hardcoded EbtFloat vec4 at L330–331 does coerce int/uint/bool to float, but this PR doesn't change that. The old condition (!= EbtSampler && !isMatrix()) already let EbtInt/EbtUint/EbtBool through into the same rewrite, so behavior for those types is unchanged here. The narrow goal — excluding EbtBlock/EbtStruct — is correctly achieved.
The new test only checks compilation doesn't throw, so an int→float coercion wouldn't fail it. Proper preservation (ivec4/uvec4/bvec4) is worth a follow-up issue, but doesn't block this PR.
CedricGuillemet
changed the title
| @@ -0,0 +1,76 @@ | |||
| #include <gtest/gtest.h> | |||
There was a problem hiding this comment.
[Reviewed by Copilot on behalf of @bghgary]
Naming nit: the existing pattern in this folder is Tests.<SuiteName>.cpp with one dot after Tests. and the filename segment matching the gtest TEST() first arg — Tests.ExternalTexture.cpp → TEST(ExternalTexture, …), Tests.ShaderCache.cpp → TEST(ShaderCache, …), etc. This file is Tests.Shaders.Cross.cpp + TEST(ShadersCross, …), which breaks both (two dots in the filename, and the suite name reads oddly).
Suggestion: Tests.ShaderCompilation.cpp + TEST(ShaderCompilation, CompileComprehensiveGLSL). The test exercises the whole compile pipeline (ShaderMaterial → glslang → spirv-cross → bgfx), not just the ShaderCompiler class directly, so "ShaderCompilation" matches what's actually under test.
…lemet/BabylonNative into TestShaderCompilation
Use a generated shader to check no mandatory glsl function missing in spirv-cross when using WEBMIN flag.
BabylonJS/SPIRV-Cross#10
Fix
UniformTypeChangeTraverserandMoveNonSamplerUniformsIntoStructwere also visiting wholelayout(std140) uniformblocks (EbtBlock) anduniform Foo foo;struct instances (EbtStruct) and rewriting them tovec4, producing unnamed placeholder uniforms in the spirv-cross output and aIdentifier can't be empty.assert in bgfx. Both traversers are now restricted to scalar/vector basic types (Float/Int/Uint/Bool).CollectNonSamplerUniformsalso throws early if a uniform name is empty so future regressions surface in the compiler instead of deep in bgfx.