Skip to content

Correct gl_FragCoord Y orientation on D3D, Metal and Vulkan - #1840

Open
bkaradzic-microsoft wants to merge 11 commits into
BabylonJS:masterfrom
bkaradzic-microsoft:fix/fragcoord-y-orientation
Open

Correct gl_FragCoord Y orientation on D3D, Metal and Vulkan#1840
bkaradzic-microsoft wants to merge 11 commits into
BabylonJS:masterfrom
bkaradzic-microsoft:fix/fragcoord-y-orientation

Conversation

@bkaradzic-microsoft

Copy link
Copy Markdown
Member

Babylon Native's shader model is "shader-visible coordinates are GL-logical, converted to physical at each sampler access". FlipSamplerCoordinatesTraverser (texture v -> 1-v, texelFetch y -> h-1-y) and InvertYDerivativeOperandsTraverser (negate dFdy) implement that for DXBC/DXIL/Metal/Vulkan, and are skipped for OpenGL.

gl_FragCoord was the one shader input still left in physical space. D3D, Metal and Vulkan rasterize with a top-left origin while GL uses bottom-left, and Babylon Native does not flip geometry (ProcessShaderCoordinates only remaps depth), so for GL row y the hardware yields height - y - 0.5 instead of y + 0.5. gl_FragCoord.y arrives mirrored.

Shaders that sample at their own position are unaffected, because the physical/physical pairing is self-consistent. The mismatch only shows up where the row index itself is meaningful: prefix sums (iblCdfy), neighbour offsets, and copies into a differently-oriented target (copyTexture3DLayerToTexture). That is why 39 shaders reference gl_FragCoord but only a handful render incorrectly.

Change

FragCoordYFlipTraverser rewrites every gl_FragCoord read in the fragment stage to vec4(fc.x, targetHeight - fc.y, fc.z, fc.w). The correction is exactly height - y, with no -1 term. Shaders that never read gl_FragCoord are left byte-for-byte unchanged.

The height comes from a new vec4 uniform, bnFragCoordTargetSize, declared as a linker object so MoveNonSamplerUniformsIntoStruct sweeps it into the Frame struct like every other uniform. NativeEngine sets it in DrawInternal from the bound framebuffer's dimensions.

Notes:

  • bgfx's predefined u_viewRect is deliberately not reused: FrameBuffer::SetBgfxViewPortAndScissor narrows it to the viewport whenever one is set, whereas gl_FragCoord is relative to the whole render target.
  • FlipFragCoordY must run before ChangeUniformTypes / MoveNonSamplerUniformsIntoStruct so its uniform is collected with the rest.
  • A fresh replacement subtree is built per occurrence rather than reusing MakeReplacements, which maps one node per symbol name and would give that node multiple parents.
  • OpenGL is left alone, as with the other flip traversers.

Tests

Two render-and-readback tests in UnitTests, gated off where the existing render tests are (D3D12, noop Metal device):

  • FragCoordYIncreasesUpwards writes gl_FragCoord.y / height into a render target and checks the ramp is brightest at the top row. On a 64-row target it reads 253 / 126 / 2 for the top, middle and bottom rows, exactly the (height - row - 0.5) / height ramp the correction is derived from.
  • FragCoordAndUVAddressATextureIdentically samples one texture twice, once through the interpolated UVs of a full-screen quad and once through gl_FragCoord.xy / targetSize, and requires the two images to match. This is the addressing pattern used by order-independent transparency, TAA and screen space curvature, and it holds only if the gl_FragCoord correction and FlipSamplerCoordinatesTraverser compose to a no-op.

Both fail without the fix: the first ramp inverts to 2 / 129 / 253, and the second renders vertically mirrored (255..3 against 3..255).

Validation

  • Full UnitTests suite on D3D11. The only failure is the pre-existing JavaScript.All TextEncoder assertion, which is unrelated and also fails without this change.
  • 149 Playground validation tests on D3D11 with no pixel-diff regressions.
  • IBL voxel shadowing improves measurably: IBL Voxel Shadowing Right-Handed 13.455% -> 13.392% and Left-Handed 14.995% -> 14.592% pixel difference.

