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/NativeEngine/Source/NativeEngine.cpp b/Plugins/NativeEngine/Source/NativeEngine.cpp index d3774310b..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,7 +2431,16 @@ namespace Babylon for (const auto& instance : instances) { const bgfx::Attrib::Enum attrib = instance.first; - if (attrib < bgfx::Attrib::TexCoord3) + // "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 + // 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); 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")