diff --git a/Core/Graphics/InternalInclude/Babylon/Graphics/BgfxShaderInfo.h b/Core/Graphics/InternalInclude/Babylon/Graphics/BgfxShaderInfo.h index 5e73c86d5..509459080 100644 --- a/Core/Graphics/InternalInclude/Babylon/Graphics/BgfxShaderInfo.h +++ b/Core/Graphics/InternalInclude/Babylon/Graphics/BgfxShaderInfo.h @@ -1,8 +1,10 @@ #pragma once +#include #include #include #include +#include #include namespace Babylon::Graphics @@ -23,16 +25,54 @@ 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}; + /// Mirrors bgfx's BGFX_CONFIG_MAX_INSTANCE_DATA_COUNT (bgfx/src/config.h, a private header): + /// the number of 16-byte per-instance slots (i_data0..i_data15) bgfx can bind in one draw. + inline constexpr uint32_t MAX_INSTANCE_DATA_SLOT_COUNT{16}; + /// 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}; + /// slots. Which slot each one gets is decided per shader, from the set the shader actually + /// declares (see ShaderCompilerTraversers.cpp), because bgfx requires the used i_data slots to + /// be a contiguous run starting at i_data0. The count is the size of the largest possible set: + /// world0-3 (or splatIndex0-3), instanceColor, and previousWorld0-3 for motion vectors. + /// 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. + inline constexpr uint32_t BUILTIN_INSTANCE_DATA_SLOT_COUNT{9}; inline constexpr uint32_t BUILTIN_INSTANCE_DATA_LAST_LOCATION{INSTANCE_DATA_FIRST_LOCATION - (BUILTIN_INSTANCE_DATA_SLOT_COUNT - 1)}; + /// The names Babylon.js uses for those built-in per-instance attributes. The shader compiler + /// recognizes them by name (ShaderCompilerTraversers.cpp) and NativeEngine counts how many of + /// them a program declares to size the instance data buffer, so both must read the same table. + inline constexpr std::array BUILTIN_INSTANCE_ATTRIBUTE_NAMES{ + "world0", + "world1", + "world2", + "world3", + "previousWorld0", + "previousWorld1", + "previousWorld2", + "previousWorld3", + "instanceColor", + "splatIndex0", + "splatIndex1", + "splatIndex2", + "splatIndex3", + }; + + /// True when `name` is one of BUILTIN_INSTANCE_ATTRIBUTE_NAMES. + inline constexpr bool IsBuiltInInstanceAttributeName(std::string_view name) + { + for (const std::string_view builtIn : BUILTIN_INSTANCE_ATTRIBUTE_NAMES) + { + if (builtIn == name) + { + return true; + } + } + return false; + } + struct BgfxShaderInfo { std::vector VertexBytes{}; diff --git a/Plugins/NativeEngine/Source/NativeEngine.cpp b/Plugins/NativeEngine/Source/NativeEngine.cpp index d572b9b55..b71c6d1df 100644 --- a/Plugins/NativeEngine/Source/NativeEngine.cpp +++ b/Plugins/NativeEngine/Source/NativeEngine.cpp @@ -2553,6 +2553,29 @@ namespace Babylon m_boundFrameBufferNeedsRebinding.Set(false); } + // The number of contiguous i_data slots the current program's vertex shader reads for its + // built-in per-instance attributes. ShaderCompilerTraversers assigns those attributes a dense + // run of slots starting at i_data0, one per declared attribute, so the count of built-in names + // in the program's attribute table is that run's length. The instance data buffer must cover it + // even when the draw supplied fewer attributes, or D3D11 rejects the input layout. + uint32_t NativeEngine::GetBuiltInInstanceDataSlotCount() const + { + if (m_currentProgram == nullptr) + { + return 0; + } + + uint32_t count{}; + for (const auto& [name, location] : m_currentProgram->VertexAttributeLocations()) + { + if (Babylon::Graphics::IsBuiltInInstanceAttributeName(name)) + { + ++count; + } + } + return count; + } + // Note: For legacy reasons JS might call this function for instance drawing. // In that case the instanceCount will be calculated inside the SetVertexBuffers method. void NativeEngine::DrawIndexed(NativeDataStream::Reader& data) @@ -2565,7 +2588,7 @@ namespace Babylon if (m_boundVertexArray != nullptr) { m_boundVertexArray->SetIndexBuffer(encoder, indexStart, indexCount); - m_boundVertexArray->SetVertexBuffers(encoder, 0, std::numeric_limits::max()); + m_boundVertexArray->SetVertexBuffers(encoder, 0, std::numeric_limits::max(), 0, GetBuiltInInstanceDataSlotCount()); } DrawInternal(encoder, fillMode); } @@ -2581,7 +2604,7 @@ namespace Babylon if (m_boundVertexArray != nullptr) { m_boundVertexArray->SetIndexBuffer(encoder, indexStart, indexCount); - m_boundVertexArray->SetVertexBuffers(encoder, 0, std::numeric_limits::max(), instanceCount); + m_boundVertexArray->SetVertexBuffers(encoder, 0, std::numeric_limits::max(), instanceCount, GetBuiltInInstanceDataSlotCount()); } DrawInternal(encoder, fillMode); } @@ -2597,7 +2620,7 @@ namespace Babylon bgfx::Encoder* encoder = GetEncoder(); if (m_boundVertexArray != nullptr) { - m_boundVertexArray->SetVertexBuffers(encoder, verticesStart, verticesCount); + m_boundVertexArray->SetVertexBuffers(encoder, verticesStart, verticesCount, 0, GetBuiltInInstanceDataSlotCount()); } DrawInternal(encoder, fillMode); } @@ -2612,7 +2635,7 @@ namespace Babylon bgfx::Encoder* encoder = GetEncoder(); if (m_boundVertexArray != nullptr) { - m_boundVertexArray->SetVertexBuffers(encoder, verticesStart, verticesCount, instanceCount); + m_boundVertexArray->SetVertexBuffers(encoder, verticesStart, verticesCount, instanceCount, GetBuiltInInstanceDataSlotCount()); } DrawInternal(encoder, fillMode); } @@ -3025,10 +3048,10 @@ namespace Babylon { const bgfx::Attrib::Enum attrib = instance.first; // "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. + // built-in instanced attributes (world0-3, splatIndex0-3, previousWorld0-3, + // instanceColor) are assigned synthetic locations at or above + // BUILTIN_INSTANCE_DATA_LAST_LOCATION, 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 diff --git a/Plugins/NativeEngine/Source/NativeEngine.h b/Plugins/NativeEngine/Source/NativeEngine.h index f92ab5722..98fb05cb9 100644 --- a/Plugins/NativeEngine/Source/NativeEngine.h +++ b/Plugins/NativeEngine/Source/NativeEngine.h @@ -126,6 +126,7 @@ namespace Babylon void DeleteFrameBuffer(NativeDataStream::Reader& data); void BindFrameBuffer(NativeDataStream::Reader& data); void UnbindFrameBuffer(NativeDataStream::Reader& data); + uint32_t GetBuiltInInstanceDataSlotCount() const; void DrawIndexed(NativeDataStream::Reader& data); void DrawIndexedInstanced(NativeDataStream::Reader& data); void Draw(NativeDataStream::Reader& data); diff --git a/Plugins/NativeEngine/Source/VertexArray.cpp b/Plugins/NativeEngine/Source/VertexArray.cpp index 6cec3a373..a041b6a84 100644 --- a/Plugins/NativeEngine/Source/VertexArray.cpp +++ b/Plugins/NativeEngine/Source/VertexArray.cpp @@ -1,5 +1,7 @@ #include "VertexArray.h" #include +#include +#include "Babylon/Graphics/BgfxShaderInfo.h" #include "Babylon/Graphics/DeviceContext.h" namespace Babylon @@ -48,10 +50,15 @@ namespace Babylon throw std::runtime_error{"Instancing is not supported"}; } - // bgfx allows instancing on at most 4 vec4 attributes - if (m_vertexBufferInstances.size() > 4) + // Instance data is packed into the top i_data slots, of which bgfx has + // MAX_INSTANCE_DATA_SLOT_COUNT. Only a new attribute can overflow: re-recording + // one that is already present overwrites its entry and needs no extra slot. + // The check runs before the insert, so `size() >= max` is the entry that would + // overflow. + if (m_vertexBufferInstances.find(attrib) == m_vertexBufferInstances.end() && + m_vertexBufferInstances.size() >= Babylon::Graphics::MAX_INSTANCE_DATA_SLOT_COUNT) { - throw std::runtime_error{"Number of vertex buffer instances greater than 4 is not supported"}; + throw std::runtime_error{"Number of vertex buffer instances greater than " + std::to_string(Babylon::Graphics::MAX_INSTANCE_DATA_SLOT_COUNT) + " is not supported"}; } m_vertexBufferInstances[attrib] = {vertexBuffer, byteOffset, byteStride, static_cast(sizeof(float) * numElements)}; @@ -82,14 +89,14 @@ namespace Babylon } } - void VertexArray::SetVertexBuffers(bgfx::Encoder* encoder, uint32_t startVertex, uint32_t numVertices, uint32_t instanceCount) + void VertexArray::SetVertexBuffers(bgfx::Encoder* encoder, uint32_t startVertex, uint32_t numVertices, uint32_t instanceCount, uint32_t minInstanceDataSlotCount) { // Check if instancing is supported. const bool instancingSupported = 0 != (BGFX_CAPS_INSTANCING & bgfx::getCaps()->supported); if (!m_vertexBufferInstances.empty() && instancingSupported) { bgfx::InstanceDataBuffer instanceDataBuffer{}; - VertexBuffer::BuildInstanceDataBuffer(instanceDataBuffer, m_vertexBufferInstances, instanceCount); + VertexBuffer::BuildInstanceDataBuffer(instanceDataBuffer, m_vertexBufferInstances, instanceCount, minInstanceDataSlotCount); encoder->setInstanceDataBuffer(&instanceDataBuffer); } diff --git a/Plugins/NativeEngine/Source/VertexArray.h b/Plugins/NativeEngine/Source/VertexArray.h index 8a162e805..a8d9cd1ef 100644 --- a/Plugins/NativeEngine/Source/VertexArray.h +++ b/Plugins/NativeEngine/Source/VertexArray.h @@ -22,7 +22,7 @@ namespace Babylon void RecordVertexBuffer(VertexBuffer* vertexBuffer, uint32_t location, uint32_t byteOffset, uint32_t byteStride, uint32_t numElements, uint32_t type, bool normalized, uint32_t divisor); void SetIndexBuffer(bgfx::Encoder* encoder, uint32_t firstIndex, uint32_t numIndices); - void SetVertexBuffers(bgfx::Encoder* encoder, uint32_t startVertex, uint32_t numVertices, uint32_t instanceCount = 0); + void SetVertexBuffers(bgfx::Encoder* encoder, uint32_t startVertex, uint32_t numVertices, uint32_t instanceCount = 0, uint32_t minInstanceDataSlotCount = 0); const std::map& GetInstances() const { return m_vertexBufferInstances; } diff --git a/Plugins/NativeEngine/Source/VertexBuffer.cpp b/Plugins/NativeEngine/Source/VertexBuffer.cpp index 651878c34..81fd0523e 100644 --- a/Plugins/NativeEngine/Source/VertexBuffer.cpp +++ b/Plugins/NativeEngine/Source/VertexBuffer.cpp @@ -1,5 +1,6 @@ #include "VertexBuffer.h" #include "Babylon/Graphics/DeviceContext.h" +#include #include namespace Babylon @@ -124,7 +125,7 @@ namespace Babylon } } - void VertexBuffer::BuildInstanceDataBuffer(bgfx::InstanceDataBuffer& instanceDataBuffer, const std::map& instances, uint32_t instanceCount) + void VertexBuffer::BuildInstanceDataBuffer(bgfx::InstanceDataBuffer& instanceDataBuffer, const std::map& instances, uint32_t instanceCount, uint32_t minSlotCount) { // bgfx expects that each instance attribute occupies exactly one 16-byte slot. static constexpr uint16_t kSlotSize = 16; @@ -145,15 +146,25 @@ namespace Babylon return; } - const uint16_t instanceStride = static_cast(instances.size() * kSlotSize); + // The buffer must cover every i_data slot the vertex shader reads, not just the ones the + // draw supplied data for: bgfx derives the number of instance-data inputs it declares from + // this buffer's stride, and D3D11's CreateInputLayout fails outright when the vertex + // shader's input signature reads a semantic the layout does not declare. Babylon.js can + // legitimately draw with fewer: _renderWithThinInstances creates the previousWorld buffer + // only *after* the first draw, so that draw binds world0-3 while the effect already + // declares previousWorld0-3. The padded slots stay zeroed, matching what WebGL feeds a + // vertex attribute whose array is disabled. + const size_t slotCount = std::max(static_cast(minSlotCount), instances.size()); + const uint16_t instanceStride = static_cast(slotCount * kSlotSize); // Create instance datas. Instance Data Buffer is transient. bgfx::allocInstanceDataBuffer(&instanceDataBuffer, instanceCount, instanceStride); uint8_t* data{instanceDataBuffer.data}; - // Zero the buffer so any unused bytes within a 16-byte slot (when ElementSize < 16) read as - // zero in the shader instead of leaking transient ring-buffer garbage. + // Zero the buffer so any unused bytes within a 16-byte slot (when ElementSize < 16), and any + // slot the draw supplied no data for at all, read as zero in the shader instead of leaking + // transient ring-buffer garbage. std::memset(data, 0, static_cast(instanceStride) * instanceCount); // Reverse because bgfx maps instance data in reverse attrib order: diff --git a/Plugins/NativeEngine/Source/VertexBuffer.h b/Plugins/NativeEngine/Source/VertexBuffer.h index 5ee6551af..d92af75b2 100644 --- a/Plugins/NativeEngine/Source/VertexBuffer.h +++ b/Plugins/NativeEngine/Source/VertexBuffer.h @@ -40,7 +40,9 @@ namespace Babylon uint32_t ElementSize{}; }; - static void BuildInstanceDataBuffer(bgfx::InstanceDataBuffer& instanceDataBuffer, const std::map& instances, uint32_t instanceCount); + /// `minSlotCount` is the number of i_data slots the vertex shader reads; the buffer is + /// padded with zeroed slots when the draw supplied fewer instanced attributes than that. + static void BuildInstanceDataBuffer(bgfx::InstanceDataBuffer& instanceDataBuffer, const std::map& instances, uint32_t instanceCount, uint32_t minSlotCount = 0); private: Graphics::DeviceContext& m_deviceContext; diff --git a/Plugins/ShaderCompiler/Source/ShaderCompilerCommon.cpp b/Plugins/ShaderCompiler/Source/ShaderCompilerCommon.cpp index e2222792a..e8080aa64 100644 --- a/Plugins/ShaderCompiler/Source/ShaderCompilerCommon.cpp +++ b/Plugins/ShaderCompiler/Source/ShaderCompilerCommon.cpp @@ -21,10 +21,12 @@ namespace Babylon::ShaderCompilerCommon 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. + // depends on is the *lowest* built-in one (the i_data slot at BUILTIN_INSTANCE_DATA_SLOT_COUNT - 1). + // Were bgfx::Attrib::Count to grow past it, the assert above would still pass while that attribute + // 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)); + static_assert(Babylon::Graphics::BUILTIN_INSTANCE_DATA_SLOT_COUNT <= Babylon::Graphics::MAX_INSTANCE_DATA_SLOT_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 e1d2cfe67..7cd59813f 100644 --- a/Plugins/ShaderCompiler/Source/ShaderCompilerTraversers.cpp +++ b/Plugins/ShaderCompiler/Source/ShaderCompilerTraversers.cpp @@ -570,34 +570,77 @@ namespace Babylon::ShaderCompilerTraversers { return true; } - return (!strcmp(name, "world0") || - !strcmp(name, "world1") || - !strcmp(name, "world2") || - !strcmp(name, "world3") || - !strcmp(name, "instanceColor") || - !strcmp(name, "splatIndex0") || - !strcmp(name, "splatIndex1") || - !strcmp(name, "splatIndex2") || - !strcmp(name, "splatIndex3")); + return IsBuiltInInstance(name); } - // True when the name has no built-in instance mapping and must be assigned a - // generic per-instance i_data slot (top TEXCOORD semantics). - bool IsGenericInstance(const char* name) const + // The per-instance attributes Babylon.js declares by convention (as opposed to the + // consumer-declared ones the caller routes explicitly). previousWorld0-3 carry the + // previous frame's world matrix and are declared alongside world0-3 whenever the + // effect needs motion vectors (object based motion blur, prepass velocity). + static bool IsBuiltInInstance(const char* name) { - if (m_instancedAttributes == nullptr || m_instancedAttributes->count(name) == 0) - { - return false; - } - return strcmp(name, "world0") != 0 && - strcmp(name, "world1") != 0 && - strcmp(name, "world2") != 0 && - strcmp(name, "world3") != 0 && - strcmp(name, "instanceColor") != 0 && - strcmp(name, "splatIndex0") != 0 && - strcmp(name, "splatIndex1") != 0 && - strcmp(name, "splatIndex2") != 0 && - strcmp(name, "splatIndex3") != 0; + return Babylon::Graphics::IsBuiltInInstanceAttributeName(name); + } + + // True when the caller routed this attribute explicitly. NativeEngine::Draw derives the + // supplied location from the draw-time packing rank over *every* recorded instanced + // attribute, built-in or not, so it already accounts for attributes this shader also + // maps by name. It therefore wins over the built-in slot map: on OpenGL/Metal the + // built-ins are recorded at their stable, name-sorted locations, which puts them in the + // same location range as consumer-declared ones, so a generic attribute sorting after a + // built-in (e.g. `zOffset` after `world3`) shifts the packing and only the caller-supplied + // locations reflect it. On D3D the built-ins carry synthetic locations at or above + // BUILTIN_INSTANCE_DATA_LAST_LOCATION, are never rerouted, and so never reach this map. + bool HasCallerSuppliedInstanceLocation(const char* name) const + { + return m_instancedAttributes != nullptr && m_instancedAttributes->count(name) != 0; + } + + // Assign an i_data slot to every built-in per-instance attribute this shader declares + // and the caller did not route explicitly. + // + // The slots cannot come from a fixed per-name table because bgfx requires the used + // i_data slots to form a contiguous run starting at i_data0: the D3D11 input layout + // declares TEXCOORD31 down to TEXCOORD(31 - N + 1) at 16-byte-dense offsets, and the + // GL path compacts the i_data attribute locations it finds. The declared set varies + // (world0-3 alone, plus instanceColor, plus previousWorld0-3), so a fixed table would + // leave a hole in the run and the attributes past the hole would read zero. + // + // Assigning in reverse name order -- the alphabetically first name gets the highest + // slot, the last gets i_data0 -- is dense by construction, is stable between the base + // program and any instanced variant (the declared set is identical), and matches + // BuildInstanceDataBuffer, which packs the recorded instance buffers by descending + // attribute location. It also reproduces the previous fixed assignment exactly for the + // sets that existed before previousWorld0-3 (world0-3 on i_data3..i_data0, instanceColor + // on i_data4). + void AssignBuiltInInstanceSlots() + { + unsigned int slot{}; + for (const auto& [name, symbol] : m_varyingNameToSymbol) + { + if (IsBuiltInInstance(name.c_str()) && !HasCallerSuppliedInstanceLocation(name.c_str())) + { + ++slot; + } + } + + if (slot > Babylon::Graphics::BUILTIN_INSTANCE_DATA_SLOT_COUNT) + { + throw std::runtime_error("Shader declares " + std::to_string(slot) + " built-in per-instance attributes, but at most " + std::to_string(Babylon::Graphics::BUILTIN_INSTANCE_DATA_SLOT_COUNT) + " are supported."); + } + + for (const auto& [name, symbol] : m_varyingNameToSymbol) + { + if (IsBuiltInInstance(name.c_str()) && !HasCallerSuppliedInstanceLocation(name.c_str())) + { + m_builtInInstanceSlots[name] = --slot; + } + } + } + + unsigned int GetBuiltInInstanceSlot(const char* name) const + { + return m_builtInInstanceSlots.at(name); } // The shader attribute location assigned to a varying must be stable regardless @@ -625,6 +668,7 @@ namespace Babylon::ShaderCompilerTraversers unsigned int m_genericAttributesRunningCount{0}; const std::map* m_instancedAttributes{nullptr}; std::map m_varyingNameToSymbol{}; + std::map m_builtInInstanceSlots{}; std::vector> m_symbolsToParents{}; // This table is a copy of the table bgfx uses for vertex attribute -> shader symbol association. @@ -678,6 +722,7 @@ namespace Babylon::ShaderCompilerTraversers "i_data14", "i_data15", }; + static_assert(BX_COUNTOF(s_attribInstanceName) == Babylon::Graphics::MAX_INSTANCE_DATA_SLOT_COUNT); }; /// Implementation of VertexVaryingInTraverser for OpenGL and Metal @@ -691,18 +736,8 @@ namespace Babylon::ShaderCompilerTraversers traverser.m_instancedAttributes = &instancedAttributes; intermediate->getTreeRoot()->traverse(&traverser); - // Pre-count instance attributes so i_data names can be assigned in reverse. - // bgfx maps i_data0 to the last attribute (TEXCOORD7), so instance names - // must be assigned in reverse order, matching the Metal traverser. Generic - // (consumer-declared) instanced attributes are excluded here because they are - // routed to an explicit i_data slot from their caller-supplied location. - for (const auto& [name, symbol] : traverser.m_varyingNameToSymbol) - { - if (traverser.IsInstance(name.c_str()) && !traverser.IsGenericInstance(name.c_str())) - { - traverser.m_instanceAttributeCount++; - } - } + // Assign the dense i_data slots the built-in per-instance attributes will use. + traverser.AssignBuiltInInstanceSlots(); VertexVaryingInTraverser::Traverse(intermediate, ids, replacementToOriginalName, traverser); } @@ -717,11 +752,12 @@ namespace Babylon::ShaderCompilerTraversers const unsigned int stableLocation = GetStableLocation(name); if (stableLocation >= static_cast(bgfx::Attrib::Count)) throw std::runtime_error("Cannot support more than " + std::to_string(static_cast(bgfx::Attrib::Count)) + " vertex attributes."); - if (IsGenericInstance(name)) + if (HasCallerSuppliedInstanceLocation(name)) { - // Consumer-declared instanced attribute: route to the explicit bgfx i_data - // slot derived from its caller-supplied per-instance location (INSTANCE_DATA_FIRST_LOCATION - // == i_data0 == TEXCOORD31, descending), matching BuildInstanceDataBuffer's packing and the D3D path. + // Explicitly routed instanced attribute: use the caller-supplied per-instance + // location (INSTANCE_DATA_FIRST_LOCATION == i_data0 == TEXCOORD31, descending). + // It is derived from the packing rank over every recorded instanced attribute, + // so it stays correct when built-in and consumer-declared ones are mixed. const unsigned int location = m_instancedAttributes->at(name); const unsigned int slot = Babylon::Graphics::INSTANCE_DATA_FIRST_LOCATION - location; if (slot >= BX_COUNTOF(s_attribInstanceName)) @@ -730,13 +766,12 @@ namespace Babylon::ShaderCompilerTraversers } if (IsInstance(name)) { - // Reverse: bgfx maps i_data0 to the highest semantic (TEXCOORD31), - // so the first instance attribute gets the highest i_data index. - return {stableLocation, s_attribInstanceName[--m_instanceAttributeCount]}; + // bgfx maps i_data0 to the highest instance-data semantic, so the slots run + // in reverse: see AssignBuiltInInstanceSlots. + return {stableLocation, s_attribInstanceName[GetBuiltInInstanceSlot(name)]}; } return {stableLocation, s_attribName[stableLocation]}; } - unsigned int m_instanceAttributeCount{0}; }; class VertexVaryingInTraverserMetal final : private VertexVaryingInTraverser @@ -748,6 +783,7 @@ namespace Babylon::ShaderCompilerTraversers VertexVaryingInTraverserMetal traverser{}; traverser.m_instancedAttributes = &instancedAttributes; intermediate->getTreeRoot()->traverse(&traverser); + traverser.AssignBuiltInInstanceSlots(); traverser.Traverse(intermediate, ids, replacementToOriginalName); } @@ -776,13 +812,6 @@ namespace Babylon::ShaderCompilerTraversers const bool isInstance = IsInstance(name.c_str()); if ((pass == 0 && isInstance) || (pass == 1 && !isInstance)) { - // Count only built-in instance attributes for the reverse i_data - // assignment; generic (consumer-declared) instanced attributes are - // routed to an explicit i_data slot from their caller-supplied location. - if (pass == 0 && !IsGenericInstance(name.c_str())) - { - m_instanceAttributeCount++; - } continue; } HandleVarying(name, symbol, publicType, intermediate, ids, originalNameToReplacement, replacementToOriginalName, *this); @@ -802,11 +831,12 @@ namespace Babylon::ShaderCompilerTraversers const unsigned int stableLocation = GetStableLocation(name); if (stableLocation >= static_cast(bgfx::Attrib::Count)) throw std::runtime_error("Cannot support more than " + std::to_string(static_cast(bgfx::Attrib::Count)) + " vertex attributes."); - if (IsGenericInstance(name)) + if (HasCallerSuppliedInstanceLocation(name)) { - // Consumer-declared instanced attribute: route to the explicit bgfx i_data - // slot derived from its caller-supplied per-instance location (INSTANCE_DATA_FIRST_LOCATION - // == i_data0 == TEXCOORD31, descending), matching BuildInstanceDataBuffer's packing and the D3D path. + // Explicitly routed instanced attribute: use the caller-supplied per-instance + // location (INSTANCE_DATA_FIRST_LOCATION == i_data0 == TEXCOORD31, descending). + // It is derived from the packing rank over every recorded instanced attribute, + // so it stays correct when built-in and consumer-declared ones are mixed. const unsigned int location = m_instancedAttributes->at(name); const unsigned int slot = Babylon::Graphics::INSTANCE_DATA_FIRST_LOCATION - location; if (slot >= BX_COUNTOF(s_attribInstanceName)) @@ -815,11 +845,10 @@ namespace Babylon::ShaderCompilerTraversers } if (IsInstance(name)) { - return {stableLocation, s_attribInstanceName[--m_instanceAttributeCount]}; + return {stableLocation, s_attribInstanceName[GetBuiltInInstanceSlot(name)]}; } return {stableLocation, s_attribName[stableLocation]}; } - unsigned int m_instanceAttributeCount{0}; }; /// Implementation of VertexVaryingInTraverser for DirectX @@ -832,6 +861,8 @@ namespace Babylon::ShaderCompilerTraversers VertexVaryingInTraverserD3D traverser{}; traverser.m_instancedAttributes = &instancedAttributes; intermediate->getTreeRoot()->traverse(&traverser); + // Assign the dense i_data slots the built-in per-instance attributes will use. + traverser.AssignBuiltInInstanceSlots(); // UVs are effectively a special kind of generic attribute since they both use // are implemented using texture coordinates, so we preprocess to pre-count the // number of UV coordinate variables to prevent collisions. @@ -848,13 +879,14 @@ namespace Babylon::ShaderCompilerTraversers private: std::pair GetVaryingLocationAndNewNameForName(const char* name) { - // Consumer-declared instanced attributes with no built-in mapping (e.g. the - // fluid renderer's `position` or an instanced `color`) are routed to the bgfx - // per-instance i_data location supplied by the caller. That location is derived - // from the draw-time instance packing order (INSTANCE_DATA_FIRST_LOCATION == i_data0 + // Instanced attributes the caller routed explicitly (e.g. the fluid renderer's + // `position` or an instanced `color`) are bound to the bgfx per-instance i_data + // location it supplied. That location is derived from the draw-time packing rank + // over every recorded instanced attribute (INSTANCE_DATA_FIRST_LOCATION == i_data0 // == TEXCOORD31, descending), so per-instance data reaches the shader instead of the - // per-vertex input. - if (IsGenericInstance(name)) + // per-vertex input, and the mapping stays correct when built-in and consumer-declared + // instanced attributes are mixed. + if (HasCallerSuppliedInstanceLocation(name)) { const unsigned int location = m_instancedAttributes->at(name); const unsigned int slot = Babylon::Graphics::INSTANCE_DATA_FIRST_LOCATION - location; @@ -862,6 +894,17 @@ namespace Babylon::ShaderCompilerTraversers throw std::runtime_error(std::string{"Instanced attribute '"} + name + "' has location " + std::to_string(location) + " which does not map to a valid bgfx i_data slot (computed slot " + std::to_string(slot) + ")."); return {location, s_attribInstanceName[slot]}; } + if (IsInstance(name)) + { + // Built-in instanced attribute: its i_data slot was assigned from the set this + // shader declares (see AssignBuiltInInstanceSlots), and the synthetic location + // follows from the slot. Combined with BuildInstanceDataBuffer's descending-key + // packing this puts the attribute on the i_data slot bgfx reads it from. The + // i_data name is cosmetic on D3D (binding is by TEXCOORD semantic, resolved from + // the location via the HLSLVertexAttributeRemap table). + const unsigned int slot = GetBuiltInInstanceSlot(name); + return {Babylon::Graphics::INSTANCE_DATA_FIRST_LOCATION - slot, s_attribInstanceName[slot]}; + } #define IF_NAME_RETURN_ATTRIB(varyingName, attrib, newName) \ if (std::strcmp(name, varyingName) == 0) \ { \ @@ -877,22 +920,6 @@ namespace Babylon::ShaderCompilerTraversers IF_NAME_RETURN_ATTRIB("color", bgfx::Attrib::Color0, "a_color0") IF_NAME_RETURN_ATTRIB("matricesIndices", bgfx::Attrib::Indices, "a_indices") IF_NAME_RETURN_ATTRIB("matricesWeights", bgfx::Attrib::Weight, "a_weight") - // Built-in instanced attributes: each occupies a fixed synthetic instance-data location. - // world0..world3 (and splatIndex0..3) pack lowest-location -> highest i_data slot so that, - // 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). 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") - IF_NAME_RETURN_ATTRIB("world2", Babylon::Graphics::INSTANCE_DATA_FIRST_LOCATION - 1, "i_data1") - IF_NAME_RETURN_ATTRIB("world3", Babylon::Graphics::INSTANCE_DATA_FIRST_LOCATION - 0, "i_data0") - IF_NAME_RETURN_ATTRIB("splatIndex0", Babylon::Graphics::INSTANCE_DATA_FIRST_LOCATION - 3, "i_data3") - IF_NAME_RETURN_ATTRIB("splatIndex1", Babylon::Graphics::INSTANCE_DATA_FIRST_LOCATION - 2, "i_data2") - IF_NAME_RETURN_ATTRIB("splatIndex2", Babylon::Graphics::INSTANCE_DATA_FIRST_LOCATION - 1, "i_data1") - IF_NAME_RETURN_ATTRIB("splatIndex3", Babylon::Graphics::INSTANCE_DATA_FIRST_LOCATION - 0, "i_data0") #undef IF_NAME_RETURN_ATTRIB const unsigned int attributeLocation = FIRST_GENERIC_ATTRIBUTE_LOCATION + m_genericAttributesRunningCount++; if (attributeLocation >= static_cast(bgfx::Attrib::Count))