Fix Vulkan >16 sampler test false-pass and add Linux Vulkan CI by bkaradzic-microsoft · Pull Request #1859 · BabylonJS/BabylonNative

bkaradzic-microsoft · 2026-09-01T22:06:03Z

Summary

Follow-up to #1857 addressing @bghgary:

False pass on unexpected success — ShaderCompilation.VulkanRejectsMoreThanSixteenSamplers no longer treats unexpected success as the expected rejection.
Vulkan CI (build-only) — Ubuntu_Clang_Vulkan configures GRAPHICS_API=Vulkan and builds UnitTests/Playground so the Vulkan TU path is compiled on every PR. Runtime under lavapipe is not run: GH runners have no GPU, and bgfx Xlib surface init SEGV'd when we tried the gtest.
Minimal Vulkan stubs so the tree links: RendererType/Vulkan, DeviceImpl_Vulkan, ExternalTexture_Vulkan (throws / not implemented), Helpers.Vulkan. ExternalTexture tests skipped via SKIP_EXTERNAL_TEXTURE_TESTS.
Compile fixes surfaced by the new job: SPIRV-Cross GLSL for Vulkan, glslang IR using namespace, ExternalTexture Shared.h stub pattern.

Test plan

Ubuntu_Clang_Vulkan green (configure + ninja only)
Existing Linux OpenGL / Win32 jobs unaffected
False-pass fix still in Tests.ShaderCompilation.cpp

bkaradzic-microsoft · 2026-09-01T22:06:18Z

Addresses @bghgary’s comment on #1857:

False pass: VulkanRejectsMoreThanSixteenSamplers uses explicit status values (unexpected_success / threw / not_ready); unexpected success fails outside the catch path.
CI: Ubuntu_Clang_Vulkan with GRAPHICS_API=Vulkan + lavapipe runs that test.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Updates Vulkan coverage by fixing a gtest that could falsely pass on unexpected shader compile success, and by introducing a Linux Vulkan CI lane (lavapipe) plus minimal Vulkan stubs to allow configure/link.

Changes:

Fix ShaderCompilation.VulkanRejectsMoreThanSixteenSamplers to distinguish expected rejection from unexpected success.
Add Ubuntu_Clang_Vulkan CI job and extend Linux workflow to support GRAPHICS_API=Vulkan with Mesa lavapipe.
Add minimal Vulkan backend stubs (renderer types, device impl, and unimplemented helpers/external texture) and skip incompatible tests.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
Plugins/ExternalTexture/Source/ExternalTexture_Vulkan.cpp	Adds Vulkan stub implementation that throws (not supported yet) to allow linking.
Core/Graphics/Source/DeviceImpl_Vulkan.cpp	Adds Vulkan `DeviceImpl` renderer type + platform info plumbing.
Core/Graphics/Include/RendererType/Vulkan/Babylon/Graphics/RendererType.h	Introduces Vulkan renderer-type typedefs for build plumbing.
Apps/UnitTests/Source/Tests.ShaderCompilation.cpp	Fixes false-pass logic in Vulkan “>16 samplers” shader compilation test.
Apps/UnitTests/Source/Helpers.Vulkan.cpp	Adds Vulkan stubs for unit test helpers (throws for unimplemented paths).
Apps/UnitTests/CMakeLists.txt	Defines Vulkan build flags and skips ExternalTexture tests for Vulkan.
.github/workflows/ci.yml	Adds a Linux Vulkan CI job entry.
.github/workflows/build-linux.yml	Adds `graphics-api` input and Vulkan/lavapipe setup + conditional test execution.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

- Unexpected success is no longer thrown inside the try/catch that treats any "16" message as the expected rejection. - Add Ubuntu_Clang_Vulkan job (GRAPHICS_API=Vulkan + lavapipe) that builds UnitTests and runs ShaderCompilation.VulkanRejectsMoreThanSixteenSamplers. - Minimal Vulkan Graphics/ExternalTexture/UnitTest helpers so GRAPHICS_API=Vulkan can configure and link (ExternalTexture skipped). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601

…ture. The workflow YAML from the cherry-pick had broken nesting under inputs/steps (parse failure). Rewrite with correct alignment matching the prior layout. Also throw from Helpers::DestroyTexture on non-null Vulkan handles. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601

Nested under Ubuntu_Clang_QuickJS, which made the workflow file invalid and aborted the whole CI run before any jobs started. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601

ShaderCompilerVulkan.cpp includes spirv_glsl.hpp and ShaderCompilerCommon.h includes spirv_cross.hpp; both need spirv-cross-glsl. Disabling GLSL for every non-OpenGL API left the Linux Vulkan CI job without those headers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601

TIntermNode/TIntermSymbol are not nested in namespace glslang; qualify them like ShaderCompilerTraversers so the Linux Vulkan job compiles. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601

Bare TIntermSymbol/TIntermNode need namespace glslang in scope (same pattern as ShaderCompilerTraversers). Include localintermediate.h and using namespace glslang so the Ubuntu Vulkan job can compile CollectStageUniforms. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601

Declare Impl/Update like other backends; Shared.h owns the out-of-line bodies. Keep GetInfo throwing so Vulkan remains unsupported without duplicate definitions that break Ubuntu_Clang_Vulkan. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601

GH runners have no GPU; lavapipe+bgfx Xlib surface init SEGV'd on the >16-sampler gtest. Keep GRAPHICS_API=Vulkan configure+link coverage so Vulkan packaging breaks cannot land unbuilt again; drop runtime steps. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601

Copilot AI lite review requested due to automatic review settings September 1, 2026 22:06

bkaradzic-microsoft mentioned this pull request Sep 1, 2026

Bump bgfx.cmake (TextureRegion blit/read, shader bin v12, Vulkan bindings) #1857

Merged

4 tasks

Copilot AI reviewed Sep 1, 2026

View reviewed changes

bkaradzic-microsoft requested a balanced review from Copilot September 1, 2026 22:45

Copilot AI reviewed Sep 1, 2026

View reviewed changes

Comment thread Plugins/ExternalTexture/Source/ExternalTexture_Vulkan.cpp

Comment thread Apps/UnitTests/Source/Helpers.Vulkan.cpp Outdated

Copilot started reviewing on behalf of bkaradzic-microsoft September 1, 2026 23:15 View session

Copilot started reviewing on behalf of bkaradzic-microsoft September 2, 2026 00:00 View session

bkaradzic-microsoft and others added 5 commits September 2, 2026 07:39

Address Copilot nits: optional include and DestroyTexture indentation.

2ae0f2d

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601

bkaradzic-microsoft force-pushed the fix/vulkan-16-sampler-test-ci branch from 6346fe8 to bdeb5c1 Compare September 2, 2026 14:39

bkaradzic-microsoft and others added 4 commits September 2, 2026 08:56

bkaradzic-microsoft requested a review from bghgary September 2, 2026 20:39

Navigation Menu

Uh oh!

Fix Vulkan >16 sampler test false-pass and add Linux Vulkan CI - #1859

Fix Vulkan >16 sampler test false-pass and add Linux Vulkan CI#1859
bkaradzic-microsoft wants to merge 9 commits into
BabylonJS:masterfrom
bkaradzic-microsoft:fix/vulkan-16-sampler-test-ci

bkaradzic-microsoft commented Sep 1, 2026 •

edited

Loading

Uh oh!

bkaradzic-microsoft commented Sep 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

bkaradzic-microsoft commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

bkaradzic-microsoft commented Sep 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bkaradzic-microsoft commented Sep 1, 2026 •

edited

Loading