Risk

The blast radius is every shader that reads gl_FragCoord on D3D/Metal/Vulkan, so this is worth close review even though the shipped shaders that change behaviour are few.

Only DXBC was exercised on hardware. The DXIL, Metal and Vulkan call sites are the same one-line addition in the same position, but they are untested and would benefit from a run on those backends before merging.

No Playground validation test flips from failing to passing here. The tests that would exercise this most directly (order-independent transparency, TAA, screen space curvature, IBL voxel shadowing) are currently excluded for unrelated reasons, and the voxel tests additionally need a newer babylonjs than the pinned 9.15.0 to run at all. Hence the unit tests above, which pin the behaviour down independently of the npm version.

bkaradzic and others added 2 commits August 17, 2026 17:57
Babylon Native's shader model is "shader-visible coordinates are
GL-logical; convert to physical at each sampler access". This is
implemented by FlipSamplerCoordinatesTraverser (texture() v -> 1-v,
texelFetch y -> h-1-y) and InvertYDerivativeOperandsTraverser (negate
dFdy), which run for DXBC/DXIL/Metal/Vulkan but not OpenGL.

gl_FragCoord was the one shader input left in physical space. D3D, Metal
and Vulkan rasterize with a top-left origin while GL uses bottom-left,
and BN does not flip geometry (ProcessShaderCoordinates only remaps
depth). So for GL row y the hardware yields height - y - 0.5 instead of
y + 0.5, i.e. gl_FragCoord.y arrives mirrored.

Shaders using the symmetric "sample at my own position" pattern are
unaffected because the physical/physical pairing is self-consistent. The
mismatch only shows up where the row index itself is meaningful:
prefix sums (iblCdfy), neighbour offsets, and copies into a
differently-oriented target (copyTexture3DLayerToTexture). That is why
39 shaders reference gl_FragCoord but only a handful render incorrectly.

Add FragCoordYFlipTraverser, which rewrites every gl_FragCoord read in
the fragment stage to vec4(fc.x, targetHeight - fc.y, fc.z, fc.w). The
correction is exactly `height - y` with no -1 term (see derivation
above). Shaders that never read gl_FragCoord are left byte-for-byte
unchanged.

The target height comes from a new vec4 uniform, bnFragCoordTargetSize,
declared as a linker object so MoveNonSamplerUniformsIntoStruct sweeps
it into the "Frame" struct like every other uniform and it is emitted by
name into the bgfx uniform table. NativeEngine sets it in DrawInternal
from the bound framebuffer's dimensions.

bgfx's predefined u_viewRect is deliberately not used: it is narrowed to
the viewport by FrameBuffer::SetBgfxViewPortAndScissor whenever one is
set, whereas gl_FragCoord is relative to the whole render target.

FlipFragCoordY must run before ChangeUniformTypes /
MoveNonSamplerUniformsIntoStruct so the uniform is collected with the
rest. A fresh replacement subtree is built per occurrence rather than
reusing MakeReplacements, which maps one node per symbol name and would
give that node multiple parents - something later traversers do not
expect.

OpenGL is intentionally left alone, as with the other flip traversers.

Validated on D3D11: 149 tests validated with 0 pixel-diff failures.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Two render-and-readback tests in UnitTests, both gated off where the
existing render tests are (D3D12, noop Metal device).

FragCoordYIncreasesUpwards writes gl_FragCoord.y / height into a render
target and checks the ramp is brightest at the top row, matching GL's
bottom-left origin. Values come out as 253 / 126 / 2 for the top, middle
and bottom rows of a 64-row target, exactly the (height - row - 0.5) /
height ramp the correction is derived from.

FragCoordAndUVAddressATextureIdentically samples one texture twice, once
through the interpolated UVs of a full-screen quad and once through
gl_FragCoord.xy / targetSize, and requires the two images to match. That
is the addressing pattern used by order-independent transparency, TAA and
screen space curvature, and it only holds if the gl_FragCoord correction
and FlipSamplerCoordinatesTraverser compose to a no-op. Comparing the two
addressing modes against each other rather than against the source pixels
keeps the test independent of how createRawTexture orients its upload.

