Fix ExternalTexture_OpenGL throw-stubs to avoid MSVC C4702 under /WX by bkaradzic-microsoft · Pull Request #1703 · BabylonJS/BabylonNative

bkaradzic-microsoft · 2026-05-18T16:35:16Z

Replaces throw "not implemented" stubs in Plugins/ExternalTexture/Source/ExternalTexture_OpenGL.cpp with inert default-value returns. The dispatcher in ExternalTexture_Shared.h no longer trips MSVC C4702 (unreachable code) under /WX, so the /wd4702 workaround can be removed from Plugins/ExternalTexture/CMakeLists.txt.

Changes:

Plugins/ExternalTexture/Source/ExternalTexture_OpenGL.cpp - stubs return default-constructed values.
Plugins/ExternalTexture/CMakeLists.txt - drop /wd4702.

Verified: clean under Debug + Release + RelWithDebInfo on OpenGL and D3D11.

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:

Fix ExternalTexture_OpenGL throw-stubs to avoid MSVC C4702 under /WX #1703 - ExternalTexture C4702 build fix
Document accurate root cause for post-#1695 pixel-diff fallouts #1704 - config.json reason rewrites (5 entries)
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:

Add File/Blob/FileReader polyfill for Playground (re-enables 19 GLTF tests) #1706 - File/Blob/FileReader polyfill (largest test impact: 19 re-enables)
Add fetch() polyfill over XMLHttpRequest for Playground #1707 - fetch polyfill
Add DOM globals polyfill + native AbortController for Playground #1708 - DOM globals + native AbortController + Android CMake link
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.

…tubs The OpenGL backend's ExternalTexture::Impl methods (GetInfo, Set, Get) unconditionally threw std::runtime_error{"not implemented"}, but these methods are called unconditionally from the dispatch code in ExternalTexture_Shared.h (included after the class definition). MSVC's flow analysis inlined the throw and flagged the post-call code as unreachable, producing C4702 errors under /WX on the OpenGLWindowsDevOnly build configuration. A previous change (BabylonJS#1695) masked the warning with a `target_compile_options(ExternalTexture PRIVATE /wd4702)` gated on GRAPHICS_API=OpenGL+MSVC. This silenced the diagnostic but hid any other legitimate C4702 introduced in the same translation unit going forward. Attempts to fix this via `[[noreturn]]` on the stub methods made things worse - MSVC propagated the "never returns" attribute through the dispatch code, flagging MORE post-call statements as unreachable. This change instead replaces the throw-stubs with inert no-op stubs that return default-constructed values. The dispatch code in ExternalTexture_Shared.h now sees the calls as returning normally, which eliminates the unreachable-code paths entirely. The /wd4702 workaround in CMakeLists.txt is removed. Functional impact: Calling an ExternalTexture API on the OpenGL backend (which is not shipped, only used for the OpenGLWindowsDevOnly developer build) now yields an inert/null texture rather than a thrown exception. The OpenGL backend has never supported ExternalTexture, no tests exercise it on OpenGL, and the Playground does not use it - so this behavioural change has no test impact. Verified clean build under Debug + Release + RelWithDebInfo of ExternalTexture, full Playground build under OpenGL and D3D11 backends. Smoke test passes on D3D11 Release.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Replaces unimplemented throw stubs in the OpenGL ExternalTexture backend with default-value returns to silence MSVC C4702 (unreachable code) warnings under /WX, and removes the corresponding /wd4702 workaround.

Changes:

Replace throw std::runtime_error{"not implemented"} with default returns / empty bodies in ExternalTexture_OpenGL.cpp.
Remove the OpenGL+MSVC-specific /wd4702 compile option from CMakeLists.txt.
Add an explanatory comment describing the rationale for the stub behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
Plugins/ExternalTexture/Source/ExternalTexture_OpenGL.cpp	Convert throwing stubs into inert default-returning stubs and add an explanatory comment.
Plugins/ExternalTexture/CMakeLists.txt	Remove the now-unneeded `/wd4702` MSVC workaround for the OpenGL build.

bkaradzic-microsoft · 2026-05-18T17:11:41Z

+            return Graphics::TextureT{};
        }

    private:
        static void GetInfo(Graphics::TextureT, std::optional<Graphics::TextureFormatT>, Info&)
        {
-            throw std::runtime_error{"not implemented"};
        }

        void Set(Graphics::TextureT)
        {
-            throw std::runtime_error{"not implemented"};
        }


