Map previousWorld0-3 so instanced motion vectors are correct

Core/Graphics/InternalInclude/Babylon/Graphics/BgfxShaderInfo.h

@@ -1,8 +1,10 @@
 #pragma once
 
+#include <array>
 #include <cstdint>
 #include <string>
 #include <map>
+#include <string_view>
 #include <vector>
 
 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<std::string_view, 13> 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<uint8_t> VertexBytes{};

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<uint32_t>::max());
+            m_boundVertexArray->SetVertexBuffers(encoder, 0, std::numeric_limits<uint32_t>::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<uint32_t>::max(), instanceCount);
+            m_boundVertexArray->SetVertexBuffers(encoder, 0, std::numeric_limits<uint32_t>::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

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);

Plugins/NativeEngine/Source/VertexArray.cpp

@@ -1,5 +1,7 @@
 #include "VertexArray.h"
 #include <cassert>
+#include <string>
+#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<uint16_t>(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);
         }
 

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<bgfx::Attrib::Enum, VertexBuffer::InstanceInfo>& GetInstances() const { return m_vertexBufferInstances; }
 

Plugins/NativeEngine/Source/VertexBuffer.cpp

@@ -1,5 +1,6 @@
 #include "VertexBuffer.h"
 #include "Babylon/Graphics/DeviceContext.h"
+#include <algorithm>
 #include <cassert>
 
 namespace Babylon
@@ -124,7 +125,7 @@ namespace Babylon
         }
     }
 
-    void VertexBuffer::BuildInstanceDataBuffer(bgfx::InstanceDataBuffer& instanceDataBuffer, const std::map<bgfx::Attrib::Enum, InstanceInfo>& instances, uint32_t instanceCount)
+    void VertexBuffer::BuildInstanceDataBuffer(bgfx::InstanceDataBuffer& instanceDataBuffer, const std::map<bgfx::Attrib::Enum, InstanceInfo>& 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<uint16_t>(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<size_t>(minSlotCount), instances.size());
+        const uint16_t instanceStride = static_cast<uint16_t>(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<size_t>(instanceStride) * instanceCount);
 
         // Reverse because bgfx maps instance data in reverse attrib order:

Plugins/NativeEngine/Source/VertexBuffer.h

@@ -40,7 +40,9 @@ namespace Babylon
             uint32_t ElementSize{};
         };
 
-        static void BuildInstanceDataBuffer(bgfx::InstanceDataBuffer& instanceDataBuffer, const std::map<bgfx::Attrib::Enum, InstanceInfo>& 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<bgfx::Attrib::Enum, InstanceInfo>& instances, uint32_t instanceCount, uint32_t minSlotCount = 0);
 
     private:
         Graphics::DeviceContext& m_deviceContext;

Plugins/ShaderCompiler/Source/ShaderCompilerCommon.cpp

@@ -21,10 +21,12 @@ namespace Babylon::ShaderCompilerCommon
     static_assert(Babylon::Graphics::INSTANCE_DATA_FIRST_LOCATION >= static_cast<uint32_t>(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<uint32_t>(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.