Let embedders override Babylon Native's bgfx configuration - #1807
Open
bghgary wants to merge 4 commits into
Open
Let embedders override Babylon Native's bgfx configuration#1807bghgary wants to merge 4 commits into
bghgary wants to merge 4 commits into
Conversation
Babylon Native tunes a number of bgfx compile-time settings in Dependencies/CMakeLists.txt. They were applied unconditionally, so an embedder could not change any of them. The definitions are emitted after bgfx.cmake's, which puts them last on the compile line, so they take effect regardless of what the embedder asked for. For a setting bgfx.cmake does not forward, such as BGFX_CONFIG_MAX_VERTEX_STREAMS, the embedder's value never reached the compile line at all and was silently ignored. For one it does forward, both values reached it and Babylon Native's won, leaving a conflicting duplicate behind. Embedders have worked around this by reading COMPILE_DEFINITIONS and INTERFACE_COMPILE_DEFINITIONS back off the bgfx target and filtering entries out. Route these through babylon_native_bgfx_config(), which uses the embedder's value when one is set and Babylon Native's otherwise. Being emitted last still decides the outcome; the value being emitted is now the embedder's whenever they expressed one. The value is used as written rather than being required to be numeric, since bgfx spells most of its own defaults as expressions such as (4<<10). CMake booleans are the exception and fall back to the default, because the C preprocessor cannot use them and bgfx.cmake declares several of these names as options that can already hold ON or OFF. Also gate the BGFX_CONFIG_MAX_FRAME_BUFFERS floor of 512 on BABYLON_NATIVE_POLYFILL_CANVAS. The floor exists because the Canvas polyfill allocates a framebuffer per canvas and per text-rendering operation; an embedder that disables the polyfill draws into a handful of them and should be able to select a smaller pool. No behaviour change by default. Verified by configuring three ways and reading the generated definitions. Default: MAX_VERTEX_STREAMS=18, MAX_FRAME_BUFFERS=512, MIN_UNIFORM_BUFFER_SIZE=4096, unchanged, and no setting emitted more than once. Canvas off with overrides: 12, 4 and 2048 respectively, plus MAX_TEXTURES=(4<<10) passed through verbatim, so the filtering workaround is no longer needed. Canvas on with an explicit smaller frame-buffer pool: still clamped up to the 512 the Canvas sweeps need. UnitTests pass (17/17). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 714b4495-258e-4645-abf7-26c17bc29d5b
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts Babylon Native’s bgfx integration so embedders can override Babylon Native’s bgfx compile-time configuration via BGFX_CONFIG_* variables, while keeping Babylon Native’s current values as defaults. It also makes the BGFX_CONFIG_MAX_FRAME_BUFFERS minimum of 512 apply only when the Canvas polyfill is enabled, allowing smaller framebuffer pools when Canvas is disabled.
Changes:
- Introduces
babylon_native_bgfx_config()to apply a defaultBGFX_CONFIG_*value unless the embedder provides one. - Routes Babylon Native’s bgfx compile definitions through the new helper to avoid duplicate/losing definitions.
- Gates the MAX_FRAME_BUFFERS 512-floor behind
BABYLON_NATIVE_POLYFILL_CANVASand emitsBGFX_CONFIG_MAX_FRAME_BUFFERSonly when explicitly set/clamped.
Comments suppressed due to low confidence (1)
Dependencies/CMakeLists.txt:95
- If an embedder accidentally sets
BGFX_CONFIG_MAX_FRAME_BUFFERSto a CMake boolean (e.g.-DBGFX_CONFIG_MAX_FRAME_BUFFERS=ON), this block will currently emitBGFX_CONFIG_MAX_FRAME_BUFFERS=ON, which is not a valid preprocessor value. Since there is no Babylon Native default for MAX_FRAME_BUFFERS in non-Canvas builds, it’s better to treat boolean-like values as "unset" and skip emitting the definition.
if(BGFX_CONFIG_MAX_FRAME_BUFFERS)
babylon_native_bgfx_config(MAX_FRAME_BUFFERS ${BGFX_CONFIG_MAX_FRAME_BUFFERS})
endif()
…dded The settings were applied with target_compile_definitions after bgfx.cmake had already been added. That put them last on the compile line, so they took effect regardless of what an embedder asked for: a setting bgfx.cmake forwards ended up defined twice with Babylon Native's value winning, and one it does not forward never reached the compile line at all. Embedders work around this by reading COMPILE_DEFINITIONS and INTERFACE_COMPILE_DEFINITIONS back off the bgfx target and filtering entries out. Set them as ordinary variables before bgfx.cmake is added instead, each only when nothing has supplied a value, and let bgfx.cmake forward whichever value survives. A setting is now defined once, and an embedder's value is used as written -- including expressions such as (4<<10), which is how bgfx spells most of its own defaults. This depends on BabylonJS/bgfx.cmake#137. Before it, a set() in this scope is erased by bgfx.cmake's own set(<name> "" CACHE STRING ...) and never seen. BGFX_CONFIG_DEBUG_ANNOTATION stays a compile definition. bgfx.cmake forwards it through a generator expression that turns it on in Debug regardless of the value, so overriding it after the fact is the only thing that works. BGFX_CONFIG_MAX_FRAME_BUFFERS becomes a plain default of 512 rather than a floor. The floor could not do its job anyway: LESS returns false rather than erroring on a non-numeric value, so an expression passed straight through unclamped -- '(1<<3)' gave 8 framebuffers where 512 was required. As a default it is honored when nothing else is set and stays out of the way when something is. The CI workflows passed 256 and relied on being clamped up, so they no longer pass it at all. Verified by configuring and reading the definitions off the bgfx target. Default build: every Babylon Native value present exactly once, MAX_FRAME_BUFFERS=512. With overrides and Canvas off: MAX_VERTEX_STREAMS=12, MIN_UNIFORM_BUFFER_SIZE=2048, DRAW_CALL_BLOCK=128 and MAX_TEXTURES=(4<<10) all honored, no MAX_FRAME_BUFFERS, and the remaining Babylon Native defaults untouched. UnitTests pass (17/17). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 714b4495-258e-4645-abf7-26c17bc29d5b
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 714b4495-258e-4645-abf7-26c17bc29d5b
…able # Conflicts: # Dependencies/CMakeLists.txt
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
Babylon Native's bgfx settings are applied unconditionally and land last on the compile line, so an embedder cannot change any of them. Embedders work around this today by reading
COMPILE_DEFINITIONSback off thebgfxtarget and filtering the losing entries out.Worth a look
(4<<10). CMake booleans fall back to the default, because the preprocessor cannot use them and several of these names are declared as options that may holdON/OFF.MAX_FRAME_BUFFERSfloor of 512 is now gated onBABYLON_NATIVE_POLYFILL_CANVAS: the polyfill allocates a framebuffer per canvas and per text-rendering operation, so an embedder without it should not pay for the pool. CI no longer passes-D BGFX_CONFIG_MAX_FRAME_BUFFERS=256.f7333d92is required, not incidental — that commit forwards the remaining settings, and without it several would go back to being silently ignored.No behaviour change by default.
Verification
Embedder overrides reach the compiler (three of the four tested previously vanished), Babylon Native's defaults still apply where the embedder said nothing, and each setting is emitted exactly once per configuration. UnitTests 17/17.
Related
BabylonJS/bgfx.cmake#137 and #138 fix the bgfx.cmake half; both are merged and included in the pin.