Skip to content

NativeEngine: route instanced generic attributes landing on TexCoord3..TexCoord15 - #1801

Open
bkaradzic-microsoft wants to merge 1 commit into
BabylonJS:masterfrom
bkaradzic-microsoft:pr/instancing-vertex-attributes
Open

NativeEngine: route instanced generic attributes landing on TexCoord3..TexCoord15#1801
bkaradzic-microsoft wants to merge 1 commit into
BabylonJS:masterfrom
bkaradzic-microsoft:pr/instancing-vertex-attributes

Conversation

@bkaradzic-microsoft

Copy link
Copy Markdown
Member

Fixes a silent data-loss bug in the divisor-driven instancing reroute in NativeEngine::Draw.

Problem

When Babylon.js declares a generic (consumer-named) vertex attribute with divisor == 1, the attribute is compiled to an ordinary per-vertex bgfx::Attrib slot, but bgfx can only feed per-instance data through the i_data slots (the top TEXCOORD semantics). NativeEngine therefore reroutes such attributes to the correct i_data slot via a lazily-compiled program variant.

That reroute was guarded by attrib < bgfx::Attrib::TexCoord3, so any generic instanced attribute that landed on TexCoord3 or above was silently skipped. BuildInstanceDataBuffer still packed its data into the instance data buffer, but the shader kept reading the attribute as a per-vertex input — i.e. unrelated vertex bytes.

Sprite instancing assigns cellInfo -> TexCoord3, so instanced sprites read garbage cell info and rendered collapsed / mis-UVed.

Fix

Widen the guard to every real bgfx attribute (attrib < bgfx::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 >= bgfx::Attrib::Count, so they still compare false and remain correctly skipped — they already arrive as instance data.

Evidence

Measured on Win32 x64 D3D11 (Debug) with Babylon.js _features.supportSpriteInstancing forced to true, so the instanced sprite path is actually exercised. Negative control = this commit reverted, everything else identical:

test without fix with fix
155 Sprites 4.242% FAIL 1.405% PASS
293 Prepass SSAO + sprites 1.617% pass 0.001% pass
369 Sprites Pixel Perfect 25.532% FAIL 7.612% FAIL (much closer, still over threshold)

Full validation suite is unchanged: ran=295 passed=295 failed=0 (--test-index=0-52,58-719; 53-57 are the pre-existing scissor crash on master).

Notes

  • No config.json changes. Un-excluding the sprite/MSDF tests additionally requires the Babylon.js-side supportSpriteInstancing flag; this PR is the Native prerequisite for that work and is a strict improvement on its own.
  • No shader cache version bump needed. Instanced program variants bypass the shader cache entirely (ShaderProvider::Get consults the cache only when instancedAttributes is empty), and the non-instanced compilation path is untouched.

…..TexCoord15

The divisor-driven instancing reroute only remapped consumer-instanced
attributes whose compiled bgfx Attrib was below TexCoord3, silently dropping
every generic instanced attribute that landed on TexCoord3 or above. Sprite
instancing assigns cellInfo -> TexCoord3, so its per-instance data was packed
into the instance data buffer by BuildInstanceDataBuffer but never read by the
shader: the attribute stayed a per-vertex input reading unrelated vertex bytes,
which collapsed and mis-UVed instanced sprites.

Widen the guard to every real bgfx Attrib (< Attrib::Count). The built-in
instanced attributes (world0-3, splatIndex0-3, instanceColor) use synthetic
locations at/above INSTANCE_DATA_FIRST_LOCATION - 4, which is >= Attrib::Count,
so they still compare false and remain correctly skipped.

No shader cache invalidation is required: instanced program variants bypass the
cache (ShaderProvider::Get uses the cache only when instancedAttributes is
empty), and the non-instanced compilation path is unchanged.

Measured with Babylon.js _features.supportSpriteInstancing forced to true
(Win32 x64 D3D11 Debug), before -> after:

  idx 155  Sprites                    4.242% FAIL -> 1.405% PASS
  idx 293  Prepass SSAO + sprites     1.617% pass -> 0.001% pass
  idx 369  Sprites Pixel Perfect     25.532% FAIL -> 7.612% FAIL (still over)

The sprite/MSDF tests stay excluded here because un-excluding them additionally
requires the Babylon.js-side supportSpriteInstancing change; this commit is the
Native prerequisite.

Full suite unchanged: ran=295 passed=295 failed=0 (--test-index=0-52,58-719).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Copilot AI review requested due to automatic review settings July 29, 2026 21:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a silent data-loss issue in NativeEngine::Draw where divisor-driven (per-instance) generic vertex attributes were not rerouted to bgfx i_data slots when their compiled attribute location was TexCoord3..TexCoord15, causing shaders to read incorrect per-vertex data (notably impacting sprite instancing).

Changes:

  • Expand the instanced-attribute reroute guard from attrib < bgfx::Attrib::TexCoord3 to attrib < bgfx::Attrib::Count so all real bgfx per-vertex attributes are handled.
  • Add detailed in-code documentation clarifying why synthetic instance-data locations (>= bgfx::Attrib::Count) remain excluded and how the packing/reroute mapping works.

Comment on lines +2434 to +2438
// Reroute every consumer-instanced attribute that was compiled to a real
// per-vertex bgfx Attrib slot (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 still compare false here and are correctly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants