Skip to content

Honor native cube render-target allocation and face selection - #1885

Open
bkaradzic-microsoft wants to merge 5 commits into
BabylonJS:masterfrom
bkaradzic-microsoft:pr/native-cube-render-targets
Open

Honor native cube render-target allocation and face selection#1885
bkaradzic-microsoft wants to merge 5 commits into
BabylonJS:masterfrom
bkaradzic-microsoft:pr/native-cube-render-targets

Conversation

@bkaradzic-microsoft

@bkaradzic-microsoft bkaradzic-microsoft commented Sep 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

  • Honor the optional isCube argument in initializeTexture, allocating a cube rather than a 2D texture.
  • Thread the requested cube face from createFrameBuffer into the shared attachment implementation. Keep the default layer at zero for ordinary 2D targets and MRTs.
  • Reject cube faces outside 0-5 before reaching bgfx validation.
  • Add a native GPU regression that clears all six faces to distinct colors and checks every RGBA byte after copying the faces into readback storage. Also cover legacy 2D calls and invalid face rejection.

Independence

Rebased onto master (2a9dc944), independently of the other rendering PRs. Published Babylon.js 9.21.2 already sends the cube and face arguments; Native currently ignores them. No Babylon.js, protocol, dependency-pin, fixture, or tolerance changes.

This is deliberately limited to cube allocation and face attachment. It does not include newer volume/mip attachment APIs or claim to fix point-light projection/orientation issues in JavaScript.

Validation

Windows x64 / D3D11 / Chakra / RelWithDebInfo, with pinned upstream dependencies: the six-face GPU regression and legacy 2D controls pass.

Copilot AI lite review requested due to automatic review settings September 17, 2026 20:13
Copilot AI reviewed Sep 17, 2026
View reviewed changes

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.

🟡 Changes recommended

The new GPU test does not honor the existing SKIP_RENDER_TESTS configuration used for D3D12.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates native texture and framebuffer handling to support cube render targets and face selection.

Changes:

  • Honors isCube during texture initialization.
  • Passes and validates cube face layers.
  • Adds GPU regression coverage and CMake registration.
File summaries
File Description
NativeEngine.h Extends framebuffer implementation signature with layer support.
NativeEngine.cpp Allocates cube textures and attaches selected faces.
Tests.NativeEngine.CubeRenderTargets.cpp Adds cube-face clearing and readback tests.
CMakeLists.txt Registers the new test source.
Review details
  • Files reviewed: 4/4 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 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.NativeEngine.CubeRenderTargets.cpp Outdated
@bkaradzic-microsoft
bkaradzic-microsoft requested review from CedricGuillemet, bghgary and ryantrem and a balanced review from Copilot September 18, 2026 01:26
Copilot AI reviewed Sep 18, 2026
View reviewed changes

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.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

Comment thread Plugins/NativeEngine/Source/NativeEngine.cpp
Comment thread Plugins/NativeEngine/Source/NativeEngine.cpp Outdated
Comment thread Plugins/NativeEngine/Source/NativeEngine.cpp Outdated
Comment thread Apps/UnitTests/Source/Tests.NativeEngine.CubeRenderTargets.cpp
Comment thread Apps/UnitTests/Source/Tests.NativeEngine.CubeRenderTargets.cpp
Comment thread Apps/UnitTests/Source/Tests.NativeEngine.CubeRenderTargets.cpp
Comment thread Plugins/NativeEngine/Source/NativeEngine.cpp
@bkaradzic-microsoft

bkaradzic-microsoft commented Sep 18, 2026

Copy link
Copy Markdown
Member Author

