Bump bgfx.cmake (TextureRegion blit/read, shader bin v12, Vulkan bindings) - #1857
Conversation
- Pin bgfx.cmake GIT_TAG to 1a135dbf (bgfx 3b85a1c): TextureRegion blit/read, shader bin v12 - Rewrite FrameBuffer/DeviceImpl/NativeCapture/NativeEngine blit and readTexture call sites for TextureRegion / bgfx::read - ShaderCompilerCommon: package shaders as BGFX_SHADER_BIN_VERSION 12 (raw SRV/UAV masks + texComponent/texDimension/texFormat on uniforms) - Canvas precompiled shaders: bump embedded headers from bin v11 to v12 (insert raw binding masks) Local Win32 Debug validation: Nested BBG + the three previously failing motion-blur instance tests all pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601
There was a problem hiding this comment.
🔵 Needs a closer look
It changes shader binary packaging and core GPU blit/readback paths across multiple backends, which is high-impact and needs full CI/platform validation.
Pull request overview
Updates Babylon Native’s bgfx integration to a newer bgfx.cmake/bgfx revision and adapts engine/shader tooling to match breaking API and shader-binary format changes introduced in that range.
Changes:
- Bump
bgfx.cmaketo1a135dbf...(bgfx3b85a1c) viaCMakeLists.txt. - Update runtime code to use
TextureRegion-based blit/read APIs (encoder->blit(...)andbgfx::read(TextureRegion, ...)). - Update shader binary emission to bgfx shader package v12 (including v12-required fields) and bump embedded Canvas precompiled shader headers to v12.
File summaries
| File | Description |
|---|---|
| Polyfills/Canvas/Source/Shaders/spirv/vs_nanovg_fill.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/spirv/vs_fspass.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/spirv/fs_nanovg_fill.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/spirv/fs_gaussblur.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/spirv/fs_boxblur.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/metal/vs_nanovg_fill.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/metal/vs_fspass.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/metal/fs_nanovg_fill.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/metal/fs_gaussblur.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/metal/fs_boxblur.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/glsl/vs_nanovg_fill.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/glsl/vs_fspass.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/glsl/fs_nanovg_fill.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/glsl/fs_gaussblur.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/glsl/fs_boxblur.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/essl/vs_nanovg_fill.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/essl/vs_fspass.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/essl/fs_nanovg_fill.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/essl/fs_gaussblur.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/essl/fs_boxblur.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/dxil/vs_nanovg_fill.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/dxil/vs_fspass.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/dxil/fs_nanovg_fill.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/dxil/fs_gaussblur.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/dxil/fs_boxblur.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/dxbc/vs_nanovg_fill.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/dxbc/vs_fspass.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/dxbc/fs_nanovg_fill.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/dxbc/fs_gaussblur.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Polyfills/Canvas/Source/Shaders/dxbc/fs_boxblur.h | Bump embedded shader binary header to v12 format (adds v12 fields). |
| Plugins/ShaderCompiler/Source/ShaderCompilerCommon.cpp | Emit bgfx shader binary v12 (raw SRV/UAV masks + per-uniform texture metadata). |
| Plugins/NativeEngine/Source/NativeEngine.cpp | Update blit + readback paths to TextureRegion APIs (copy + readTexture pipeline). |
| Plugins/NativeCapture/Source/NativeCapture.cpp | Update framebuffer readback blit to TextureRegion API. |
| Core/Graphics/Source/FrameBuffer.cpp | Update FrameBuffer::Blit to TextureRegion API and preserve “whole texture” semantics. |
| Core/Graphics/Source/DeviceImpl.cpp | Switch async readback to bgfx::read(TextureRegion, ...). |
| CMakeLists.txt | Bump bgfx.cmake dependency tag to the targeted commit. |
Review details
Suppressed comments (2)
Plugins/NativeEngine/Source/NativeEngine.cpp:2375
- This ReadTexture blit fallback block is over-indented compared to the rest of the else-branch, which makes it look like it is nested differently than it actually is.
// If the image needs to be cropped, the texture lacks the READ_BACK flag, or we are reading a
// specific cube-map face, blit to a temp 2D texture. bgfx::read addresses a whole mip of one
// slice via TextureRegion::z, but a cropped sub-rect still needs the blit path. Cube-face
// reads use srcZ = face index on the source region of that blit.
if (isCubeFace || x != 0 || y != 0 || width != mipWidth || height != mipHeight || (texture->Flags() & BGFX_TEXTURE_READ_BACK) == 0)
Plugins/ShaderCompiler/Source/ShaderCompilerCommon.cpp:312
- This BGFX_SHADER_BIN_VERSION/comment block is over-indented compared to the rest of CreateBgfxShader, which makes the shader-binary header layout harder to follow.
// Must match BGFX_SHADER_BIN_VERSION in bgfx tools/shaderc/shaderc.cpp.
// v12 requires raw SRV/UAV binding masks after the in/out hashes, and
// uniform entries always carry texComponent/texDimension/texFormat (v8/v10).
constexpr uint8_t BGFX_SHADER_BIN_VERSION{12};
- Files reviewed: 36/36 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Plain D32 is not a valid D3D11 depth render-target (bgfx maps it to R24G8 with no DSV). Newer bgfx asserts in createTexture2D when isTextureValid fails, which broke CI on "NME Shadow Map". Prefer D32F for depth-only framebuffers when supported, otherwise fall back to D24/D24S8; Android still always uses D24S8. Also fix over-indented blit/read/shader packaging blocks flagged by Copilot review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601
UWP CI failed compiling bgfx dxgi.cpp: SetMaximumFrameLatency is not on IDXGISwapChain1 (WinRT SwapChainI). Pulls BabylonJS/bgfx.cmake#143 / BabylonJS/bgfx#77 which QI for IDXGISwapChain2 first. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601
|
UWP-only remaining failure was a bgfx compile error from present-pacing:
WinRT Fixed upstream:
BN pin bumped in 02c0586. |
There was a problem hiding this comment.
🟡 Changes recommended
The v12 shader package selects bgfx’s new Vulkan binding model without emitting its required shifted bindings.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 36/36 changed files
- Comments generated: 1
- Review effort level: Balanced
v12 selects bgfx's new Vulkan binding model (m_oldBindingModel=false), which computes sampler stage as regIndex - kSpirvBindShift (2). BN still wrote regIndex=0 and left SPIR-V image/sampler bindings unshifted, so a sampler could index m_bindInfo[65534]. - Rewrite glslang AST bindings to match shaderc (UBO 0/1, images at 2+i, samplers at image+16) - Package separate_images with regIndex = image binding - Strip the SplitSamplers "Texture" suffix and map UniformStages to the unshifted bgfx sampler slot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601
Pulls BabylonJS/bgfx.cmake#144 / bgfx#78. bgfx defines DXGI IIDs locally instead of linking dxguid.lib; the UWP frame-latency QI path referenced IID_IDXGISwapChain2 without a local GUID, breaking all MSVC app links (LNK2001). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601
- Preserve SamplerSplitter shared layoutBinding when applying bgfx Vulkan shifts (do not compact per-stage) - Map BackBufferDepthStencilFormat::Depth32 to D32F (D32 has no DSV on D3D11/12 in current bgfx) - Bump ShaderCache CACHE_VERSION 4 -> 5 for shader bin v12 packaging Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601
- Reject Vulkan programs with >16 distinct sampler textures (bgfx stage limit) - Move Vulkan sampler packaging out of ShaderCompilerCommon into ShaderCompilerVulkan (CreateBgfxShader takes SamplerResourceSet + appender) - Align Depth32/D32F case and ShaderCache CACHE_VERSION comments - Add Disjoint/PartiallyShared stage-sampler compile tests; Vulkan-only >16 rejection test when BABYLON_NATIVE_GRAPHICS_API_VULKAN 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
Resolve conflicts keeping shotgun fork pins and particle/depth/readback work: - CMakeLists: bkaradzic-microsoft/* @ shotgun (bgfx.cmake already carries bump) - Texture: SetMetadata + Attach(ownsHandle) for sampleable depth ownership - ShaderCompiler: dual CreateBgfxShader (v12) + CreateBgfxComputeShader @ v12 with raw SRV/UAV masks - NativeEngine readTexture: TextureRegion blit API + D3D y-flip/crop - Framebuffer depth: D32F/D24/D24S8 selection inside shotgun requestDepthStencil path
|
[Commented by Copilot on behalf of @bghgary] The Vulkan sampler-limit regression test merged with two gaps: no CI job configures |
|
Addressed:
Pushed in 79b363c. |
|
#1857 was already merged when this landed on the fork branch. Follow-up is in a new PR: That PR has:
|
Master BabylonJS#1857 changed encoder.blit to TextureRegion pairs; Context.cpp still used the old 14-arg form and broke CI after merging master. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601
Master BabylonJS#1857 changed encoder.blit to TextureRegion pairs; Context.cpp still used the old 14-arg form and broke CI after merging master. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: d47cbab2-d751-4cf9-984f-4412dd9ec601
Summary
Bumps
bgfx.cmaketo9a569c775917e3dd2a8cfb8836aeddbe790a8deb(bgfx0837eadvia BabylonJS/bgfx#78 / bgfx.cmake#144) and adapts Babylon Native to the Graphics/shader packaging changes in that range.API adaptations
encoder.blit/bgfx::blittakeTextureRegionpairs;bgfx::readTexture→bgfx::read(TextureRegion, data). Updated inFrameBuffer,DeviceImpl,NativeCapture,NativeEngine.CreateBgfxShaderwritesBGFX_CHUNK_MAGIC_*S_VERSION12, emits per-uniform texture meta (AppendUniformTextureMeta), and carries D3D SRV/UAV masks.Depth32maps toD32F(D3D has no DSV for plain D32).CACHE_VERSION4 → 5 (v12 binaries are not compatible with prior cache entries).Vulkan packaging (shaderc / bgfx v12 binding model)
layoutBindingper sampler name (no per-stage compact remap that desynced UniformStages).image + 16; UBOs stay 0/1.AppendSamplersVulkan:separate_images, stripTexturesuffix, setregIndexfrom binding.ShaderCompilerVulkan.cpp/ commonCreateBgfxShadersampler-set path (not#if VULKANin Common).Tests / CI
ShaderCompilation.VulkanRejectsMoreThanSixteenSamplers(compiled only whenGRAPHICS_API=Vulkan): unexpected success fails outside the catch path (no false pass via a thrown"…>16…"message).Ubuntu_Clang_VulkanCI job:GRAPHICS_API=Vulkan, lavapipe ICD, runs that gtest.RendererType/DeviceImpl_Vulkan/ExternalTexture_Vulkanstub /Helpers.Vulkanso the API can configure and link. ExternalTexture remains unimplemented (SKIP_EXTERNAL_TEXTURE_TESTS).Upstream deps already merged
IDXGISwapChain2beforeSetMaximumFrameLatency)IID_IDXGISwapChain2GUID; UWP does not linkdxguid)Test plan
Ubuntu_Clang_Vulkangreen (build +VulkanRejectsMoreThanSixteenSamplersunder lavapipe)