Both fail without FlipFragCoordY: the first ramp inverts to 2 / 129 / 253
and the second renders vertically mirrored (255..3 against 3..255).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Copilot AI lite review requested due to automatic review settings August 18, 2026 01:00

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 normalizes gl_FragCoord.y to OpenGL/WebGL’s bottom-left-origin convention on the non-OpenGL backends (D3D, Metal, Vulkan) by injecting an AST rewrite during shader compilation and supplying the render-target dimensions at draw time.

Changes:

  • Add a new shader-compiler traverser to rewrite every fragment-stage gl_FragCoord read to a Y-flipped equivalent using a new target-size uniform.
  • Populate the injected target-size uniform from the currently bound framebuffer dimensions in NativeEngine::DrawInternal.
  • Add two render-and-readback unit tests to pin down gl_FragCoord.y orientation and its composition with sampler coordinate flips.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
Plugins/ShaderCompiler/Source/ShaderCompilerVulkan.cpp Runs FlipFragCoordY in the Vulkan compilation pipeline before uniform transforms.
Plugins/ShaderCompiler/Source/ShaderCompilerMetal.cpp Runs FlipFragCoordY in the Metal compilation pipeline before uniform transforms.
Plugins/ShaderCompiler/Source/ShaderCompilerDXIL.cpp Runs FlipFragCoordY in the DXIL compilation pipeline before uniform transforms.
Plugins/ShaderCompiler/Source/ShaderCompilerDXBC.cpp Runs FlipFragCoordY in the DXBC compilation pipeline before uniform transforms.
Plugins/ShaderCompiler/Source/ShaderCompilerTraversers.h Declares and documents the new FlipFragCoordY traverser API.
Plugins/ShaderCompiler/Source/ShaderCompilerTraversers.cpp Implements FragCoordYFlipTraverser, declares bnFragCoordTargetSize, and rewrites gl_FragCoord reads.
Core/Graphics/InternalInclude/Babylon/Graphics/BgfxShaderInfo.h Introduces FRAGCOORD_TARGET_SIZE_UNIFORM_NAME constant for the injected uniform name.
Plugins/NativeEngine/Source/Program.h Adds cached lookup accessor for the injected uniform’s UniformInfo.
Plugins/NativeEngine/Source/Program.cpp Caches bnFragCoordTargetSize uniform info during program initialization.
Plugins/NativeEngine/Source/NativeEngine.cpp Sets bnFragCoordTargetSize each draw based on the bound framebuffer size (when present).
Apps/UnitTests/Source/Tests.ShaderCompilation.FragCoord.cpp Adds render/readback tests validating gl_FragCoord.y orientation and UV-vs-fragcoord addressing equivalence.
Apps/UnitTests/CMakeLists.txt Adds the new test source file to the UnitTests build.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread Apps/UnitTests/Source/Tests.ShaderCompilation.FragCoord.cpp
Matches Tests.ShaderCompilation.cpp and Tests.UniformPadding.cpp, and
avoids relying on the caps property being configurable.
Helpers::ReadPixels is a plain glReadPixels on OpenGL, which returns the
bottom scanline first, whereas the D3D11 path returns the top scanline
first. The test asserted an absolute ramp direction over readback rows, so
it encoded the D3D11 readback convention and failed on Linux even though
gl_FragCoord.y was correct there.

Compare normalized gl_FragCoord.y against the interpolated vUV.y written by
the same fragment invocation instead. The quad maps uv.y to clip y, so the
two ramps must agree on every backend regardless of readback row order, and
a flipped gl_FragCoord.y still misses by the full range of the ramp.

Renamed to FragCoordYMatchesInterpolatedUV to match what it now checks.
@bkaradzic-microsoft

Copy link
Copy Markdown
Member Author

CI caught a real problem with the test, though not with the fix. Pushed 666e1ad.

What failed: ShaderCompilation.FragCoordYIncreasesUpwards failed on the four Ubuntu jobs (OpenGL) and passed everywhere else. Linux read back 2 / 129 / 253 where Windows D3D11 read back 253 / 126 / 2 — an exact mirror.

Why: the test, not the traverser. Helpers::ReadPixels is a bare glReadPixels(0, 0, ...) on OpenGL, which returns the bottom scanline first, while the D3D11 path maps the staging texture and returns the top scanline first. My test asserted an absolute ramp direction over readback rows, so it silently encoded the D3D11 readback convention. On OpenGL 2 / 129 / 253 is the correct result: readback row 0 is the bottom scanline, which legitimately holds the smallest gl_FragCoord.y.

Worth stating plainly: gl_FragCoord behaviour on OpenGL is unchanged by this PR — FlipFragCoordY is only invoked from the DXBC, DXIL, Metal and Vulkan compilers. The GL result was correct before and after.

Fix: the test now writes normalized gl_FragCoord.y and the interpolated vUV.y into two channels of the same fragment invocation and requires them to agree row by row. The quad maps uv.y = 0 to clip y = -1 and uv.y = 1 to clip y = +1, so the two ramps must co-vary on every backend no matter which end of the image the readback starts at. Renamed to FragCoordYMatchesInterpolatedUV. This is the same orientation-agnostic approach as the second test, which is why that one passed on Linux unmodified.

Re-verified the negative control — with the FlipFragCoordY call commented out on D3D11:

row 0 fragCoord=2 uv=253, row 32 fragCoord=129 uv=126, row 63 fragCoord=253 uv=2
gl_FragCoord.y disagrees with the interpolated vUV.y at row 0 (gl_FragCoord=2, vUV=253)
[  FAILED  ] ShaderCompilation.FragCoordYMatchesInterpolatedUV
[  FAILED  ] ShaderCompilation.FragCoordAndUVAddressATextureIdentically

and with it restored:

row 0 fragCoord=253 uv=253, row 32 fragCoord=126 uv=126, row 63 fragCoord=2 uv=2
[  PASSED  ] 5 tests.

So the reworked assertion still fails by the full range of the ramp when the fix is absent — it did not become weaker by becoming portable.

@bkaradzic-microsoft

Copy link
Copy Markdown
Member Author

Short answer: no — and I can now show why rather than just reporting that the candidates I tried didn't flip.

I instrumented FragCoordYFlipTraverser with two counters: one that fires every time the traverser scans a fragment stage, and one that fires only when it finds at least one gl_FragCoord read to rewrite. Then I ran the Playground suite under the D3D11 backend.

Result: the traverser is invoked constantly and matches nothing. Across a 37-test spread sampling the whole 720-test catalog, plus ~40 hand-picked tests covering every feature whose shaders reference gl_FragCoord (OIT, TAA, screen-space curvature, volumetric lighting, clustered lighting, fluid rendering, prepass SSAO, IBL voxel/screen-space shadowing, GPU picking, shadow maps, Gaussian splatting):

fragment stages scanned : ~95
stages containing a gl_FragCoord read : 0

The reason is that in babylonjs 9.15.0 — the version pinned by Apps/package-lock.json, which is what npm install resolves for CI — essentially every gl_FragCoord use is behind a define that these tests don't enable:

Shader Guard
shadowMapPixelShader #if SM_SOFTTRANSPARENTSHADOW==1
copyTextureToTexturePixelShader #ifdef NO_SAMPLER
geometryPixelShader #ifdef IBL_SHADOW_TEXTURE

and the features that do use it unconditionally — OIT, TAA, curvature, volumetric, clustered lighting, the FrameGraph tests — abort before any shader is compiled, e.g.:

Failed to evaluate playground snippet #SYQW69#1366:
TypeError: Cannot read properties of undefined (reading 'COLOR_ATTACHMENT0')

Those depend on engine APIs added to Babylon.js after 9.15.0, so they fail during scene construction regardless of this change.

Two consequences worth stating explicitly:

  1. No validation test can be enabled by this PR on the pinned Babylon.js version. Enabling any of the candidates would produce a red CI for reasons unrelated to gl_FragCoord.
  2. This PR provably cannot regress the validation suite either — the rewrite is inert for every shader the suite currently compiles. That is consistent with CI, where the only failure was my own unit test making a bad assumption about readback row order.

