NativeEngine: divisor-driven per-instance vertex attributes#1739
NativeEngine: divisor-driven per-instance vertex attributes#1739bkaradzic-microsoft wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect routing of consumer-declared per-instance vertex attributes that previously ended up bound to per-vertex shader inputs unless they matched a hardcoded instanced-name list. The solution makes instancing behavior depend on the vertex-buffer divisor by compiling and selecting shader variants that explicitly route divisor-driven attributes into bgfx’s i_data slots.
Changes:
- Extend shader compilation (D3D/OpenGL/Metal/Vulkan paths) to accept an
instancedAttributesmap (name -> i_data target location) and route those attributes to expliciti_data0..4slots. - Add lazy, per-program caching of instanced shader variants keyed by the instanced-attribute map, and select the correct variant at draw time.
- Re-enable previously excluded Playground validation tests by removing
excludeFromAutomaticTestingentries for the affected particle/instancing scenarios.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Plugins/ShaderCompiler/Source/ShaderCompilerVulkan.cpp | Passes instanced-attribute routing info into the traverser for Vulkan compilation. |
| Plugins/ShaderCompiler/Source/ShaderCompilerTraversers.h | Adds instancedAttributes parameter (with docs) to varying-location assigners; includes needed headers. |
| Plugins/ShaderCompiler/Source/ShaderCompilerTraversers.cpp | Implements generic instanced-attribute routing to explicit i_data slots across D3D/OpenGL/Metal traversers. |
| Plugins/ShaderCompiler/Source/ShaderCompilerOpenGL.cpp | Threads instancedAttributes into OpenGL shader compilation. |
| Plugins/ShaderCompiler/Source/ShaderCompilerMetal.cpp | Threads instancedAttributes into Metal shader compilation. |
| Plugins/ShaderCompiler/Source/ShaderCompilerDXIL.cpp | Threads instancedAttributes into DXIL shader compilation. |
| Plugins/ShaderCompiler/Source/ShaderCompilerDXBC.cpp | Threads instancedAttributes into DXBC shader compilation. |
| Plugins/ShaderCompiler/InternalInclude/Babylon/Plugins/ShaderCompiler.h | Updates public compiler API to accept instanced-attribute routing map (default empty). |
| Plugins/NativeEngine/Source/VertexArray.h | Exposes recorded instance-attribute bindings for draw-time variant selection. |
| Plugins/NativeEngine/Source/ShaderProvider.h | Updates shader-provider API to accept instanced-attribute routing map. |
| Plugins/NativeEngine/Source/ShaderProvider.cpp | Bypasses source-only shader cache for instanced variants and compiles with routing map. |
| Plugins/NativeEngine/Source/Program.h | Stores sources and adds instanced-variant caching keyed by the instanced-attribute map. |
| Plugins/NativeEngine/Source/Program.cpp | Implements lazy instanced-variant compilation and destroys cached variant program handles on dispose. |
| Plugins/NativeEngine/Source/NativeEngine.cpp | Computes divisor-driven instanced attribute routing from bound vertex array and submits the matching program variant. |
| Apps/Playground/Scripts/config.json | Re-enables excluded tests by removing excludeFromAutomaticTesting flags for impacted cases. |
| #include <set> | ||
| #include <string> | ||
|
|
| for (const auto& [attrib, instanceInfo] : instances) | ||
| { |
| if (slot >= BX_COUNTOF(s_attribInstanceName)) | ||
| throw std::runtime_error("Instanced attribute location is not a valid bgfx i_data slot."); |
Consumer-declared per-instance vertex attributes (e.g. GPU particle position/color/angle/size, or a custom instanced color buffer) were routed to per-vertex shader slots because the ShaderCompiler only recognized the built-in instanced names (world0-3, instanceColor, splatIndex). Any other attribute backed by an instance buffer (divisor > 0) was assigned a per-vertex location, so its data was read incorrectly at draw time. Drive instancing purely from the vertex-buffer divisor instead of a fixed name list: - ShaderCompiler traversers (D3D, OpenGL, Metal) now take a map<name, target-location> of instanced attributes and route each declared attribute to an explicit bgfx i_data slot (TEXCOORD7..TEXCOORD3 = i_data0..i_data4). Built-in instanced names keep their existing reverse-count assignment. - Program caches lazily-compiled instanced shader variants keyed by the instanced-attribute map; the base program is used when nothing is instanced. - NativeEngine computes the instanced-attribute set at draw time from the bound vertex array's per-attribute divisors and submits the matching variant. This removes the need for any JS-side hint. Enables 17 previously-excluded Playground validation tests that failed under the prior name-list instancing (the GPU particle suite plus the instanced color-buffer test), verified passing on D3D11 and OpenGL. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
49f4b03 to
64df62e
Compare
|
Pushed an update addressing CI + review feedback: CI fix (Linux GL): That test's original exclusion reason was "crashes or hangs" (not the instancing follow-up), so it has been re-excluded. The genuinely instancing-attributable particle tests are confirmed passing on OpenGL (e.g. Copilot review nits (all addressed):
|
Problem
Consumer-declared per-instance vertex attributes were routed to per-vertex shader slots, so their data was read incorrectly at draw time. The
ShaderCompileronly recognized the built-in instanced names (world0-3,instanceColor,splatIndex); any other attribute backed by an instance buffer (divisor > 0) — e.g. GPU-particleposition/color/angle/size, or a custom instanced color buffer — was assigned a per-vertex location.Fix
Drive instancing purely from the vertex-buffer divisor instead of a fixed name list:
map<name, target-location>of instanced attributes and route each declared attribute to an explicit bgfxi_dataslot (TEXCOORD7..TEXCOORD3=i_data0..i_data4). Built-in instanced names keep their existing reverse-count assignment.This generalizes the instancing path so it works for any divisor-driven attribute, not just the hardcoded names, and supersedes the prior name-list approach.
Test enablement
Re-enables 18 previously-excluded Playground validation tests that failed under the old name-list instancing (most were marked as follow-ups to the #1691 instance-data stride fix). These are the GPU-particle suite plus the instanced color-buffer test:
Particles,Instances with color buffer,Prepass SSAO + particles,halo-particle-system,particle-system-with-custom-nme-shader, andParticles - Basic Properties / Change / Emitters / Attractorsvariants.Each was confirmed to actually exercise the fixed path (it compiles a per-instance shader variant) and now matches its reference image.
Verification
ran=18 passed=18 failed=0); no regression in existing instanced tests (Gaussian Splatting, mesh instancing).