Bump Babylon.js to 9.21.2 and fix i_data slot overlap - #1852
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes i_data slot collisions between caller-supplied instance attributes and shader built-in instance attributes when a draw records only a subset of built-ins (notably affecting GL/Metal thin instances + motion blur).
Changes:
- Update built-in instance slot assignment to choose the lowest unclaimed i_data slots rather than starting from 0.
- Expand instance data buffer coverage to include recorded instance attributes plus unrecorded built-ins so D3D11 input layouts remain valid.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Plugins/ShaderCompiler/Source/ShaderCompilerTraversers.cpp | Assigns missing built-in instance attributes to truly free i_data slots, avoiding overlap with caller-packed slots. |
| Plugins/NativeEngine/Source/NativeEngine.cpp | Computes required i_data span using recorded instance attributes plus missing built-ins to avoid D3D11 layout rejection. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
35e0059 to
eadcb87
Compare
Clustered lighting, the FrameGraph tests and several prepass/SSAO tests construct scenes against engine APIs added after 9.15.0 and throw before any shader is compiled, so they cannot be validated on the pinned version.
eadcb87 to
d09fac1
Compare
60742d1 to
4379a7d
Compare
bgfx exposes every non-sampler uniform as a float vec4, so ChangeUniformTypes
retypes each loose scalar/vector uniform to vec4 and inserts a shape conversion
at every use site to narrow it back.
addShapeConversion only reconciles vector *shape*, not basic type. Given a vec4
source and an int target it builds an EOpConstructInt aggregate whose operand is
still a float and calls it done, so the AST claims "int" while holding a float.
glslang faithfully lowers that to SPIR-V in which an integer operation consumes a
float, and SPIRV-Cross renders it verbatim.
For ssao2's blur pass, which declares `uniform int samples` and loops
`for (int i = -samples; i < samples; i += 2)`, the generated ESSL 300 was:
uniform vec4 samples;
...
mediump int _42 = -samples.x;
for (mediump int i = _42; i < samples.x; i += 2)
GLES rejects both lines, bgfx raises Fatal::InvalidShader, and the validation run
aborts. HLSL happened to tolerate it because it converts implicitly, which is why
this only ever showed up on the OpenGL and OpenGLES backends.
Shape the value to float first and then ask glslang for a real conversion node,
which produces the expected `int(...)` and correctly typed SPIR-V.
Validated on Windows: D3D11 is unchanged at 351/351, and on an ANGLE/GLES build
eleven tests go from Fatal::InvalidShader to passing - the whole prepass SSAO
family (287, 289, 293, 296, 299, 302, 304, 305, 306) plus GUI Slate (176) and
GUI Near Menu (177).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
These were excluded against Babylon.js 9.15.0 and now pass. Verified on
Win32 D3D11 both per-test in isolation and in a single sequential process
covering indices 56-719 (ran=256 passed=256 failed=0), since several of
these exclusions describe order-dependent behaviour that a per-test sweep
cannot reproduce.
137 Volumetric Light Scattering Post Process with Morph Targets exact
287 Prepass SSAO + particles exact
289 Prepass SSAO + instanced bones 0.012%
299 Prepass SSAO + GUI 1.064%
302 Prepass SSAO + highlight layer 0.018%
304 Prepass SSAO + on/off post-process exact
305 Prepass SSAO + thin instances 0.003%
306 Prepass SSAO + depth renderer 0.044%
363 Screen Space Reflections 2 1.315%
Three carried backend-specific exclusion reasons that cannot be reproduced
on a D3D11 host, so CI is the arbiter for them:
137 "Pixel comparison fails on Linux (large diff)"
287 "SSAO2 blur post-process shader fails to compile on desktop GL"
299 OpenGL "mediump float" compile failure in PrePassRenderer, plus an
order-dependent state leak that produced a ~6000 px diff right at the
2.5% threshold; it now measures 1.064% in sequential order.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
With the uniform basic-type fix in place the ssao2 blur shader compiles on
OpenGL, so the Ubuntu jobs now get past shader compilation for this test and
reach the particle shader instead, where llvmpipe's JIT aborts the process:
BGFX attr a_tangent: 2
BGFX instance data i_data0: 4
...
LLVM ERROR: Cannot emit physreg copy instruction
--- BN: ABORT --- SIGABRT raised.
All four Ubuntu jobs stop at exactly the same place, immediately after linking
the particle program (diffuseSampler / textureMask / i_data0-3), so this is not
the SSAO shader and not something we emit differently - it is a Mesa software
rasterizer codegen bug on the CI runner. The test itself passes on D3D11 and on
an ANGLE/GLES build.
The other eight tests enabled by the Babylon.js 9.21.2 bump stay enabled.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
With the uniform basic-type fix in place these shaders compile on OpenGL, but
the Ubuntu runner's llvmpipe then aborts the process while JIT-compiling them:
LLVM ERROR: Cannot emit physreg copy instruction
--- BN: ABORT --- SIGABRT raised.
The abort moved from 287 to 289 once 287 was skipped, so it follows the shader
family rather than any one scene. All four Ubuntu jobs stop at byte-identical
positions, and this is a register allocator failure inside Mesa's software
rasterizer, not something Babylon Native can emit its way around. The family
already had a history of this: 288, 290, 291 and 292 were excluded on master
with "Test crashes or hangs on Babylon Native".
These tests do pass on D3D11 and, with this PR's shader fixes, on an ANGLE/GLES
build, which is recorded in each reason so the exclusion is understood as
CI-runner-specific rather than a statement about the renderer.
Tests 137 and 363 remain enabled.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
GUI Near Menu was excluded from OpenGL with this recorded reason:
OpenGL: BGFX FATAL shader compile error in GUI fragment shader
('=' : cannot convert from 'highp float' to 'bool')
That is precisely the bug fixed earlier in this branch: a bool uniform widened
to a float vec4 was narrowed back by shape alone, so the AST claimed bool while
holding a float. The test now compiles and passes on OpenGL, so the graphics
API exclusion is removed and it runs everywhere.
Screen Space Reflections 2 is excluded instead. It reaches a multiple-render-
target resolve, where bgfx does:
const GLenum drawBuffer = GL_COLOR_ATTACHMENT0 + colorIdx;
GL_CHECK(glDrawBuffers(1, &drawBuffer) );
GLES requires bufs[i] to be GL_NONE or GL_COLOR_ATTACHMENT0 + i, so resolving
any attachment past the first is GL_INVALID_OPERATION. Desktop GL uses
glDrawBuffer just above and has no such restriction, but Babylon Native builds
the ES path on Linux, so the Ubuntu jobs hit it. The test passes on D3D11.
A full sequential run now completes on an ANGLE/GLES build with no assert and
no BGFX FATAL: 286 ran, 282 passed. The four remaining reds are the three
motion blur tests that BabylonJS#1839 fixes, plus one ANGLE-only
pixel difference in MeshDebugPluginMaterial. D3D11 is unchanged at 297/300,
red on the same three motion blur tests.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
4379a7d to
dc3c8f2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- Apps/package-lock.json: Generated file
Suppressed comments (6)
Apps/Playground/Scripts/config.json:1858
- This global exclusion also skips the test on D3D11 (and every other backend), even though the updated reason says the failure is specific to llvmpipe/OpenGL and that D3D11 passes. Use the existing API-specific exclusion so passing backends retain coverage.
"excludeFromAutomaticTesting": true,
Apps/Playground/Scripts/config.json:1938
- This global exclusion also skips the test on D3D11 (and every other backend), even though the updated reason says the failure is specific to llvmpipe/OpenGL and that D3D11 passes. Use the existing API-specific exclusion so passing backends retain coverage.
"excludeFromAutomaticTesting": true,
Apps/Playground/Scripts/config.json:1962
- This global exclusion also skips the test on D3D11 (and every other backend), even though the updated reason says the failure is specific to llvmpipe/OpenGL and that D3D11 passes. Use the existing API-specific exclusion so passing backends retain coverage.
"excludeFromAutomaticTesting": true,
Apps/Playground/Scripts/config.json:1978
- This global exclusion also skips the test on D3D11 (and every other backend), even though the updated reason says the failure is specific to llvmpipe/OpenGL and that D3D11 passes. Use the existing API-specific exclusion so passing backends retain coverage.
"excludeFromAutomaticTesting": true,
Apps/Playground/Scripts/config.json:1986
- This global exclusion also skips the test on D3D11 (and every other backend), even though the updated reason says the failure is specific to llvmpipe/OpenGL and that D3D11 passes. Use the existing API-specific exclusion so passing backends retain coverage.
"excludeFromAutomaticTesting": true,
Apps/Playground/Scripts/config.json:1994
- This global exclusion also skips the test on D3D11 (and every other backend), even though the updated reason says the failure is specific to llvmpipe/OpenGL and that D3D11 passes. Use the existing API-specific exclusion so passing backends retain coverage.
"excludeFromAutomaticTesting": true,
`Win32_x64_D3D11` and `Win32_x64_D3D11_Sanitizers` have hit the one-hour
job timeout on every run of this PR. Both are the jobs that build the
default JavaScript engine, Chakra; the V8, Hermes, JSI and QuickJS jobs
on the same matrix all pass. The hang is reproducible locally with a
Chakra build and is deterministic: `Playground --headless
--test-index=23` never returns, spinning one core and growing the heap
by ~7 MB/s until the runner gives up.
Test 23 is "Glow layer and LODs". The scene itself renders fine - what
never completes is `Scene.executeWhenReady`, because
`EffectLayer.isReady` stays false forever:
frame 300 scene.isReady=false layer.isReady(subMesh)=false
_shadersLoaded=false isLayerReady=false
while the same probe on V8 flips everything to true by frame 50. The
one link that never settles is `ThinGlowLayer._importShadersAsync()`.
The cause is a Chakra bug, exposed by a Babylon.js code-generation
change. `super.x` inside an arrow function nested in a class method
resolves to the *derived* class's own method instead of the base:
class A { foo() { return "BASE"; } }
class B extends A {
foo() {
const s = Object.create(null, { foo: { get: () => super.foo } });
return s.foo.call(this); // V8: "BASE" Chakra: recurses
}
}
TypeScript emits exactly that `Object.create(null, { get: () => super.x })`
helper for a `super` call inside an `async` method, and Babylon.js
started shipping it in the UMD bundle in 9.16.0 - which is precisely
where this PR's bump to 9.21.2 crosses. Called synchronously it dies
with "Out of stack space"; called from a promise chain, as
`_importShadersAsync` is, each level is a fresh microtask, so it recurses
forever without ever overflowing the stack, never settles, and burns CPU
and memory - the exact signature seen on the runner.
The repo already has the remedy. `scripts/downlevelNativeScripts.mjs`
transpiles the bundles to ES5 for this very reason ("Babylon Native's
Chakra engine consumes ES5-level script"); TypeScript's ES5 emit rewrites
`super.x` to `_super.prototype.x` and drops the arrow entirely. It was
only ever wired into `getNightly`, so builds that take Babylon.js from
npm - which is every normal build - ran the un-downleveled ES2015 bundle.
Running it from `postinstall` closes that gap for `npm install`,
`npm ci`, CI and local builds alike, and leaves the nightly path alone
(`getNightly.js` still downlevels the files it refills from the CDN).
Validated on Windows/D3D11, Debug, tests 0-52 and 56-719 (53-55 crash
locally in Debug regardless of this change):
| engine | before | after |
|---|---|---|
| Chakra | hangs at test 23 | **297/300** |
| V8 | 297/300 | **297/300** |
Byte-identical results on V8, and Chakra now matches it. The three
remaining failures are the motion-blur trio that BabylonJS#1839 fixes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
dc3c8f2 to
0864f3d
Compare
0864f3d to
934d42c
Compare
934d42c to
a6f4866
Compare
|
[Commented by Copilot on behalf of @bghgary] The implementation now carries the compiler-assigned built-in slot map into runtime and packs recorded attributes into their exact slots. The PR description still says it chooses the lowest unclaimed slots and relies on zero padding, which describes the superseded implementation. Please update the Fix paragraph before merge. |
…ce attributes AssignBuiltInInstanceSlots numbered the built-ins the draw did not record from zero, the same range the draw-time caller slots use, and threw when the two collided. On OpenGL/Metal the built-ins are routed through the caller map, so a shader declaring world0-3 and previousWorld0-3 whose first draw records only world0-3 always collided -- this is the "Thin instances + render self motion blur" failure on the Linux CI legs since Babylon.js 9.21.2. Assign the lowest free slots instead, and size the instance data buffer from the recorded attribute count plus the built-ins the draw left out, so those land in the buffer's zero padding on both backends. Verified: Linux/OpenGL validation sweep 289/289 (was 288/289), Win32 D3D11 306/306. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
The reasons on these eight say the failure is specific to the OpenGL CI runner and that D3D11 passes, but the global excludeFromAutomaticTesting flag skipped every backend. Narrow them to excludedGraphicsApis: ["OpenGL"]; the sweep runs only on Linux/GL and Win32/D3D11, so this restores D3D11 coverage without enabling them anywhere untested. Win32 D3D11 sweep: 314/314 (was 306/306). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 88569c10-a7ff-4373-9a58-afa9c68b8c09
a6f4866 to
c722274
Compare
Bring in i_data slot-overlap fix (BabylonJS#1852) and JsRuntimeHost pin (BabylonJS#1854 content via shotgun fork). Keep shotgun JsRuntimeHost/SPIRV-Cross forks, fragCoord Y flip, GPU instance repack, compute shader path, and playground enables. Resolve config by preserving shotgun enablements and taking upstream OpenGL CI exclusions where applicable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Bumps Babylon.js 9.15.0 -> 9.21.2, and fixes the i_data slot collision that bump exposes.
The bug
AssignBuiltInInstanceSlotsnumbered the built-in per-instance attributes a draw did not record starting at zero -- the same range the draw-time caller slots use -- and threw when the two collided.OpenGL/Metal route built-ins through the caller map, so a shader declaring
world0-3andpreviousWorld0-3whose first draw records onlyworld0-3always collides (_renderWithThinInstancescreates the previousWorld buffer after the first draw). That isThin instances + render self motion blurfailing on the four Ubuntu legs. D3D keeps built-ins out of that map, so it only collides once a draw mixes a generic instanced attribute with unrecorded built-ins.Fix: give the built-ins the lowest slots not already claimed by a caller-supplied location, and size the instance data buffer from the recorded attributes plus the built-ins the draw left out, so those land in the buffer's zero padding.
Introduced by #1839, latent on master only because the lockfile pinned 9.15.0.
Supporting commits
Restore the original basic type when narrowing widened uniformsis a shader-compiler fix required by 9.21.2. The rest enable 9 newly-passing tests, downlevel the bundles to ES5 for Chakra (typescriptmoves todependenciesbecause the postinstall hook needs it under--omit=dev), and retarget the prepass SSAO / SSR2 exclusions.Those eight were already globally excluded on master for a Mesa/LLVM crash on the Ubuntu runner; they are now
excludedGraphicsApis: ["OpenGL"]so D3D11 keeps covering them. The sweep only runs on Linux/GL and Win32/D3D11, so this enables them nowhere untested.Testing