Skip to content

Add cubemap auto-expand polyfill for Playground (re-enables 7 PBR tests)#1710

Open
bkaradzic-microsoft wants to merge 2 commits into
BabylonJS:masterfrom
bkaradzic-microsoft:weekend/tpc-1577-cubemap-6-files
Open

Add cubemap auto-expand polyfill for Playground (re-enables 7 PBR tests)#1710
bkaradzic-microsoft wants to merge 2 commits into
BabylonJS:masterfrom
bkaradzic-microsoft:weekend/tpc-1577-cubemap-6-files

Conversation

@bkaradzic-microsoft
Copy link
Copy Markdown
Contributor

@bkaradzic-microsoft bkaradzic-microsoft commented May 18, 2026 •
edited
Loading

Adds a JS polyfill that patches BABYLON.NativeEngine.prototype.createCubeTexture so single-URL .dds/.ktx/.ktx2 cubemap loads (which BN does not currently support) transparently retry as .env. Babylon's CDN co-hosts both formats from the same source HDR, so the swap is invisible to the playground.

Changes:

  • New Apps/Playground/Scripts/cube_texture_polyfill.js - patches createCubeTexture; retries via self._loadFile; falls back to the original throw on 404.
  • Apps/Playground/CMakeLists.txt - adds cube_texture_polyfill.js to SCRIPTS list.
  • Apps/Playground/Shared/AppContext.cpp - loads cube_texture_polyfill.js after babylon.max.js (the polyfill needs BABYLON.NativeEngine to exist).
  • Apps/Playground/Scripts/config.json - re-enables 7 affected tests (idx 141, 172, 173, 246, 247, 248, 290 - PBR/cubemap heavy).

Note: Bridges the gap until Plugins/NativeEngine wires a proper loader registry path for cubemap formats. Remaining 35 of 42 affected tests either exceed pixel-diff threshold once they reach the renderer or hit unrelated secondary bugs (particles, PrePassRenderer caps, Chakra parse, etc.) - all already filed.

Landing context

This PR is one of 7 splits from the proven CI-green combined preview in draft PR #1702 (see #1702 for the full intended end-state and verified CI run 26044922430).

