Map previousWorld0-3 so instanced motion vectors are correct - #1839
Conversation
A mesh rendered with object based motion blur (or prepass velocity) declares previousWorld0-3 alongside world0-3, so an instanced or thin-instanced mesh has 8 per-instance vec4 attributes, 9 with instanceColor. previousWorld0-3 had no built-in mapping, and VertexArray::RecordVertexBuffer rejected anything past the 5th instanced buffer, so those buffers were never recorded: the shader read a zero previous world matrix, produced a huge bogus velocity and smeared the whole object. This is what the nightly has been failing on since MRT support (BabylonJS#1754) turned object based motion blur on for Native. The i_data slots cannot come from a fixed per-name table. bgfx requires the used 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 locations it finds. Since the declared set varies (world0-3 alone, plus instanceColor, plus previousWorld0-3), a fixed table leaves a hole in the run and every attribute past the hole reads zero. So assign the slots per shader, from the set that shader actually declares, in reverse name order: the alphabetically first name gets the highest slot and the last gets i_data0. That is dense by construction, is stable between the base program compile and any instanced variant (the declared set is identical), and matches BuildInstanceDataBuffer, which packs the recorded buffers by descending attribute location. It also reproduces the previous fixed D3D table exactly for every set that existed before previousWorld0-3, so nothing else changes. OpenGL and Metal already assigned densely; they now share the same map instead of each counting attributes themselves. Raise the instanced buffer limit to bgfx's real BGFX_CONFIG_MAX_INSTANCE_DATA_COUNT (16, not the 5 its stale doc comment claims) and fix the off-by-one: the check ran before the insert, so it rejected the 5th buffer rather than the 6th. Playground validation, D3D11, Babylon.js 9.21.2: Thin instances + dynamic buffer resize 3.346% -> 2.478% Thin instances + render self motion blur 2.844% -> 2.355% Instances + render self motion blur 2.531% -> 2.061% all now under the 2.5% allowance, and the "Number of vertex buffer instances greater than 4 is not supported" errors go from 9 to 0. The numbers are bit-identical on hardware D3D11 and on forced WARP. A full 720 test A/B sweep shows no other test changing by more than 0.001%. A smaller residual difference remains on these three tests, from a separate bug: Babylon.js's bindAttachments is a no-op on Native, so the scene clear is applied to every MRT attachment and wipes the velocity attachment's alpha 0 background, leaving a faint halo around moving objects. That needs a matching Babylon.js change and is not addressed here. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect instanced motion vectors by adding built-in shader compiler handling for previousWorld0-3 and ensuring per-instance i_data slots are assigned densely per shader (as required by bgfx). It also lifts the NativeEngine instanced-buffer recording limit to match bgfx’s actual instance-data slot capacity.
Changes:
- Add
previousWorld0-3as built-in instanced attributes and assign built-in instancei_dataslots per shader from the declared set (dense, stable mapping across variants). - Add/align compile-time guards for instance-data slot/location invariants and maximum slot counts.
- Increase the NativeEngine instanced vertex-buffer instance limit to
MAX_INSTANCE_DATA_SLOT_COUNTand fix the prior off-by-one behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Plugins/ShaderCompiler/Source/ShaderCompilerTraversers.cpp | Adds previousWorld0-3 built-in instance detection and introduces per-shader dense built-in i_data slot assignment used across GL/Metal/D3D traversers. |
| Plugins/ShaderCompiler/Source/ShaderCompilerCommon.cpp | Updates static asserts/comments for the built-in instance-data location boundary and ensures built-in slot count doesn’t exceed max slot count. |
| Plugins/NativeEngine/Source/VertexArray.cpp | Raises instanced buffer instance limit to the bgfx max slot count and updates the thrown error message accordingly. |
| Plugins/NativeEngine/Source/NativeEngine.cpp | Updates documentation/comments for built-in instanced attribute rerouting behavior and the new built-in set. |
| Core/Graphics/InternalInclude/Babylon/Graphics/BgfxShaderInfo.h | Introduces MAX_INSTANCE_DATA_SLOT_COUNT, increases BUILTIN_INSTANCE_DATA_SLOT_COUNT to cover previousWorld0-3, and adjusts built-in location boundaries. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
NativeEngine::Draw derives the routed location from the packing rank over every recorded instanced attribute, so it already accounts for built-ins. On OpenGL/Metal the built-ins are recorded at their stable, name-sorted locations, which puts them in the same range as consumer-declared ones, so a generic attribute sorting after a built-in shifts the packing and only the caller-supplied location reflects it. Prefer that location everywhere and exclude those attributes from the built-in slot assignment. On D3D built-ins carry synthetic locations at or above BUILTIN_INSTANCE_DATA_LAST_LOCATION and are never rerouted, so they never reach the map and nothing changes there. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
Mapping previousWorld0-3 onto i_data slots makes the vertex shader read instance-data semantics the draw does not necessarily supply. bgfx sizes the instance-data half of the D3D11 input layout from the instance data buffer's stride, and CreateInputLayout fails outright (E_INVALIDARG) when the vertex shader's input signature reads a TEXCOORD the layout does not declare, so the mismatch is a hard crash rather than a wrong pixel. Babylon.js reaches that state legitimately. Mesh._renderWithThinInstances creates the previousWorld buffer *after* the draw that first needs it, so that first draw binds world0-3 only while the effect already declares previousWorld0-3: eight declared slots, four supplied. WebGL tolerates this -- an attribute whose array is disabled reads the generic attribute value -- and before previousWorld0-3 were built-ins so did Babylon Native, because a per-vertex attribute missing from the layout gets a dummy element from bgfx's fillVertexLayout. Only instance data has no such fallback. Count the built-in per-instance attributes the current program declares and pad the instance data buffer to that many 16-byte slots. The padding lands on the high slots, after the packed data, since BuildInstanceDataBuffer fills from i_data0 up; it is already zeroed. Move the built-in attribute name table into BgfxShaderInfo.h so the shader compiler and NativeEngine cannot disagree about which names count. Verified on D3D11: "Thin instances + render self motion blur" went from an assert in RendererContextD3D11::setInputLayout to validating at 2.355%, and the sequential validation run is back to 256/256. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
|
Pushed 5025029, which fixes a hard crash this PR would otherwise introduce. It is not reachable on the Babylon.js version this branch pins, which is why CI here is green — I only hit it while testing this PR together with #1840's bump to Babylon.js 9.21.2. What breaksTest 323 WhyTraced the offending draw: The vertex shader declares 8 i_data slots; the draw supplies 4. bgfx derives the instance-data half of the D3D11 input layout from the instance data buffer's stride ( Babylon.js gets there legitimately: this._bind(subMesh, effect, fillMode);
this._draw(subMesh, fillMode, instancesCount);
// Write current matrices as previous matrices
if (this._scene.needsPreviousWorldMatrices && !...previousMatrixData && ...matrixData) {
if (!this._thinInstanceDataStorage.previousMatrixBuffer) {
this._thinInstanceDataStorage.previousMatrixBuffer = this._thinInstanceCreateMatrixBuffer("previousWorld", ...);So the first draw binds FixCount the built-in per-instance attributes the current program declares and pad the instance data buffer to that many 16-byte slots. Padding lands on the high slots, after the packed data, since Verification
2.355% is bit-identical to what the instance-limit raise alone produced before this PR's slot assignment existed, so the padded first frame does not perturb the final image. |
Fixes the nightly Playground validation failures on the three motion blur tests.
Problem
A mesh rendered with object based motion blur (or prepass velocity) declares
previousWorld0-3alongsideworld0-3, so an instanced or thin-instanced mesh has 8 per-instancevec4attributes, 9 withinstanceColor.previousWorld0-3had no built-in mapping in the shader compiler, andVertexArray::RecordVertexBufferrejected anything past the 5th instanced buffer (Number of vertex buffer instances greater than 4 is not supported). Those buffers were never recorded, the shader read a zero previous world matrix, and produced a huge bogus velocity that smeared the whole object.This is what the nightly has been failing on since MRT support (#1754) enabled object based motion blur on Native.
Why slots are now assigned per shader
The
i_dataslots cannot come from a fixed per-name table. bgfx requires the used slots to form a contiguous run starting ati_data0:TEXCOORD31down toTEXCOORD(31 - N + 1)at 16-byte-dense offsets;i_datalocations it finds.Since the declared set varies (
world0-3alone, plusinstanceColor, pluspreviousWorld0-3), a fixed table leaves a hole in the run and every attribute past the hole reads zero.So slots are assigned per shader from the set that shader actually declares, in reverse name order: alphabetically first gets the highest slot, last gets
i_data0. That is dense by construction, stable between the base program compile and any instanced variant (identical declared set), and matchesBuildInstanceDataBuffer, which packs the recorded buffers by descending attribute location.It also reproduces the previous fixed D3D table exactly for every set that existed before
previousWorld0-3(world0-3→i_data3..0,+instanceColor→i_data4,splatIndex0-3→i_data3..0), so nothing else changes. OpenGL and Metal already assigned densely; they now share the same map instead of each counting attributes themselves.The instanced buffer limit is raised to bgfx's real
BGFX_CONFIG_MAX_INSTANCE_DATA_COUNT(16 — its doc comment still claims 5), and the off-by-one is fixed: the check ran before the insert, so it rejected the 5th buffer rather than the 6th.Validation
Playground validation, Win32 D3D11, Babylon.js 9.21.2:
All now under the 2.5% allowance, and the instance-limit errors go from 9 to 0. The numbers are bit-identical on hardware D3D11 and on forced WARP, so they are not adapter-flaky.
A full 720 test A/B sweep shows no other test changing by more than 0.001%, and an identical set of pre-existing hangs on both builds.
Known remaining difference
A smaller residual diff remains on these three tests, from a separate bug: Babylon.js's
bindAttachmentsis a no-op on Native, so the scene clear is applied to every MRT attachment and wipes the velocity attachment's alpha-0 background. The motion blur shader multiplies velocity by that alpha, so the background gets a fixed non-zero velocity and moving objects get a faint halo. Fixing it needs a matching Babylon.js change plus per-attachment clear masking here, so it is not addressed in this PR.The worst test passes at 2.478% against a 2.5% allowance, so there is little headroom until that second bug is fixed.