From bbbd0d7c48837d75d64a5b6a19fdcc68844d9140 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Wed, 29 Jul 2026 14:03:55 -0700 Subject: [PATCH 1/3] NativeEngine: route instanced generic attributes landing on TexCoord3..TexCoord15 The divisor-driven instancing reroute only remapped consumer-instanced attributes whose compiled bgfx Attrib was below TexCoord3, silently dropping every generic instanced attribute that landed on TexCoord3 or above. Sprite instancing assigns cellInfo -> TexCoord3, so its per-instance data was packed into the instance data buffer by BuildInstanceDataBuffer but never read by the shader: the attribute stayed a per-vertex input reading unrelated vertex bytes, which collapsed and mis-UVed instanced sprites. Widen the guard to every real bgfx Attrib (< Attrib::Count). The built-in instanced attributes (world0-3, splatIndex0-3, instanceColor) use synthetic locations at/above INSTANCE_DATA_FIRST_LOCATION - 4, which is >= Attrib::Count, so they still compare false and remain correctly skipped. No shader cache invalidation is required: instanced program variants bypass the cache (ShaderProvider::Get uses the cache only when instancedAttributes is empty), and the non-instanced compilation path is unchanged. Measured with Babylon.js _features.supportSpriteInstancing forced to true (Win32 x64 D3D11 Debug), before -> after: idx 155 Sprites 4.242% FAIL -> 1.405% PASS idx 293 Prepass SSAO + sprites 1.617% pass -> 0.001% pass idx 369 Sprites Pixel Perfect 25.532% FAIL -> 7.612% FAIL (still over) The sprite/MSDF tests stay excluded here because un-excluding them additionally requires the Babylon.js-side supportSpriteInstancing change; this commit is the Native prerequisite. Full suite unchanged: ran=295 passed=295 failed=0 (--test-index=0-52,58-719). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae --- Plugins/NativeEngine/Source/NativeEngine.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/Plugins/NativeEngine/Source/NativeEngine.cpp b/Plugins/NativeEngine/Source/NativeEngine.cpp index d3774310b..eb0e4fb3e 100644 --- a/Plugins/NativeEngine/Source/NativeEngine.cpp +++ b/Plugins/NativeEngine/Source/NativeEngine.cpp @@ -2431,7 +2431,17 @@ namespace Babylon for (const auto& instance : instances) { const bgfx::Attrib::Enum attrib = instance.first; - if (attrib < bgfx::Attrib::TexCoord3) + // Reroute every consumer-instanced attribute that was compiled to a real + // per-vertex bgfx Attrib slot (Position..TexCoord15, i.e. < Attrib::Count). + // The built-in instanced attributes (world0-3, splatIndex0-3, instanceColor) + // are assigned synthetic locations at/above INSTANCE_DATA_FIRST_LOCATION - 4, + // which is >= Attrib::Count, so they still compare false here and are correctly + // skipped: they already arrive as instance data. + // The previous TexCoord3 boundary silently dropped generic instanced attributes + // landing on TexCoord3..TexCoord15 (e.g. sprite cellInfo -> TexCoord3), leaving + // them reading per-vertex garbage even though BuildInstanceDataBuffer had + // already packed them into the instance data buffer. + if (attrib < bgfx::Attrib::Count) { const size_t rank = count - 1 - ascendingIndex; const uint32_t targetLocation = Babylon::Graphics::INSTANCE_DATA_FIRST_LOCATION - static_cast(rank); From 670a1d4e825fa6993864120b60c6241475955607 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 30 Jul 2026 07:47:52 -0700 Subject: [PATCH 2/3] Update the stale TexCoord3 wording in the enclosing comment The block comment above the instancing reroute still described the old boundary ("recorded at a base bgfx location below TexCoord3"), which contradicted the widened guard right below it. Say "real per-vertex bgfx location" instead, and trim the sentence the inline comment duplicated from it so the two read as one explanation rather than two overlapping ones. Comment-only; no functional change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae --- Plugins/NativeEngine/Source/NativeEngine.cpp | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/Plugins/NativeEngine/Source/NativeEngine.cpp b/Plugins/NativeEngine/Source/NativeEngine.cpp index eb0e4fb3e..cdcf461a0 100644 --- a/Plugins/NativeEngine/Source/NativeEngine.cpp +++ b/Plugins/NativeEngine/Source/NativeEngine.cpp @@ -2413,7 +2413,7 @@ namespace Babylon } // Divisor-driven instancing: a consumer-instanced attribute (divisor==1) recorded at a - // base bgfx location below TexCoord3 was compiled to a per-vertex slot. bgfx can only feed + // real per-vertex bgfx location was compiled to a per-vertex slot. bgfx can only feed // per-instance data into i_data slots (the top TEXCOORD semantics), so route those attributes // to the correct i_data slot via a lazily-compiled program variant. The target location mirrors // BuildInstanceDataBuffer's reverse-attrib packing: highest base attrib -> i_data0 (TEXCOORD31), @@ -2431,12 +2431,11 @@ namespace Babylon for (const auto& instance : instances) { const bgfx::Attrib::Enum attrib = instance.first; - // Reroute every consumer-instanced attribute that was compiled to a real - // per-vertex bgfx Attrib slot (Position..TexCoord15, i.e. < Attrib::Count). - // The built-in instanced attributes (world0-3, splatIndex0-3, instanceColor) - // are assigned synthetic locations at/above INSTANCE_DATA_FIRST_LOCATION - 4, - // which is >= Attrib::Count, so they still compare false here and are correctly - // skipped: they already arrive as instance data. + // "Real per-vertex slot" means Position..TexCoord15, i.e. < Attrib::Count. The + // built-in instanced attributes (world0-3, splatIndex0-3, instanceColor) are + // assigned synthetic locations at/above INSTANCE_DATA_FIRST_LOCATION - 4, which + // is >= Attrib::Count, so they compare false here and are correctly skipped: + // they already arrive as instance data. // The previous TexCoord3 boundary silently dropped generic instanced attributes // landing on TexCoord3..TexCoord15 (e.g. sprite cellInfo -> TexCoord3), leaving // them reading per-vertex garbage even though BuildInstanceDataBuffer had From f92bd3b3050ed2e63f10d03878ef52b44da00854 Mon Sep 17 00:00:00 2001 From: Branimir Karadzic Date: Thu, 30 Jul 2026 11:06:57 -0700 Subject: [PATCH 3/3] Assert the lowest built-in instance-data location against bgfx::Attrib::Count NativeEngine::Draw's reroute guard treats location < bgfx::Attrib::Count as a real per-vertex attribute. The only assert backing that invariant covered INSTANCE_DATA_FIRST_LOCATION (41, i_data0) - the highest instance-data location. The value the guard actually rests on is the lowest built-in one, instanceColor on i_data4 at 37. Had bgfx::Attrib::Count grown into 38..41 the existing assert would still have passed while instanceColor began being rerouted as per-vertex data - the same silent-garbage failure this guard exists to prevent. Name the slot count so the magic 4 is no longer implicit and assert on it. Verified: Count == 26, first == 41, last built-in == 37 (slack 11); forcing Count to 39 fires the new assert and not the old one. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: af4ea82e-5fb0-4e56-b71a-f8ff3932d033 --- .../InternalInclude/Babylon/Graphics/BgfxShaderInfo.h | 10 ++++++++++ Plugins/ShaderCompiler/Source/ShaderCompilerCommon.cpp | 6 ++++++ .../ShaderCompiler/Source/ShaderCompilerTraversers.cpp | 3 ++- 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/Core/Graphics/InternalInclude/Babylon/Graphics/BgfxShaderInfo.h b/Core/Graphics/InternalInclude/Babylon/Graphics/BgfxShaderInfo.h index a72f6c60e..5e73c86d5 100644 --- a/Core/Graphics/InternalInclude/Babylon/Graphics/BgfxShaderInfo.h +++ b/Core/Graphics/InternalInclude/Babylon/Graphics/BgfxShaderInfo.h @@ -23,6 +23,16 @@ namespace Babylon::Graphics inline constexpr uint32_t TEXCOORD0_ATTRIBUTE_LOCATION{10}; inline constexpr uint32_t INSTANCE_DATA_FIRST_LOCATION{TEXCOORD0_ATTRIBUTE_LOCATION + INSTANCE_DATA_FIRST_TEXCOORD}; + /// The built-in per-instance attributes occupy the top BUILTIN_INSTANCE_DATA_SLOT_COUNT i_data + /// slots: world0-3 and splatIndex0-3 map to i_data0..i_data3, instanceColor to i_data4 (see + /// ShaderCompilerTraversers.cpp's attribute table). BUILTIN_INSTANCE_DATA_LAST_LOCATION is the + /// lowest synthetic location any of them can occupy; it is the boundary NativeEngine::Draw's + /// "< bgfx::Attrib::Count means a real per-vertex attribute that needs rerouting" guard rests + /// on, so it -- not just INSTANCE_DATA_FIRST_LOCATION -- must stay >= bgfx::Attrib::Count. + /// Keep in sync when adding a built-in per-instance attribute on a lower i_data slot. + inline constexpr uint32_t BUILTIN_INSTANCE_DATA_SLOT_COUNT{5}; + inline constexpr uint32_t BUILTIN_INSTANCE_DATA_LAST_LOCATION{INSTANCE_DATA_FIRST_LOCATION - (BUILTIN_INSTANCE_DATA_SLOT_COUNT - 1)}; + struct BgfxShaderInfo { std::vector VertexBytes{}; diff --git a/Plugins/ShaderCompiler/Source/ShaderCompilerCommon.cpp b/Plugins/ShaderCompiler/Source/ShaderCompilerCommon.cpp index 77d4b55b5..425dc359b 100644 --- a/Plugins/ShaderCompiler/Source/ShaderCompilerCommon.cpp +++ b/Plugins/ShaderCompiler/Source/ShaderCompilerCommon.cpp @@ -19,6 +19,12 @@ namespace Babylon::ShaderCompilerCommon // attribute table below). static_assert(Babylon::Graphics::TEXCOORD0_ATTRIBUTE_LOCATION == static_cast(bgfx::Attrib::TexCoord0)); static_assert(Babylon::Graphics::INSTANCE_DATA_FIRST_LOCATION >= static_cast(bgfx::Attrib::Count)); + // The assert above only covers i_data0, the *highest* instance-data location. NativeEngine::Draw + // reroutes any attribute whose location is < bgfx::Attrib::Count, so what that guard actually + // depends on is the *lowest* built-in one (instanceColor, on i_data4). Were bgfx::Attrib::Count + // to grow past it, the assert above would still pass while instanceColor started being rerouted + // as if it were per-vertex data -- the same silent-garbage failure the guard exists to prevent. + static_assert(Babylon::Graphics::BUILTIN_INSTANCE_DATA_LAST_LOCATION >= static_cast(bgfx::Attrib::Count)); // Patching shader code to append clip space coordinates for the current rendering API. // Can be done with glslang shader traversal. Done with string patching for now. diff --git a/Plugins/ShaderCompiler/Source/ShaderCompilerTraversers.cpp b/Plugins/ShaderCompiler/Source/ShaderCompilerTraversers.cpp index 6bd0b05f5..3289d86ce 100644 --- a/Plugins/ShaderCompiler/Source/ShaderCompilerTraversers.cpp +++ b/Plugins/ShaderCompiler/Source/ShaderCompilerTraversers.cpp @@ -873,7 +873,8 @@ namespace Babylon::ShaderCompilerTraversers // combined with BuildInstanceDataBuffer's descending-key packing, world3 lands on i_data0 // (TEXCOORD31) and world0 on i_data3. instanceColor follows at i_data4. The i_data name is // cosmetic on D3D (binding is by TEXCOORD semantic, resolved from the location via the - // HLSLVertexAttributeRemap table). + // HLSLVertexAttributeRemap table). Adding one on a lower slot means bumping + // BUILTIN_INSTANCE_DATA_SLOT_COUNT in BgfxShaderInfo.h. IF_NAME_RETURN_ATTRIB("instanceColor", Babylon::Graphics::INSTANCE_DATA_FIRST_LOCATION - 4, "i_data4") IF_NAME_RETURN_ATTRIB("world0", Babylon::Graphics::INSTANCE_DATA_FIRST_LOCATION - 3, "i_data3") IF_NAME_RETURN_ATTRIB("world1", Babylon::Graphics::INSTANCE_DATA_FIRST_LOCATION - 2, "i_data2")