Skip to content

Commit 02add01

Browse files
Fix instancing data layout for all graphics backends. (#1656)
VertexBuffer: Always pack instance data in reverse attribute order. bgfx maps i_data0 to the highest semantic (TEXCOORD7) in all backends, so reverse iteration is correct universally, not just D3D11/D3D12. ShaderCompilerTraversers: Fix OpenGL traverser to assign i_data names in reverse order (--m_instanceAttributeCount), matching the Metal traverser. Previously it used forward order (m_instanceAttributeIndex++) which produced wrong attribute-to-slot mapping when combined with reverse instance buffer packing. ShaderCompilerTraversers: Add splatIndex0-3 to IsInstance() so Gaussian Splatting attributes are recognized as instance data on OpenGL and Metal. config.json: Re-enable Gaussian Splatting tests on OpenGL and Metal now that instancing is fixed for those backends. Co-authored-by: Branimir Karadzic <branimirkaradzic@gmail.com>
1 parent e8ee5f7 commit 02add01

3 files changed

Lines changed: 28 additions & 21 deletions

File tree

Apps/Playground/Scripts/config.json

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -5,22 +5,13 @@
55
"title": "Gaussian Splatting Compressed ply SH",
66
"playgroundId": "#U8O4EP#1",
77
"renderCount": 15,
8-
"referenceImage": "gsplat-compressedply-sh.png",
9-
"excludedGraphicsApis": [ "OpenGL" ],
10-
"comments": {
11-
"OpenGL": "Test doesn't render correctly"
12-
}
8+
"referenceImage": "gsplat-compressedply-sh.png"
139
},
1410
{
1511
"title": "Gaussian Splatting Update Data",
1612
"playgroundId": "#Q0LBM8#2",
1713
"renderCount": 15,
18-
"excludedGraphicsApis": [ "OpenGL", "Metal" ],
19-
"referenceImage": "gsplat-update-data.png",
20-
"comments": {
21-
"OpenGL": "Test doesn't render correctly",
22-
"Metal": "Crashes"
23-
}
14+
"referenceImage": "gsplat-update-data.png"
2415
},
2516
{
2617
"title": "Native Canvas",
@@ -56,10 +47,10 @@
5647
"playgroundId": "#QFIGLW#9",
5748
"renderCount": 10,
5849
"errorRatio": 6,
59-
"excludedGraphicsApis": [ "OpenGL" ],
6050
"referenceImage": "gltfExtensionExtMeshGpuInstancingTest.png",
51+
"excludedGraphicsApis": [ "OpenGL" ],
6152
"comments": {
62-
"OpenGL": "Test works with OpenGL but it so slow that CI times out"
53+
"OpenGL": "Test works with OpenGL but it so slow that CI times out"
6354
}
6455
},
6556
{

Plugins/NativeEngine/Source/VertexBuffer.cpp

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -145,12 +145,10 @@ namespace Babylon
145145
uint8_t* data{instanceDataBuffer.data};
146146
uint32_t offset{};
147147

148-
// Reverse because bgfx is also reversed: https://github.com/bkaradzic/bgfx/blob/4581f14cd481bad1e0d6292f0dd0a6e298c2ee18/src/renderer_d3d11.cpp#L2701
149-
#if D3D11 || D3D12
148+
// Reverse because bgfx maps instance data in reverse attrib order:
149+
// D3D11: TEXCOORD7 = i_data0, TEXCOORD6 = i_data1, etc.
150+
// OpenGL also expects this layout since bgfx abstracts the mapping.
150151
for (auto iter = instances.rbegin(); iter != instances.rend(); ++iter)
151-
#else
152-
for (auto iter = instances.cbegin(); iter != instances.cend(); ++iter)
153-
#endif
154152
{
155153
const auto& element{iter->second};
156154
const auto* source{element.Buffer->m_bytes.data()};

Plugins/ShaderCompiler/Source/ShaderCompilerTraversers.cpp

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -548,7 +548,11 @@ namespace Babylon::ShaderCompilerTraversers
548548
!strcmp(name, "world1") ||
549549
!strcmp(name, "world2") ||
550550
!strcmp(name, "world3") ||
551-
!strcmp(name, "instanceColor"));
551+
!strcmp(name, "instanceColor") ||
552+
!strcmp(name, "splatIndex0") ||
553+
!strcmp(name, "splatIndex1") ||
554+
!strcmp(name, "splatIndex2") ||
555+
!strcmp(name, "splatIndex3"));
552556
}
553557

554558
unsigned int m_genericAttributesRunningCount{0};
@@ -598,6 +602,18 @@ namespace Babylon::ShaderCompilerTraversers
598602
auto intermediate{program.getIntermediate(EShLangVertex)};
599603
VertexVaryingInTraverserOpenGL traverser{};
600604
intermediate->getTreeRoot()->traverse(&traverser);
605+
606+
// Pre-count instance attributes so i_data names can be assigned in reverse.
607+
// bgfx maps i_data0 to the last attribute (TEXCOORD7), so instance names
608+
// must be assigned in reverse order, matching the Metal traverser.
609+
for (const auto& [name, symbol] : traverser.m_varyingNameToSymbol)
610+
{
611+
if (IsInstance(name.c_str()))
612+
{
613+
traverser.m_instanceAttributeCount++;
614+
}
615+
}
616+
601617
VertexVaryingInTraverser::Traverse(intermediate, ids, replacementToOriginalName, traverser);
602618
}
603619

@@ -611,14 +627,16 @@ namespace Babylon::ShaderCompilerTraversers
611627
m_genericAttributesRunningCount++;
612628
if (IsInstance(name))
613629
{
614-
return {static_cast<unsigned int>(m_genericAttributesRunningCount - 1), s_attribInstanceName[m_instanceAttributeIndex++]};
630+
// Reverse: bgfx maps i_data0 to the highest semantic (TEXCOORD7),
631+
// so the first instance attribute gets the highest i_data index.
632+
return {static_cast<unsigned int>(m_genericAttributesRunningCount - 1), s_attribInstanceName[--m_instanceAttributeCount]};
615633
}
616634
if (m_genericAttributesRunningCount >= static_cast<unsigned int>(bgfx::Attrib::Count))
617635
throw std::runtime_error("Cannot support more than 18 vertex attributes.");
618636

619637
return {static_cast<unsigned int>(m_genericAttributesRunningCount - 1), s_attribName[static_cast<unsigned int>(m_genericAttributesRunningCount - 1)]};
620638
}
621-
unsigned int m_instanceAttributeIndex{0};
639+
unsigned int m_instanceAttributeCount{0};
622640
};
623641

624642
class VertexVaryingInTraverserMetal final : private VertexVaryingInTraverser

0 commit comments

Comments
 (0)