CI is currently blocked by an inconsistent GitHub mergeability result, not a reproduced source conflict.

  • GitHub reports CONFLICTING/dirty, and the Nightly CI check says it skipped the branch because of conflicts.
  • The base is still bb01e283. GitHub's compare API reports 7cf322c5 as three commits ahead and zero behind that base; local merge-base --is-ancestor agrees.
  • git merge-tree --write-tree bb01e283 7cf322c5 succeeds and produces exactly the validated head tree (c3bffc03b128c358bf57b0daac8530c8739487f9).
  • A same-base metadata refresh did not clear the result. I then pushed one empty refresh commit, 969d0948, whose tree is identical to 7cf322c5; GitHub still reports a conflict and has not started an Actions run for it.

The cube validation changes already pass repeated D3D11/Chakra and Vulkan/JSC runs. No code, dependency, fixture, or tolerance was changed for the refresh. I am leaving the PR open on the same base and recording the hosted mergeability/CI blocker rather than repeatedly rewriting or republishing the branch.

@bkaradzic-microsoft
bkaradzic-microsoft force-pushed the pr/native-cube-render-targets branch from 969d094 to 0ccd313 Compare September 18, 2026 14:30
@bkaradzic-microsoft

bkaradzic-microsoft commented Sep 18, 2026

Copy link
Copy Markdown
Member Author

Fixed the post-rebase Windows sanitizer-job failure in fd1b972. This was a scene-construction race, not a sanitizer finding or cube-render-target failure: #GRQHVV#134 returned its scene before the mesh-import continuation created the spheres/materials, so validation captured only the clear color.

The existing fixture replace hook now returns the mesh-loading promise so validation waits for scene construction. References, tolerances, frame budgets, and coverage are unchanged. Replacement CI: https://github.com/BabylonJS/BabylonNative/actions/runs/35365543081

bkaradzic-microsoft and others added 5 commits September 18, 2026 12:37
@bkaradzic-microsoft
Honor native cube render-target allocation and face selection
70f3215 
Consume the isCube and face arguments already supplied by released
Babylon.js. Keep 2D and MRT defaults unchanged and reject out-of-range
cube faces before bgfx attachment validation.

Cover independent colors on all six faces through GPU readback, legacy
2D defaults, and invalid face rejection.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
@bkaradzic-microsoft
Honor the render-test skip for cube framebuffer readback
c70a90a 
Skip the GPU regression when SKIP_RENDER_TESTS is set, matching the
existing D3D12 test configuration while retaining supported backend coverage.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
@bkaradzic-microsoft
Validate cube dimensions and framebuffer layers before narrowing
0c53dd8 
Reject unequal cube dimensions without comparing truncated sizes. Require
integer cube faces in [0, 5] and layer zero for 2D or depth-only targets,
including rejection of fractional, non-finite and wrapping numeric inputs.

Extend the six-face readback regression with invalid-input coverage and a
valid default depth-only framebuffer control.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
@bkaradzic-microsoft
Refresh stale GitHub mergeability for cube render-target CI
25ba3cb 
GitHub reports this branch as conflicting despite its compare API reporting
three commits ahead and zero behind the unchanged bb01e28 base. A local
merge-tree produces exactly the current head tree without conflicts. The
same-base PR metadata refresh did not restore CI.

Republish an unchanged tree under a new head to trigger mergeability and CI
recomputation without changing code, rewriting history, or altering the base.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
@bkaradzic-microsoft
Await mesh loading in the OpenPBR validation fixture
97a7e32 
The analytic-lights scene returns before ImportMeshAsync's continuation
creates its spheres and materials, allowing validation to capture only
the clear color. Use the existing fixture replacement to return that
promise and wait for scene construction before rendering.

Preserve the reference image, frame budget, thresholds, and test coverage.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 48fa7ec8-0cf6-48a0-b836-10b7ace38c1a
@bkaradzic-microsoft
bkaradzic-microsoft force-pushed the pr/native-cube-render-targets branch from fd1b972 to 97a7e32 Compare September 18, 2026 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@CedricGuillemet CedricGuillemet Awaiting requested review from CedricGuillemet
@ryantrem ryantrem Awaiting requested review from ryantrem
@bghgary bghgary Awaiting requested review from bghgary

At least 1 approving review is required 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