Note: the original split included an 8th PR (#1709, ES2020+ -> ES2019 syntax-repair polyfill for Chakra). It was closed in favour of investigating @babel/standalone properly (#1711).

Recommended landing order

Tier 1 - parallel-reviewable, no source conflicts:

  1. Fix ExternalTexture_OpenGL throw-stubs to avoid MSVC C4702 under /WX #1703 - ExternalTexture C4702 build fix
  2. Document accurate root cause for post-#1695 pixel-diff fallouts #1704 - config.json reason rewrites (5 entries)
  3. Document accurate root cause for 17 subtle pixel-diff tests #1705 - config.json reason rewrites (17 entries)

Tier 2 - sequential, each touches Apps/Playground/CMakeLists.txt SCRIPTS list + Apps/Playground/Shared/AppContext.cpp LoadScript order; rebase the next branch after the previous merges:

  1. Bump JsRuntimeHost for File/FileReader polyfill (re-enables 19 GLTF/OBJ tests) #1706 - File/Blob/FileReader polyfill (largest test impact: 19 re-enables)
  2. Add fetch() polyfill over XMLHttpRequest for Playground #1707 - fetch polyfill
  3. Add DOM globals polyfill + native AbortController for Playground #1708 - DOM globals + native AbortController + Android CMake link
  4. Add cubemap auto-expand polyfill for Playground (re-enables 7 PBR tests) #1710 - Cubemap auto-expand polyfill (loaded after babylon.max.js)

Reference policy reminder

Reference PNGs across all 7 PRs come from Babylon.js; never re-baked by BN. Combined diff: 0 PNGs.

@bkaradzic-microsoft
Add .env fallback for cubemap loads + re-enable 7 tests
6744fb6 
Babylon Native's NativeEngine.createCubeTexture override only handles
.env single-file cubemaps and 6-face arrays; .dds / .ktx / .ktx2 URLs
fall through to a throw ("Cannot load cubemap because 6 files were not
defined"). The loader-aware path used by the WebGL engine (texture
loader registry lookup) is bypassed entirely.

Add a JS-side polyfill that detects .dds / .ktx / .ktx2 single-URL
cubemap loads and retries with the .env extension. Babylon's CI
generates both .dds and .env from the same source HDR and uploads
them to the same CDN path, so the swap is transparent for the
Babylon-hosted environments these tests reference. On 404 (no .env
counterpart exists) the polyfill re-invokes the original code path,
preserving the existing throw semantics.

This unblocks 7 tests that were excluded because the throw aborted
the scene before any rendering could happen:

  idx 141 NMEGLTF
  idx 172 Anisotropic
  idx 173 Clear Coat
  idx 246 PBRMetallicRoughnessMaterial
  idx 247 PBRSpecularGlossinessMaterial
  idx 248 PBR
  idx 290 Prepass SSAO + depth of field

Strip excludeFromAutomaticTesting + reason from those 7 entries in
config.json. All 7 validate sub-threshold on Win32 V8 D3D11 Release
without --include-excluded after the strip (pixel diff range
308..2638, well under the 2.5% threshold).
Copilot AI review requested due to automatic review settings May 18, 2026 16:35
This was referenced May 18, 2026
Merged
Merged
Merged
Open
Open
Open
Closed
Draft
bkaradzic-microsoft added a commit that referenced this pull request May 21, 2026
@bkaradzic-microsoft
Document accurate root cause for 17 subtle pixel-diff tests (#1705)
f78015a 
Per-test PIL-composite triage of all 17 ``subtle pixel-diff`` tests.
None of them are deterministic-cosmetic (no re-bakes possible - all show
real visible regressions).

Updates the `reason` field in `Apps/Playground/Scripts/config.json` for
17 tests with accurate symptom descriptions, classifying into recurring
root-cause clusters:

- 9 GUI green->red color regressions (idx 160, 174, 175, 196, 197, 370,
402, 566, 587)
- 4 OpenPBR analytic-lights right-column red rendering (580, 584, 587,
592)
- 1 instanced billboard foliage red (169)
- 1 LineEdgesRenderer extra red lines (179)
- 1 Background blur red splotches (602)
- 1 Clip planes GUI sliders red (182)
- 1 Instanced Bones edge-AA (256, borderline)

**No source changes, no test re-enables, no PNGs.** Metadata-only
correction so the issue tracker reflects actionable root causes.
---

## Landing context

This PR is one of **7 splits** from the proven CI-green combined preview
in **draft PR #1702** (see
[#1702](#1702) for the
full intended end-state and verified CI run
[26044922430](https://github.com/BabylonJS/BabylonNative/actions/runs/26044922430)).

> Note: the original split included an 8th PR (#1709, ES2020+ -> ES2019
syntax-repair polyfill for Chakra). It was closed in favour of
investigating `@babel/standalone` properly (#1711).

### Recommended landing order

**Tier 1 - parallel-reviewable, no source conflicts:**
1. #1703 - ExternalTexture C4702 build fix
2. #1704 - config.json `reason` rewrites (5 entries)
3. #1705 - config.json `reason` rewrites (17 entries)

**Tier 2 - sequential, each touches `Apps/Playground/CMakeLists.txt`
SCRIPTS list + `Apps/Playground/Shared/AppContext.cpp` LoadScript order;
rebase the next branch after the previous merges:**

4. #1706 - File/Blob/FileReader polyfill (largest test impact: 19
re-enables)
5. #1707 - fetch polyfill
6. #1708 - DOM globals + native AbortController + Android CMake link
7. #1710 - Cubemap auto-expand polyfill (loaded after babylon.max.js)

### Reference policy reminder

Reference PNGs across all 7 PRs come from Babylon.js; never re-baked by
BN. Combined diff: **0 PNGs**.
bkaradzic-microsoft added a commit that referenced this pull request May 21, 2026
@bkaradzic-microsoft
Document accurate root cause for post-#1695 pixel-diff fallouts (#1704)
0f32173 
Per-test triage of 5 post-#1695 pixel-diff fallouts. Updates the
`reason` field in `Apps/Playground/Scripts/config.json` for 3 entries to
name the real BN rendering regression instead of generic "Pixel
comparison fails":

- idx 363 SSR2 - SSR not rendering wet floor.
- idx 369 Sprites Pixel Perfect - sprite alpha-blending broken.
- idx 395 soft-transparent-shadows - soft-shadow filter precision
degraded.

**No source changes, no test re-enables, no PNGs.** Metadata-only
correction so the issue tracker reflects actionable root causes for
follow-up engineering work.
---

## Landing context

This PR is one of **7 splits** from the proven CI-green combined preview
in **draft PR #1702** (see
[#1702](#1702) for the
full intended end-state and verified CI run
[26044922430](https://github.com/BabylonJS/BabylonNative/actions/runs/26044922430)).

> Note: the original split included an 8th PR (#1709, ES2020+ -> ES2019
syntax-repair polyfill for Chakra). It was closed in favour of
investigating `@babel/standalone` properly (#1711).

### Recommended landing order

**Tier 1 - parallel-reviewable, no source conflicts:**
1. #1703 - ExternalTexture C4702 build fix
2. #1704 - config.json `reason` rewrites (5 entries)
3. #1705 - config.json `reason` rewrites (17 entries)

**Tier 2 - sequential, each touches `Apps/Playground/CMakeLists.txt`
SCRIPTS list + `Apps/Playground/Shared/AppContext.cpp` LoadScript order;
rebase the next branch after the previous merges:**

4. #1706 - File/Blob/FileReader polyfill (largest test impact: 19
re-enables)
5. #1707 - fetch polyfill
6. #1708 - DOM globals + native AbortController + Android CMake link
7. #1710 - Cubemap auto-expand polyfill (loaded after babylon.max.js)

### Reference policy reminder

Reference PNGs across all 7 PRs come from Babylon.js; never re-baked by
BN. Combined diff: **0 PNGs**.
@bkaradzic-microsoft
PR BabylonJS#1710: replace cube_texture_polyfill.js with native CubeT…
c29e715 
…exture polyfill

Per review: this polyfill should be available to every Babylon Native
consumer using Chakra, not just the Playground app. Move it from a
Playground-only LoadScript() of cube_texture_polyfill.js into a proper
C++ polyfill library under Polyfills/ that any embedder can link.

The polyfill patches BABYLON.NativeEngine.prototype.createCubeTexture
to transparently retry single-URL .dds / .ktx / .ktx2 cubemap loads
(which BN does not currently handle) as their .env counterpart that
Babylon's CDN co-hosts at the same path. On 404 it falls through to
the original (which throws), preserving existing error semantics.

Implementation pattern follows the existing ErrorCause / StringTrim
native polyfills:

- New Polyfills/CubeTexture/ library.
- Public header Babylon/Polyfills/CubeTexture.h declares
  void BABYLON_API Initialize(Napi::Env env).
- Source/CubeTexture.cpp embeds the polyfill IIFE verbatim from the
  prior cube_texture_polyfill.js (same semantics, same idempotency
  marker __cubeTexturePolyfillInstalled) and dispatches it via the
  free function Napi::Eval(env, source, url). Napi::Eval is declared
  by every engine-specific <napi/env.h> across both N-API trees
  (Chakra / V8 / JavaScriptCore in Core/Node-API/Include/Engine/<X>/
  and JSI in Core/Node-API-JSI/Include/napi/) so the same source
  builds uniformly across all backends; no __has_include switch
  needed for this entry point.

Wiring:
- Top-level CMakeLists.txt: add BABYLON_NATIVE_POLYFILL_CUBETEXTURE
  option (default ON).
- Polyfills/CMakeLists.txt: guarded add_subdirectory(CubeTexture).
- Apps/Playground/CMakeLists.txt and
  Apps/Playground/Android/BabylonNative/CMakeLists.txt: PRIVATE link
  CubeTexture into the Playground / BabylonNativeJNI executables so
  AppContext.cpp can use the published header on every platform.
- Apps/Playground/Shared/AppContext.cpp: include the new header and
  replace
    LoadScript("app:///Scripts/cube_texture_polyfill.js");
  with
    Dispatch([](Napi::Env env) {
        Babylon::Polyfills::CubeTexture::Initialize(env);
    });
  scheduled right after LoadScript("babylon.max.js"). ScriptLoader's
  Dispatch and LoadScript queue onto the same ordered task chain
  (see jsruntimehost ScriptLoader::Impl), so Initialize is guaranteed
  to run after babylon.max.js finishes evaluating - same timing the
  prior LoadScript provided.

Delete Apps/Playground/Scripts/cube_texture_polyfill.js (no longer
shipped).

Verified locally on Chakra build (build/win32 RelWithDebInfo,
headless) - all 7 cubemap-affected tests re-enabled by this PR pass:

  Clear Coat                       diff=519 px
  PBR                              diff=482 px (12 sub-tests skipped, expected)
  Anisotropic                      diff=308 px
  PBRMetallicRoughnessMaterial     diff=482 px
  NMEGLTF                          diff=631 px
  PBRSpecularGlossinessMaterial    diff=522 px

Console log confirms the polyfill ran:
  [CubeTexture polyfill] NativeEngine.createCubeTexture patched with .env fallback

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bkaradzic-microsoft added a commit that referenced this pull request May 28, 2026
@bkaradzic-microsoft
Fix ExternalTexture_OpenGL throw-stubs to avoid MSVC C4702 under /WX (#…
c63332f 
…1703)

Fixes MSVC C4702 (unreachable code) under `/WX` in
`Plugins/ExternalTexture/Source/ExternalTexture_OpenGL.cpp` so the
OpenGL TU builds cleanly without the project-wide `/wd4702` workaround.

## What changed

`Plugins/ExternalTexture/Source/ExternalTexture_OpenGL.cpp` keeps the
unimplemented `Impl` stubs as `throw std::runtime_error{"not
implemented"}` (so callers get an unambiguous diagnostic matching every
other backend's unimplemented path) and wraps the `#include
"ExternalTexture_Shared.h"` line in `#pragma warning(push)/(disable:
4702)/(pop)`. The shared dispatchers instantiated by that include
unconditionally call the stubs, so MSVC's flow analysis flags the
dispatcher's post-call code as unreachable; the pragma suppresses C4702
only for the dispatcher code instantiated in this translation unit. Any
other code in this file is still subject to `/WX` C4702 enforcement.

`Plugins/ExternalTexture/CMakeLists.txt` - drop the `/wd4702` target
compile option, which previously silenced C4702 across the whole target.

## Alternatives considered

- **`[[noreturn]]` on the stubs**: MSVC propagates "never returns"
through the shared dispatcher, which flags *more* post-call statements
as unreachable. Tried and rejected (it made the warning worse).
- **TU-wide `/wd4702` via `target_compile_options`**: silences any
future legitimate C4702 elsewhere in the same TU, defeating `/WX`. The
current localized `#pragma warning(push/pop)` block keeps the rest of
the TU under `/WX` enforcement.
- **Replacing the throws with inert return-default stubs**: changes
product behaviour (callers would silently receive a null texture instead
of a clear "not implemented" error) to work around a compiler warning.
Rejected on principle.

## Verified

Clean under Debug + Release + RelWithDebInfo on OpenGL (`win32-gl`,
`GRAPHICS_API=OpenGLWindowsDevOnly`) and D3D11 (`win32`).

---

## Landing context

This PR is one of **7 splits** from the proven CI-green combined preview
in **draft PR #1702** (see
[#1702](#1702) for the
full intended end-state and verified CI run
[26044922430](https://github.com/BabylonJS/BabylonNative/actions/runs/26044922430)).

> Note: the original split included an 8th PR (#1709, ES2020+ -> ES2019
syntax-repair polyfill for Chakra). It was closed in favour of
investigating `@babel/standalone` properly (#1711).

### Recommended landing order

**Tier 1 - parallel-reviewable, no source conflicts:**
1. #1703 - ExternalTexture C4702 build fix
2. #1704 - config.json `reason` rewrites (5 entries)
3. #1705 - config.json `reason` rewrites (17 entries)

**Tier 2 - sequential, each touches `Apps/Playground/CMakeLists.txt`
SCRIPTS list + `Apps/Playground/Shared/AppContext.cpp` LoadScript order;
rebase the next branch after the previous merges:**

4. #1706 - File/Blob/FileReader polyfill (largest test impact: 19
re-enables)
5. #1707 - fetch polyfill
6. #1708 - DOM globals + native AbortController + Android CMake link
7. #1710 - Cubemap auto-expand polyfill (loaded after babylon.max.js)

### Reference policy reminder

Reference PNGs across all 7 PRs come from Babylon.js; never re-baked by
BN. Combined diff: **0 PNGs**.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@bkaradzic-microsoft bkaradzic-microsoft enabled auto-merge (squash) May 29, 2026 00:35
bghgary
bghgary requested changes May 29, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@bghgary bghgary left a comment

Choose a reason for hiding this comment

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

[Reviewed by Copilot on behalf of @bghgary]

Concerns inline. Short version: polyfill silently substitutes content and masks the real gap (no .dds/.ktx loader in BN). Let's discuss offline.

Comment thread Polyfills/CubeTexture/Source/CubeTexture.cpp
@@ -0,0 +1,134 @@
#include <Babylon/Polyfills/CubeTexture.h>
Copy link
Copy Markdown
Contributor

@bghgary bghgary May 29, 2026

Choose a reason for hiding this comment

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

Three layered concerns:

  1. Silent substitution. PG asks for .dds, we quietly load the .env co-located by Babylon's CDN. Tests "pass" but they're rendering a different file (different format, mip pyramid, precision). If BJS's .dds path regresses, we don't catch it.
  2. CDN-specific. The fallback only works for assets where someone happens to co-host .env. Babylon's playground CDN does; arbitrary BN consumers don't. Shipping this in Polyfills/ advertises a benefit non-Babylon consumers won't get.
  3. Wrong layer. This masks a real gap — BN's NativeEngine doesn't dispatch .dds/.ktx/.ktx2 cubemap loading even though bimg already supports the formats. The proper fix lives in Plugins/NativeEngine.

Let's discuss offline before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bghgary bghgary bghgary requested changes

Copilot code review Copilot Awaiting requested review from Copilot

@CedricGuillemet CedricGuillemet Awaiting requested review from CedricGuillemet

@ryantrem ryantrem Awaiting requested review from ryantrem

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bkaradzic-microsoft @bghgary