Resolve #1786 review follow-ups: drop BN workarounds, bump bgfx.cmake, guard useCache - #1788
Open
SergioRZMasson wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refines ShaderProvider::Get to avoid declaring useCache when the ShaderCache plugin is disabled, aligning the variable’s lifetime and evaluation with the existing #ifdef SHADER_CACHE usage blocks.
Changes:
- Replaced
[[maybe_unused]]onuseCachewith an#ifdef SHADER_CACHE-guarded declaration. - Ensures
instancedAttributes.empty()is only evaluated when ShaderCache support is compiled in.
useCache is only referenced inside the #ifdef SHADER_CACHE blocks, so move its declaration into the first such block instead of marking it [[maybe_unused]]. Because #ifdef does not introduce a C++ scope, the variable remains visible to the later SHADER_CACHE block, and when the ShaderCache plugin is disabled it is compiled out entirely (and instancedAttributes.empty() is not computed) rather than merely having an unused-variable diagnostic suppressed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 45aaf3cc-900d-442b-a7e5-0fb6d3fe6658
SergioRZMasson
force-pushed
the
cleanup-shaderprovider-usecache
branch
from
July 23, 2026 13:06
45481f9 to
ff35c76
Compare
Advance the bgfx.cmake pin to 8f7e4455, which carries the x86_64 SSE4.2 minspec on the bx target and bumps bx to include the platform.h fixes (MSVC ARM64 detected as 64-bit; 32-bit Windows _WIN32_WINNT default raised to 0x0601). With those handled upstream, remove the two workarounds this repo carried: * Dependencies/CMakeLists.txt: the manual -msse4.2 flag on bx/bgfx/bimg — the bx target now propagates the minspec itself (including the x86_64 slice of Apple universal builds). * Plugins/NativeEngine/CMakeLists.txt: the _WIN32_WINNT / WINVER = 0x0A00 pin — bx/platform.h now defaults every Windows arch to a >= Vista baseline, so arcana's ETW trace_region.h compiles without it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 45aaf3cc-900d-442b-a7e5-0fb6d3fe6658
SergioRZMasson
changed the title
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.
Summary
Addresses the three review follow-ups from #1786 (bghgary's notes), moving each fix to its proper home so BN no longer carries the workarounds.
1. Drop the manual
-msse4.2flag (Dependencies/CMakeLists.txt)The SSE4.2 minspec that bx's x86_64 SIMD path requires now lives in bgfx.cmake (BabylonJS/bgfx.cmake#130), applied
PUBLICon thebxtarget — including the-Xarch_x86_64scoping needed for Apple universal (arm64;x86_64) builds. BN no longer needs to re-add it.2. Drop the per-plugin
_WIN32_WINNT/WINVERpin (Plugins/NativeEngine/CMakeLists.txt)The root cause was fixed in bx (bkaradzic/bx#409):
platform.hnow detects MSVC ARM64 as 64-bit and defaults every 32-bit Windows target to0x0601instead of the pre-Vista0x0502. With that, arcana's ETWtrace_region.hcompiles on Windows x86/ARM64 without pinning an API baseline on the NativeEngine target.3. Guard
useCachewith#ifdef SHADER_CACHE(Plugins/NativeEngine/Source/ShaderProvider.cpp)useCacheis referenced only inside the#ifdef SHADER_CACHEblocks, so its declaration moves into that guard (rather than[[maybe_unused]]): the variable — andinstancedAttributes.empty()— is compiled out entirely when the ShaderCache plugin is disabled.To consume 1 and 2, this bumps the
bgfx.cmakepin to8f7e4455(which carries the SSE minspec on the bx target and advances bx tocd6720cewith the platform.h fixes).Testing
_WIN32_WINNTworkaround removed —NativeEngine.cpp/Program.cpp/ShaderProvider.cppcompile andNativeEngine.liblinks (previously this arch failed witherror C3861inTraceLoggingProvider.h)._mm_round_psSSE4.1 intrinsic on Clang/x86_64,-Xarch_x86_64-scoped on Apple).ShaderProvidercompiles clean under Clang-Werror -Wunused-variablewith ShaderCache enabled and disabled.