Skip to content

Removed all D3D12 exclusions from tests.#1665

Merged
bkaradzic-microsoft merged 3 commits into
BabylonJS:masterfrom
bkaradzic-microsoft:fix-d3d12-exclusions
Apr 20, 2026
Merged

Removed all D3D12 exclusions from tests.#1665
bkaradzic-microsoft merged 3 commits into
BabylonJS:masterfrom
bkaradzic-microsoft:fix-d3d12-exclusions

Conversation

@bkaradzic-microsoft

Copy link
Copy Markdown
Member

Re-enables the scissor test cases that were previously excluded on D3D12 due to an automatic mipmap generation issue in bgfx.

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

Re-enables the previously D3D12-excluded scissor visual tests by updating bgfx integration and adjusting framebuffer attachment resolve behavior to avoid newer bgfx validation asserts around auto-mip generation.

Changes:

  • Update bgfx.cmake FetchContent tag to a newer commit.
  • Adjust framebuffer attachment initialization to conditionally enable BGFX_RESOLVE_AUTO_GEN_MIPS only when the texture format supports mip autogen, and explicitly disable resolve for depth attachments.
  • Remove the D3D12 exclusions (and related comments) for scissor tests in the playground validation config.

Reviewed changes

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

File Description
Plugins/NativeEngine/Source/NativeEngine.cpp Guards auto-mip resolve flag based on caps to prevent bgfx validation asserts; explicitly disables resolve for depth attachment.
CMakeLists.txt Bumps bgfx.cmake dependency commit used by the build.
Apps/Playground/Scripts/config.json Re-enables scissor tests on D3D12 by removing exclusions.

@bghgary bghgary 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.

[Review by Copilot on behalf of @bghgary]

Nice fix, and the comment on the color attachment in CreateFrameBuffer captures the subtlety well. One concern: there are other Attachment::init(...) call sites using the short overload that rely on the same default-ignores-unsupported-formats behavior and will hit the new bgfx assert after this bump. They aren't covered by the scissor tests, so CI going green won't prove they're safe:

  • Polyfills/Canvas/Source/FrameBufferPool.cpp:65 — loops over a color (RGBA8) and a depth (D24S8) attachment, both with the default overload. The depth one is the concern: depth formats don't support auto-mipgen, so this should reliably trip the new assert once Canvas is exercised.
  • Plugins/NativeXr/Source/NativeXrImpl.cpp:285-286 — same pattern, color + depth, both relying on defaults.

Could we fix these in the same PR? The change is mechanical (query caps->formats[...] & BGFX_CAPS_FORMAT_TEXTURE_MIP_AUTOGEN for color, explicit BGFX_RESOLVE_NONE for depth), and leaving them for later means a known assert waiting to fire on the first Canvas/WebXR workload after the merge.

@bkaradzic-microsoft bkaradzic-microsoft enabled auto-merge (squash) April 18, 2026 04:31
@bkaradzic-microsoft bkaradzic-microsoft merged commit ff2e970 into BabylonJS:master Apr 20, 2026
26 checks passed
@bkaradzic-microsoft bkaradzic-microsoft deleted the fix-d3d12-exclusions branch April 23, 2026 00:24
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.

5 participants