Drop the dead BGFX_CONFIG_MAX_FRAME_BUFFERS=256 overrides and stop clamping the pool - #1837
Merged
Merged
Conversation
…amping The pool floor added in BabylonJS#1715 and raised to 2048 in BabylonJS#1830 clamps any smaller value upward, so the three CI overrides have done nothing since June while reading as deliberate coverage of a 256-slot pool. Removing the flags is what makes the floor safe to relax, so both halves are here: build-win32.yml and nightly.yml run the validation sweep the larger pool exists for, so honoring their =256 would reintroduce the exhaustion BabylonJS#1830 fixed. With the flags gone, 2048 becomes a plain default rather than a floor, and an embedder that deliberately sets a smaller pool is no longer overridden without a diagnostic. build-win32-shader.yml builds only PrecompiledShaderTest, where the value never mattered. Verified by configuring Win32 x64 D3D11 both ways: with no flag bgfx still compiles with BGFX_CONFIG_MAX_FRAME_BUFFERS=2048, and -D ...=256 now reaches the compile line instead of being clamped up to 2048. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e327caab-46c0-4c04-a3c0-1577ad67f08c
Contributor
There was a problem hiding this comment.
Pull request overview
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.
Removes ineffective CI overrides for BGFX_CONFIG_MAX_FRAME_BUFFERS=256 and changes the CMake logic to only apply a default (2048) when the setting is unset (no longer clamping smaller user-provided values upward).
Changes:
- Drop
-D BGFX_CONFIG_MAX_FRAME_BUFFERS=256from the Win32/nightly CI workflows. - Update
Dependencies/CMakeLists.txtto setBGFX_CONFIG_MAX_FRAME_BUFFERSto 2048 only when not provided, removing the previous “floor/clamp” behavior. - Refresh the explanatory comment around why 2048 is the default.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| nightly.yml | Removes the BGFX_CONFIG_MAX_FRAME_BUFFERS=256 CMake definition from nightly generation. |
| Dependencies/CMakeLists.txt | Stops clamping low values; only supplies a default of 2048 when unset. |
| .github/workflows/build-win32.yml | Removes the ineffective CI override for max frame buffers. |
| .github/workflows/build-win32-shader.yml | Removes the ineffective CI override for max frame buffers in shader workflow. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bghgary
enabled auto-merge (squash)
August 14, 2026 21:28
bkaradzic-microsoft
approved these changes
Aug 14, 2026
ryantrem
reviewed
Aug 14, 2026
| # that point and exhausts the pool around the FrameGraph tests, so the floor | ||
| # has to cover a complete uninterrupted run. | ||
| if(NOT BGFX_CONFIG_MAX_FRAME_BUFFERS OR BGFX_CONFIG_MAX_FRAME_BUFFERS LESS 2048) | ||
| # The Canvas polyfill holds a bgfx framebuffer per JS Canvas object and per |
Member
There was a problem hiding this comment.
It sounds like we only need to increase this by default if you are using the canvas polyfill. Should it be conditioned on this?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[Created by Copilot on behalf of @bghgary]
Context
The pool floor added in #1715 (512) and raised in #1830 (2048) clamps any smaller value upward, so the three CI workflows passing
-D BGFX_CONFIG_MAX_FRAME_BUFFERS=256have had no effect since June while reading as deliberate coverage of a 256-slot pool.The clamp is also wrong on its own terms: an embedder that deliberately picks a smaller pool is overridden with no diagnostic.
Worth a look
=256take effect and reintroduce the pool exhaustion Playground validation: make a full 720-test sweep possible #1830 fixed, sincebuild-win32.ymlandnightly.ymlrun the validation sweep the larger pool exists for.build-win32-shader.ymlbuilds onlyPrecompiledShaderTest, where the value never mattered.BGFX_CONFIG_setting. It predates Playground validation: make a full 720-test sweep possible #1830 and still carries 512, so its merge from master needs to keep 2048.Verification
Configured Win32 x64 D3D11 both ways: with no flag bgfx still compiles with
BGFX_CONFIG_MAX_FRAME_BUFFERS=2048, so CI is unchanged, and-D BGFX_CONFIG_MAX_FRAME_BUFFERS=256now reaches the compile line instead of being clamped up.