NativeEngine: texture upload, cubemap loading, and readback improvements - #1808
Open
bkaradzic-microsoft wants to merge 8 commits into
Open
NativeEngine: texture upload, cubemap loading, and readback improvements#1808bkaradzic-microsoft wants to merge 8 commits into
bkaradzic-microsoft wants to merge 8 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends BabylonNative’s NativeEngine texture pipeline so Babylon.js texture upload/load/readback behaviors match WebGL more closely, including cubemap container loading, incremental uploads, cube-face readback, and raw 3D texture support.
Changes:
- Add
NativeEngine::updateTextureDataandupdateTextureDirectlyto support incremental texture uploads and Babylon.js loader “direct upload” sinks. - Enable single-file cubemap container loading (
.dds/.ktx/.ktx2) and compute diffuse-IBL spherical-harmonics from decoded top mips. - Add cube-face readback support to
readTexture, plus 3D raw texture creation/upload plumbing and shader-compiler texelFetch coordinate flip fixes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Plugins/ShaderCompiler/Source/ShaderCompilerTraversers.cpp | Moves texelFetch coordinate flipping into an AST post-visit rewrite, avoiding WEBMIN-stripped integer multiply paths. |
| Plugins/ShaderCompiler/Source/ShaderCompilerCommon.cpp | Turns ProcessSamplerFlip into an identity passthrough since flipping is handled in the AST. |
| Plugins/NativeEngine/Source/NativeEngine.h | Adds new NativeEngine entrypoints for incremental updates, 3D raw textures, and direct upload sink. |
| Plugins/NativeEngine/Source/NativeEngine.cpp | Implements cubemap container loading + SH computation, updateTextureData, updateTextureDirectly, raw 3D textures, and cube-face readback routing. |
| Core/Graphics/Source/Texture.cpp | Adds 3D texture create/update and tracks cube/3D flags on Texture objects. |
| Apps/Playground/Scripts/config.json | Un-excludes the Test updateTextureData playground test from automatic testing. |
bkaradzic-microsoft
pushed a commit
to bkaradzic-microsoft/BabylonNative
that referenced
this pull request
Jul 31, 2026
Three fixes from review on BabylonJS#1808: - UpdateTextureData: the bounds check allows `layer` up to 6*numLayers for cube textures (six faces per array layer), but the call site passed that value straight through as the bgfx *side* and hardcoded array layer 0. For a cube array that meant a side index above 5 and every update landing on layer 0. Decompose `layer` into (layer / 6, layer % 6) so it matches the range the bounds check actually admits. - ReadTexture: the face/layer index was forwarded to encoder->blit as srcZ with no upper bound, so an out-of-range value could drive an out-of-bounds read inside bgfx. Validate it against the texture's srcZ extent. Note this is deliberately not a flat 0-5 check: Babylon.js passes this same argument for 2D arrays as a slice index (see BaseTexture.readPixels, which takes the faceIndex branch for `isCube || is2DArray`), where values above 5 are legitimate. The bound is 6*numLayers for cube textures and numLayers otherwise, matching UpdateTextureData. - Texture::Create3D: drop a duplicated `m_is3D = false;` store left over from copy/paste before the correct `m_is3D = true;`. Revalidated: ran=301 passed=301 failed=0 (RelWithDebInfo, Win32, D3D11).
…armonics loadCubeTexture now accepts a single self-contained cubemap container (all six faces + mips), decoded via bimg::imageParse, and uploads sides 0-5 x mips. ComputeCubeSphericalPolynomial derives the diffuse-IBL spherical harmonics from the top-mip faces (port of CubeMapToSphericalPolynomialTools) and returns the polynomial coefficients to JS. This is done natively because the WebGL upload and cube-readback paths are unimplemented on native and .dds stores no SH. The 6 prefiltered-environment PBR validation tests this unblocks stay excluded here; they need the paired Babylon.js change to ship in the babylonjs dependency first. Pairs with BabylonJS/Babylon.js#18560 (native createCubeTexture dispatch for single-URL containers). Depends on a babylonjs dependency bump including it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
…ureData) updateTextureData previously threw "not implemented" on Native. Implement it so sub-rectangle texture updates work. - Add NativeEngine::UpdateTextureData: upload the requested sub-rectangle via bgfx::updateTexture2D (Texture::Update2D). Validates the JS-controlled rect against the mip extents, sizes the copy with bgfx::calcTextureSize (no bimg dependency, so it also works in no-image-loading builds), and mirrors the vertical flip the base texture upload applies so the sub-rect lines up on top-left-origin backends (e.g. D3D11). - Re-enable the "Test updateTextureData" validation test. Pairs with the Babylon.js change (engine.name = "Native" so name-gated WebGL _gl access skips Native, plus the updateTextureData override). CI stays red until a babylonjs npm with that change is published and the dependency bumped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Implement the native sink for Babylon's _uploadDataToTextureDirectly / _uploadCompressedDataToTextureDirectly so single-file container textures (.dds/.ktx/.ktx2, plus Basis/IES/HDR/EXR/TGA) load through the same JS texture loaders WebGL/WebGPU use. The loaders upload one (face, mip) at a time WebGL texImage2D-style; bgfx needs the whole texture allocated first, so the underlying texture is created lazily on the first upload. Validates JS-provided dimensions against maxTextureSize before uint16 narrowing and the payload size against bimg::imageGetSize, uses bgfx::copy for async-owned upload memory, and matches the existing loader flip conventions (skipping row-flips for compressed formats). Addresses BabylonJS#218 (paired with the Babylon.js single-file cubemap loader change). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tureCube A cube created with bgfx::createTextureCube must be updated with bgfx::updateTextureCube (per-face side index), not updateTexture2D. Add an IsCube() flag (set in Texture::CreateCube, cleared in Create2D/Attach) and, in NativeEngine::UpdateTextureData, branch cubes to Texture::UpdateCube(0, side, mip, ...). Widen the layer bounds check to 6*numLayers for cubes (the JS side passes the face index in the layer arg). This is the C++ half of the HDR createRawCubeTexture fix. The IBL tests it unblocks stay excluded here: they also need the Babylon.js-side half, which is not in the pinned babylonjs dependency. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
PrepareImage passed block-compressed / unsupported formats straight to
bimg::imageGenerateMips, which only supports RGBA8/RGBA32F and returns NULL
otherwise. The NULL image was then dereferenced in LoadTextureFromImage,
crashing with an access violation. Tests 250/251/252 ("PBR shader code
coverage 1/2/3", snippets #QI7TL3#63/64/65) load a 256x256 BC1 texture with
generateMips=true and hit this.
- PrepareImage: convert any non-RGBA8/RGBA32F format before imageGenerateMips
(float/high-precision -> RGBA32F, everything else incl. BC1/DXT1 -> RGBA8),
with a null-check on the imageConvert result.
- LoadTexture: throw (routes to onError) instead of dereferencing a null image.
The crash is eliminated on all three. They stay excluded here: they also load
a single-file environment cubemap, which needs the Babylon.js-side change that
is not in the pinned babylonjs dependency.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
readTexture ignored the cube face and always read srcZ=0, and the JS
_readTexturePixels threw for any cube faceIndex. As a result
readPixels(face) returned null and ConvertCubeMapToSphericalPolynomial
crashed with "Cannot read properties of null" for tests that compute
diffuse-IBL spherical harmonics from a dynamically rendered cube
("Realtime Filtering", "Refraction local cube map PBR").
- readTexture now accepts an optional faceIndex (info[9], -1 = plain 2D).
A cube-face read always routes through the blit path with srcZ = face
(bgfx::readTexture cannot address an individual cube face).
Both tests stay excluded here: they also require Babylon.js-side changes
that are not in the pinned babylonjs dependency.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Graphics::Texture gains Create3D/Update3D (bgfx createTexture3D/ updateTexture3D) and NativeEngine gains a loadRawTexture3D binding, giving Babylon.js createRawTexture3D/updateRawTexture3D real 3D volumes on Native. Fix the sampler3D shader compile that blocks HAL Lattice: the vertical texel-coordinate flip was applied by a preprocessor macro that forced every texelFetch coordinate through ivec2(...), so sampler3D fetches failed with 'no matching overloaded function'. Move that flip into the dimension-aware FlipSamplerCoordinates AST traverser (only 2-component integer coords are flipped; 3D/array left intact), cloning the sampler and lod operands so the injected textureSize() call does not alias the original texelFetch subtree (aliasing corrupted the AST and crashed unrelated async tests on dispose). The flip is built as ivec2(uv.x, textureSize(s, lod).y - 1 - uv.y), matching the expression the old macro expanded to. The tidier vector form uv * ivec2(1, -1) + ivec2(0, size.y - 1) must not be used: it emits SPIR-V OpIMul, which SPIRV-Cross drops entirely in the SPIRV_CROSS_WEBMIN configuration Babylon Native builds (the handler is compiled out to a release-mode no-op assert). The multiply then yields no HLSL/MSL expression and the whole shader fails to cross-compile with "Cannot resolve expression type" - which regressed "Gaussian Splatting Compressed ply SH", the only enabled test that texelFetches a usampler2D. Integer subtract and vector construction are both retained by that build. The texelFetch rewrite runs on post-visit because it references the coordinate subtree twice; rewriting on the way down would make the traverser descend into that subtree twice and double-flip any nested texture() call. HAL Lattice (idx 128) stays excluded: it additionally needs Babylon.js-side 3D texture support, which is not in the pinned babylonjs dependency. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Three fixes from review on BabylonJS#1808: - UpdateTextureData: the bounds check allows `layer` up to 6*numLayers for cube textures (six faces per array layer), but the call site passed that value straight through as the bgfx *side* and hardcoded array layer 0. For a cube array that meant a side index above 5 and every update landing on layer 0. Decompose `layer` into (layer / 6, layer % 6) so it matches the range the bounds check actually admits. - ReadTexture: the face/layer index was forwarded to encoder->blit as srcZ with no upper bound, so an out-of-range value could drive an out-of-bounds read inside bgfx. Validate it against the texture's srcZ extent. Note this is deliberately not a flat 0-5 check: Babylon.js passes this same argument for 2D arrays as a slice index (see BaseTexture.readPixels, which takes the faceIndex branch for `isCube || is2DArray`), where values above 5 are legitimate. The bound is 6*numLayers for cube textures and numLayers otherwise, matching UpdateTextureData. - Texture::Create3D: drop a duplicated `m_is3D = false;` store left over from copy/paste before the correct `m_is3D = true;`. Revalidated: ran=301 passed=301 failed=0 (RelWithDebInfo, Win32, D3D11).
bkaradzic-microsoft
force-pushed
the
pr/texture-upload-formats
branch
from
August 4, 2026 14:59
73b33cf to
5d8c2e1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fills in a set of gaps in
NativeEngine's texture upload / readback paths so that Babylon.js texture APIs that already work on WebGL behave the same on Native.Seven self-contained commits:
975b63d8.dds/.ktx/.ktx2cubemaps, including embedded spherical-harmonic coefficientsb1bc011fNativeEngine::updateTextureData43dedc29updateTextureDirectlytexture-loader sink9871bf8eUpdateTextureDatatobgfx::updateTextureCube138e457735f2cdadNativeEngine.readTextured4d8d438sampler3DtexelFetchcoordinate-flip fixEach commit builds and runs on its own; they're ordered so the plumbing lands before the callers.
Validation
Full Playground validation suite,
RelWithDebInfo, Win32, D3D11:No regressions against master's baseline of 300/300.
About
config.jsonThis PR un-excludes exactly one test —
Test updateTextureData— and that one is verified to pass against the stock npmbabylonjs9.15.0 thatApps/node_modulesresolves to.I want to flag this explicitly because it bit me: while developing this I had a locally-built Babylon.js fork in
Apps/node_modules(12.7 MBbabylon.max.js, same declared version9.15.0as the 8.6 MB npm build). Against that fork, 16 tests appeared to pass. Against stock npm, only 1 of the 16 actually does. The other 15 need Babylon.js-side changes that haven't landed yet:CDF renderer is not supported by the current engineTEXTURE_3D/FLOATtests → require the raw-3D-texture constants to be plumbed through the engine capsThose un-exclusions are deliberately not in this PR and will follow once the corresponding Babylon.js work is released.
Note for anyone touching the shader compiler
d4d8d438includes a fix inShaderCompilerTraversers.cppthat is worth calling out, because the failure mode is nasty and invisible in the common configuration.BabylonNative builds SPIRV-Cross with
SPIRV_CROSS_WEBMIN(see the rootCMakeLists.txt;BABYLON_NATIVE_DISABLE_WEBMINturns it off). In that configuration a number of opcodes — includingOpIMul— are compiled out toSPIRV_CROSS_INVALID_CALL(), which is a bareassert(false). UnderNDEBUGthat is a no-op: the instruction is visited, no result id is set, and the failure surfaces much later at the first consumer of that id asCannot resolve expression type.— and sinceSPIRV_CROSS_THROWis also stripped tothrow CompilerError("")under WEBMIN, the message you actually get is empty.Concretely: emitting
coord * ivec2(1, -1)from a traverser produces a silently broken HLSL/MSL/Vulkan shader. The fix here computes the flip asivec2(coord.x, textureSize(s, lod).y - 1 - coord.y), which only needsOpCompositeExtract/OpCompositeConstruct/OpISub— all of which WEBMIN retains. Because that formulation references the coordinate subtree twice,EOpTextureFetchhandling also moved fromEvPreVisittoEvPostVisitso the traverser doesn't descend into the duplicated subtree and double-flip nestedtexture()calls.Short version: don't emit integer multiply from a shader-compiler traverser. I'll file this upstream against SPIRV-Cross separately — a stripped opcode should fail loudly rather than emit a broken shader.