Addressed by 834c99f: the throws are restored, so observable behavior matches the original "not implemented" exception path. The only thing that changes is how MSVC's C4702 is silenced (local pragma in this TU instead of returning early).

bkaradzic-microsoft · 2026-05-18T17:11:45Z

+    // The OpenGL backend does not implement ExternalTexture. The Impl methods
+    // below return default-constructed values rather than throwing so that the
+    // shared dispatchers in ExternalTexture_Shared.h (which is included after
+    // this class definition) don't produce unreachable-code paths that MSVC
+    // flags as C4702 under /WX. Any caller that actually invokes an
+    // ExternalTexture API on OpenGL will receive an inert/null texture and
+    // get a graphics-level error downstream.


Addressed by 834c99f: kept the throw and silenced C4702 locally with #pragma warning(push)/disable: 4702/pop around the ExternalTexture_Shared.h include. The rationale (and why [[noreturn]] / inert-return alternatives were rejected) is in the block comment at the top of the file.

…agma Per code review: working around a compiler warning by changing runtime behavior is undesirable. Restore the original "not implemented" throws in the OpenGL backend's ExternalTexture stubs and instead suppress C4702 only around the #include of ExternalTexture_Shared.h, which is where the unreachable-code paths are instantiated. Compared to the previous approach (returning default-constructed values from the stubs): - Callers now get a clear "not implemented" diagnostic identical to every other unimplemented backend path, instead of silently receiving a null texture. - The warning suppression is scoped to a few lines of source rather than the entire translation unit, so future legitimate C4702 in this file still surfaces under /WX. Compared to the original TU-wide /wd4702 in CMakeLists.txt (PR BabylonJS#1695): - Still removes the CMake-level workaround. - Suppression is now visible at the source site that triggers it, with comment explaining the rationale and the alternatives that were rejected ([[noreturn]] propagating through the shared dispatcher; replacing throws with inert returns changing product behavior). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

ryantrem

The PR description seems out of date with the changes. Is it just moving the warning suppression into the code to make it more restrictive (only part of the file, not the whole file)?

bghgary

[Reviewed by Copilot on behalf of @bghgary]

LGTM barring issues already mentioned, plus one inline comment.

bghgary · 2026-05-21T22:16:17Z

+    //   - 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.


This comment block is PR rationale (alternatives considered, why they were rejected) — that belongs in the PR description, not in the source. A future reader just needs to know why the pragma exists. Something like "Suppress C4702 for the shared dispatcher — Impl stubs always throw, making post-call code unreachable" is enough.

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**.

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**.

Copilot AI review requested due to automatic review settings May 18, 2026 16:35

Copilot AI reviewed May 18, 2026

View reviewed changes

bkaradzic-microsoft requested review from CedricGuillemet, bghgary and ryantrem May 18, 2026 16:42

bkaradzic-microsoft force-pushed the weekend/tpc-1652-externaltexture-c4702 branch from 1d286ee to 834c99f Compare May 18, 2026 16:51

Copilot started reviewing on behalf of bkaradzic-microsoft May 18, 2026 17:47 View session

ryantrem reviewed May 18, 2026

View reviewed changes

bghgary approved these changes May 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ExternalTexture_OpenGL throw-stubs to avoid MSVC C4702 under /WX#1703

Fix ExternalTexture_OpenGL throw-stubs to avoid MSVC C4702 under /WX#1703
bkaradzic-microsoft wants to merge 2 commits into
BabylonJS:masterfrom
bkaradzic-microsoft:weekend/tpc-1652-externaltexture-c4702

bkaradzic-microsoft commented May 18, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

bkaradzic-microsoft May 18, 2026

Uh oh!

Uh oh!

bkaradzic-microsoft May 18, 2026

Uh oh!

ryantrem left a comment

Uh oh!

bghgary left a comment

Uh oh!

bghgary May 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

bkaradzic-microsoft commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Verified: clean under Debug + Release + RelWithDebInfo on OpenGL and D3D11.

Landing context

Recommended landing order

Reference policy reminder

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

bkaradzic-microsoft May 18, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bkaradzic-microsoft May 18, 2026

Choose a reason for hiding this comment

Uh oh!

ryantrem left a comment

Choose a reason for hiding this comment

Uh oh!

bghgary left a comment

Choose a reason for hiding this comment

Uh oh!

bghgary May 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bkaradzic-microsoft commented May 18, 2026 •

edited

Loading