This is precisely why the change ships with the two GPU-readback unit tests: they exercise gl_FragCoord directly and are independent of which Babylon.js version is pinned. Both have verified negative controls — comment out the FlipFragCoordY call and both fail by the full range of the ramp.

Once the pinned Babylon.js moves past 9.15.0 the OIT / TAA / curvature / clustered-lighting tests become the natural integration coverage for this, and I'm happy to follow up with a PR enabling them at that point.

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.
@bkaradzic-microsoft

Copy link
Copy Markdown
Member Author

Pushed three follow-up commits: the Babylon.js 9.21.2 bump, an engine fix the bump exposed, and the validation tests it unlocks.

The bump exposed a real bug

VertexArray::RecordVertexBuffer rejected more than 5 per-instance vertex attributes, citing "bgfx allows instancing on at most 4 vec4 attributes". That limit is stale — bgfx reports its capacity in caps.limits.maxInstanceData, which is 16 in the pinned BabylonJS/bgfx (bkaradzic/bgfx a4555482f, "Increased number of texcoord attributes, increased instance data"). I confirmed the pinned bgfx.cmake revision in this branch (865b8e6a) contains that commit, so the higher limit is available on CI and not just locally.

Babylon.js 9.21 draws thin instances with render-self motion blur using world0-3 plus previousWorld0-3 — 8 instanced attributes — so the stale cap threw out of _renderWithThinInstances and the mesh silently failed to draw. Reading the limit from bgfx fixes tests 321/322/323, which regress on 9.21.2 without it.

Regression check

All 302 previously-enabled tests, one process per test, on Win32 D3D11: 299 pass, 0 fail. The only non-passing are 53-55 (scissor), which crash identically on 9.15.0 — pre-existing on Windows/D3D11 and unrelated to this change.

9 tests enabled

Several of these exclusions describe order-dependent behaviour, which a per-test sweep structurally cannot reproduce, so I also ran a single sequential process over indices 56-719: ran=256 passed=256 failed=0.

# Test Diff
137 Volumetric Light Scattering + 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%

Two things I want to flag honestly

1. Three of these can only be judged by CI. Their exclusion reasons are backend-specific and I have no way to reproduce them on a D3D11 host: 137 "fails on Linux (large diff)", 287 "fails to compile on desktop GL", and 299 OpenGL mediump float compile failure. If the Ubuntu jobs go red on any of these, the right response is to re-exclude that test with an updated reason rather than to paper over it.

