Use supported backing storage for D24 render targets - #1874
Open
bkaradzic-microsoft wants to merge 3 commits into
Open
bkaradzic-microsoft wants to merge 3 commits into
bkaradzic-microsoft wants to merge 3 commits into
Conversation
Validate InitializeTexture's format and effective creation flags before allocating. Keep supported D24 requests unchanged; use D24S8 backing only when a D24 render target is unsupported and the packed format is supported. Reject invalid or unsupported requests without replacing an existing texture. Add native regressions for backing formats, single-sampled framebuffer attachment, multisample allocation flags, ordinary color textures, and JavaScript errors. No dependency pins or visual fixtures change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Reject non-integral and non-finite format values before converting them to enum values.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR adds texture validation and D24-to-D24S8 fallback for supported render targets. One moderate issue remains: Uint32Value() can coerce invalid numeric inputs into valid enum values.
Changes:
- Validate formats and effective texture flags.
- Add D24S8 fallback for render targets.
- Add native regression tests and CMake registration.
File summaries
| File | Description |
|---|---|
Plugins/NativeEngine/Source/NativeEngine.cpp |
Implements validation and backing-format fallback. |
Apps/UnitTests/Source/Tests.NativeEngine.TextureFormats.cpp |
Adds texture allocation and rejection tests. |
Apps/UnitTests/CMakeLists.txt |
Registers the new test source. |
Review details
- Files reviewed: 3/3 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.
Reject fractional, non-finite, negative, and out-of-range numbers before converting a texture format to an enum. Cover values that previously wrapped or truncated to valid formats and replaced an existing texture. Use Napi::Eval instead of the non-JSI Env::RunScript extension. Compare Error::Message rather than a stack string, since JavaScriptCore stacks do not include the error message. Preserve both in unexpected-error output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
Contributor
There was a problem hiding this comment.
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 3 out of 3 changed files in this pull request and generated 3 comments.
bkaradzic-microsoft
requested review from
CedricGuillemet,
bghgary and
ryantrem
September 11, 2026 23:02
Report the original numeric value and the finite-integer enum range while preserving the existing Invalid texture format prefix. Add a regression for the NaN diagnostic alongside allocation-preservation coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60c2ec68-6de1-445d-9fc9-b699db737eae
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.
Summary
initializeTexturepasses D24 directly to bgfx even on backends that only support 24-bit depth using D24S8 storage. Unlike the generated-depth path increateFrameBuffer, it does not check format support first. An explicit D24 render-target request consequently fails during allocation on D3D11.BLIT_DSTflag added byTexture::Create2D.The fallback preserves 24-bit depth precision; this changes storage selection, not framebuffer depth/stencil state. Independently based on upstream
2caf5447, with its existing dependency pins. No visual references, tolerances, exclusions, or dependency versions change.Validation
Built Release UnitTests using the stock dependency revisions on Windows:
JavaScript.AllThe native regressions inspect the allocated handle and actual backing format, attach the single-sampled depth texture to a framebuffer, verify 1x/4x allocation flags, preserve ordinary RGBA8 allocation behavior, and check that invalid/unsupported requests leave an existing texture intact.
Negative control: the single-sampled D24 allocation regression fails against the unmodified upstream implementation on D3D11, while the ordinary RGBA8 control passes. It passes after the fix.
Review follow-up: reproduced
ToUint32coercion accepting a wrapped/fractional format and replacing an existing texture, then added non-coercing validation and regressions for those inputs, NaN, and infinities.Initial CI also exposed two test-harness portability issues:
Env::RunScriptis unavailable with JSI, and JavaScriptCore stack strings omit the error message. The harness now uses the sharedNapi::EvalAPI and assertsError::Message()directly. The JSI build and runtime coverage pass locally; cross-platform CI is rerunning.The 4x case verifies allocation and flags only: multisampled sampled-depth framebuffer support requires additional bgfx flags and is unchanged here. This PR does not claim complete FrameGraph visual recovery on stock dependencies; the original shotgun scenes include other independently tracked fixes.