2. Tests 321 and 323 are now marginal — 99.1% and 94.2% of their error budget (2.478% and 2.355% against 2.5%). Bit-identical across three runs, so not flaky, but that margin is unlikely to survive a different backend. The cause is visible in the render: Babylon Native leaves a soft motion-blur halo around objects where Babylon.js converges to zero velocity, so there is a residual gap in the motion-blur path beyond the instance-limit bug. I ruled out stale reference images (substituting Babylon.js's own PNGs gives identical diffs). Worth a follow-up issue; I did not want to hide it behind a raised errorRatio.

Note that none of the newly enabled tests exercise gl_FragCoord — they are unlocked by the bump, not by the traverser. The tests that do prove the traverser are the clustered-lighting ones, and those additionally need StorageBuffer support that is not in this branch. The unit tests remain the proof for the gl_FragCoord change itself.

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
@bkaradzic-microsoft
bkaradzic-microsoft force-pushed the fix/fragcoord-y-orientation branch from e5e1799 to b6bc698 Compare August 18, 2026 17:44
@bkaradzic-microsoft

Copy link
Copy Markdown
Member Author

Correction to my previous comment: the instance-limit commit I pushed here duplicated work that already exists in #1839, so I have dropped it and force-pushed. VertexArray.cpp is now untouched by this PR.

This PR now depends on #1839. The Babylon.js 9.21.2 bump regresses three tests that #1839 fixes:

Test with bump, without #1839 with #1839
321 Thin instances + dynamic buffer resize 3.346% 2.478%
322 Instances + render self motion blur 2.531% 2.061%
323 Thin instances + render self motion blur 2.844% 2.355%

Until #1839 merges, those three will fail here. They should not be worked around in this PR.

For the record, my dropped commit and #1839 converged on the same guard independently (find(attrib) == end() && size() >= max, replacing the stale size() > 4), which is a useful cross-check on that reasoning. But #1839 is the correct fix and mine was not: it also maps previousWorld0-3 with dense per-shader slot assignment, which is the actual root cause. My limit-only change happened to reach the same three numbers via the existing generic-attribute reroute, but it left the contiguity guarantee resting on luck rather than construction.

#1839 also explains the residual halo I flagged, and it is a third, separate bug: 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, giving the background a fixed non-zero velocity. That is why 321 still sits at 99.1% of its error budget even after #1839, and it needs a matching Babylon.js change to fix properly.

The rest of this PR is unchanged: the gl_FragCoord traverser plus its unit tests, the 9.21.2 bump, and the 9 tests the bump unlocks.

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
@bkaradzic-microsoft

Copy link
Copy Markdown
Member Author

Pushed 2eaaf94d, which fixes the Ubuntu/OpenGL failures.

What was failing

The three Ubuntu jobs aborted at test 287 "Prepass SSAO + particles" with BGFX FATAL 0x00000001: Failed to compile shader. Because a fatal aborts the whole run, tests 289/299/302/304/305/306/363 were never reached, so a single bug accounted for all seven of the remaining reds.

Root cause

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.

ssao2's blur pass declares uniform int samples and loops for (int i = -samples; i < samples; i += 2). Dumping the generated ESSL 300:

uniform vec4 samples;
...
mediump int _42 = -samples.x;
for (mediump int i = _42; i < samples.x; i += 2)
ERROR: 0:21: '=' : cannot convert from 'mediump float' to 'mediump int'
ERROR: 0:22: '<' : wrong operand types - no operation '<' exists that takes a left-hand operand of type 'mediump int' and a right operand of type 'mediump float'

HLSL tolerated this because it converts implicitly, which is why it only ever showed up on the OpenGL and OpenGLES backends.

Fix

Shape the value to float first, then ask glslang for a real conversion node via addConversion(basicType, node). That emits EOpConvFloatToInt and correctly typed SPIR-V, so SPIRV-Cross writes the expected int(...).

Validation

I stood up an ANGLE/GLES build on Windows (-DGRAPHICS_API=OpenGLWindowsDevOnly) to get a local GLES gate, and it reproduces the Ubuntu error verbatim - same shader, same two source lines.

before after
D3D11, full sequential run 351/351 351/351 (unchanged)
ANGLE/GLES, the 11 affected tests Fatal::InvalidShader all 11 pass

The eleven are the whole prepass SSAO family - 287, 289, 293, 296, 299, 302, 304, 305, 306 - plus 176 "GUI Slate" and 177 "GUI Near Menu", which are still excluded on master but were hitting the same bug.

Still red

The three Win32 failures (321/322/323, the motion blur trio) are fixed by #1839, which is still awaiting review. They should go green once that lands and this branch rebases.

Out of scope, noted for later

On the ANGLE build test 363 "Screen Space Reflections 2" trips a different, pre-existing bgfx bug: FrameBufferGL::resolve issues glDrawBuffers(1, &GL_COLOR_ATTACHMENT0 + colorIdx), but GLES requires bufs[i] to be GL_NONE or GL_COLOR_ATTACHMENT0 + i, so any attachment past the first is GL_INVALID_OPERATION. Desktop GL takes the glDrawBuffer path instead, which has no such restriction, so this does not affect the Ubuntu jobs.

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
@bkaradzic-microsoft

Copy link
Copy Markdown
Member Author

The uniform fix worked - the Ubuntu jobs no longer hit Fatal::InvalidShader. They now get past shader compilation for test 287 and abort a little later, in the driver:

BGFX attr a_tangent: 2
BGFX instance data i_data0: 4
BGFX instance data i_data1: 3
BGFX instance data i_data2: 1
BGFX instance data i_data3: 0
LLVM ERROR: Cannot emit physreg copy instruction
--- BN: ABORT ---
SIGABRT raised.

All four Ubuntu jobs stop at byte-identical positions (159 completed comparisons, then this). The program being linked at the crash carries diffuseSampler / textureMask / i_data0-3, i.e. the particle shader - not the SSAO blur shader, and nothing to do with the integer-uniform path this PR changed. It is llvmpipe's LLVM backend failing to JIT that shader on the CI runner.

Since that is a Mesa/LLVM bug rather than anything Babylon Native emits, I have re-excluded just test 287 in e6c431e1, with the crash recorded in reason. The test passes on D3D11 and on ANGLE/GLES, so the exclusion is CI-runner-specific rather than a statement about the renderer.

The other eight tests enabled by the 9.21.2 bump stay enabled, and a full sequential ANGLE/GLES run confirms all of them now render: 289, 293, 296, 299, 302, 304, 305, 306 pass, with zero BGFX FATAL anywhere in the run.

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
@bkaradzic-microsoft

Copy link
Copy Markdown
Member Author

Correction to my earlier comment: I claimed the glDrawBuffers issue in FrameBufferGL::resolve was GLES-only and therefore could not affect the Ubuntu jobs. That is wrong. Babylon Native builds the OpenGL ES path on Linux - the job log shows glcontext_egl.cpp and eglBindAPI - so it takes the same glDrawBuffers branch that ANGLE does, not the desktop glDrawBuffer branch. The Ubuntu jobs do hit it, and that is where they now stop.

Progress so far on this branch:

run comparisons completed stopped at
b6bc6981 (before the fixes) 0 Fatal::InvalidShader, ssao2 blur, test 287
e5490d2f (current) 183 glDrawBuffers assert, FrameBufferGL::resolve

The bgfx code in question is:

const GLenum drawBuffer = GL_COLOR_ATTACHMENT0 + colorIdx;
GL_CHECK(glDrawBuffers(1, &drawBuffer) );

GLES requires bufs[i] to be either GL_NONE or GL_COLOR_ATTACHMENT0 + i, so any attachment past the first is GL_INVALID_OPERATION. Desktop GL uses glDrawBuffer just above, which has no such restriction, which is why this only bites the ES path.

I am reproducing the sequential run locally on ANGLE against this exact config to pin down which test triggers it - in per-test isolation every test in that range passes, so it looks like it depends on run order rather than on one scene. Will report back.

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
@bkaradzic-microsoft

Copy link
Copy Markdown
Member Author

Found it, and the branch should now be as green as it can get without #1839.

The glDrawBuffers assert is test 363, Screen Space Reflections 2

Isolating it took a moment because ranged runs kept passing - 280-360, 230-360, 150-360, 60-360, 0-360 all complete cleanly. That looked like a run-order dependency, but it was simpler than that: 0-360 runs 181 comparisons, the full run does 183 and then asserts, and 362-363 reproduces it on its own. The test is just past the end of every range I had been bisecting with.

It is a bgfx bug on the ES path:

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 raises GL_INVALID_OPERATION. Excluded with that reason; it passes on D3D11.

Enabled GUI Near Menu on OpenGL instead

While going through the config I noticed test 177 was excluded from OpenGL with this reason already recorded on master:

OpenGL: BGFX FATAL shader compile error in GUI fragment shader ('=' : cannot convert from 'highp float' to 'bool')

That is exactly the bug fixed here, one type over - a bool uniform widened to vec4 and narrowed back by shape alone. So the OpenGL exclusion is removed and the test now runs on every backend, which gives the uniform fix direct coverage in CI rather than only in my local ANGLE runs.

Where the branch stands

ran passed failed
ANGLE/GLES, full sequential 286 282 4
D3D11, full sequential 300 297 3

Zero asserts and zero BGFX FATAL on either. The three shared failures are the motion blur trio (321, 322, 323), which #1839 fixes; the fourth on ANGLE is an ANGLE-only pixel difference in MeshDebugPluginMaterial that does not reproduce on D3D11.

So the expected CI result is Win32 and Ubuntu both red on exactly those three tests, and green once #1839 lands and this rebases.

Summary of what changed on this branch for CI

  1. 2eaaf94d - restore the original basic type when narrowing widened uniforms. Fixes int and bool uniforms on OpenGL/GLES. Eleven tests go from Fatal::InvalidShader to passing on ANGLE.
  2. e6c431e1, e5490d2f - exclude the prepass SSAO family; llvmpipe on the runner aborts with LLVM ERROR: Cannot emit physreg copy instruction, a Mesa register allocator bug. They pass on D3D11 and ANGLE.
  3. 68508845 - enable GUI Near Menu on OpenGL, exclude Screen Space Reflections 2 for the bgfx glDrawBuffers bug above.

`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
@bkaradzic-microsoft

Copy link
Copy Markdown
Member Author

Root cause of the Win32_x64_D3D11 / Win32_x64_D3D11_Sanitizers one-hour timeouts

Both jobs have hit the 1 h job timeout on every run of this PR, including at b6bc6981 before any of the shader work. They are the two jobs that build the default JavaScript engine, Chakra — the V8, Hermes, JSI and QuickJS jobs on the same matrix are fine.

Reproduced locally with a Chakra build. It is deterministic and isolated:

Playground.exe --headless --test-index=23 app:///Scripts/validation_native.js

never returns, spinning ~2.9 cores and growing the heap by ~7 MB/s. Test 23 is "Glow layer and LODs", and the last thing bgfx logs is the glow blur shader (delta vec4 + textureSampler) — matching the CI logs exactly.

It is not the renderer

The scene renders fine standalone (5 000+ frames, no stall). What never completes is Scene.executeWhenReady, whose ready-poll re-arms itself as fast as it can — hence the CPU spin and the steady allocation. Probing the readiness chain each frame:

Chakra V8
scene.isReady() false forever true @ frame 200
glowLayer.isReady(subMesh) false forever true
drawWrapper.effect.isReady() true @ frame 50 true
post-processes ready true @ frame 50 true
_shadersLoaded false forever true
isLayerReady() false forever true

Everything is ready except _shadersLoaded, and the one promise that never settles is ThinGlowLayer._importShadersAsync() — it neither resolves nor rejects.

The actual bug: super inside an arrow function on Chakra

Chakra resolves super.x inside an arrow function nested in a class method to the derived class's own method instead of the base one:

class A { foo() { return "BASE"; } }
class B extends A {
    foo() {
        const s = Object.create(null, { foo: { get: () => super.foo } });
        return s.foo.call(this);
    }
}
new B().foo();
Chakra V8
resolved fn === B.prototype.foo true false
resolved fn === A.prototype.foo false true
calling through it Error: Out of stack space "BASE"

TypeScript emits precisely 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 — exactly where this PR's bump from 9.15.0 to 9.21.2 crosses. Bisected by swapping babylon.max.js: 9.15.0 passes, 9.16.0 hangs, and there are no patch releases in between.

Called synchronously it dies with Out of stack space. Called from a promise chain — which is what _importShadersAsync is — every level is a fresh microtask, so it recurses forever without overflowing the stack: never settles, never throws, burns CPU and heap. That is the runner signature.

Fix

The repo already has the remedy: Apps/scripts/downlevelNativeScripts.mjs, added in #1789 for this exact reason —

Babylon Native's Chakra engine consumes ES5-level script, so the bundle must be down-leveled before it runs.

TypeScript's ES5 emit rewrites super.x to _super.prototype.x and removes the arrow entirely, so the bug cannot be expressed. The script was only ever wired into getNightly, so every build that takes Babylon.js from npm — i.e. 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; the nightly path is untouched (getNightly.js still downlevels the files it refills from the CDN).

Validation

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 on V8 — no regression from the ES5 emit — and Chakra now matches it. The three remaining failures are the motion-blur trio (Thin instances + dynamic buffer resize, Instances + render self motion blur, Thin instances + render self motion blur) that #1839 fixes.

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