NativeEngine: support standalone sampleable depth/stencil textures#1738
Draft
bkaradzic-microsoft wants to merge 2 commits into
Draft
NativeEngine: support standalone sampleable depth/stencil textures#1738bkaradzic-microsoft wants to merge 2 commits into
bkaradzic-microsoft wants to merge 2 commits into
Conversation
Babylon's NativeEngine._createDepthStencilTexture requests a standalone,
sampleable depth/stencil texture by calling createFrameBuffer with a
freshly-created (and therefore uninitialized) color texture whose bgfx
handle is still kInvalidHandle. CreateFrameBuffer only guarded against a
null texture, so it attached the invalid handle as a color target. bgfx
framebuffer validation then rejected the attachment ("Invalid texture
attachment") and threw, aborting the entire headless Playground sweep.
Detect this request (non-null texture with an invalid bgfx handle) and:
- skip attaching the invalid color handle (depth-only framebuffer),
- allocate the depth attachment as a readable BGFX_TEXTURE_RT so it can
be sampled, and
- alias the framebuffer's depth attachment back into the caller-supplied
texture (ownsHandle=false; the framebuffer owns the handle) so Babylon
can sample it (e.g. fluid rendering's depth copy).
This removes the whole-sweep abort and makes the depth/stencil texture
sampleable per Babylon's contract.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the NativeEngine framebuffer creation path to correctly handle Babylon’s request for a standalone, sampleable depth/stencil texture by interpreting an incoming non-null Graphics::Texture* with an invalid bgfx handle as a “depth-only framebuffer” request and then aliasing the created depth attachment back onto the provided texture for sampling.
Changes:
- Detect “standalone depth/stencil texture” requests (non-null texture with invalid bgfx handle) and avoid attaching the invalid color target.
- Create the depth attachment as readable (
BGFX_TEXTURE_RT) for the standalone depth/stencil texture case, while keeping existing write-only behavior for normal render-target depth attachments. - Alias the framebuffer’s depth attachment back into the caller-provided
Graphics::TexturewithownsHandle = falseso Babylon can sample it.
Comment on lines
+1881
to
+1885
| Graphics::FrameBuffer* frameBuffer = new Graphics::FrameBuffer(m_deviceContext, frameBufferHandle, width, height, false, generateDepth, generateStencilBuffer, depthStencilAttachmentIndex); | ||
|
|
||
| // For a standalone depth/stencil texture request, alias the framebuffer's readable depth attachment back | ||
| // into the caller-supplied texture so Babylon can sample it (e.g. fluid rendering's depth copy). The | ||
| // framebuffer owns the handle (its destructor destroys it), so the texture must not own it. |
Contributor
Author
There was a problem hiding this comment.
Good catch - fixed in e661432. CreateFrameBuffer now reports hasDepth = (generateDepth || generateStencilBuffer) so the stencil-without-depth path (which allocates a combined D24S8 attachment) no longer reports HasDepth()==false and won't skip depth clear / Z-writes.
…es depth/stencil When generateStencilBuffer is requested without generateDepth, CreateFrameBuffer still allocates a combined D24S8 depth/stencil attachment, but the FrameBuffer was constructed with hasDepth=generateDepth (false). That made HasDepth() return false, so Clear/DrawInternal skipped depth clear and Z-writes against a depth buffer that actually exists. Report hasDepth as (generateDepth || generateStencilBuffer). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
NativeEngine::CreateFrameBuffernow supports Babylon's request for a standalone, sampleable depth/stencil texture.thinNativeEngine.pure.ts::_createDepthStencilTexturecreates such a texture by callingcreateFrameBuffer(...)with a freshly-created (and therefore uninitialized) color texture — its bgfx handle is stillkInvalidHandle.CreateFrameBufferonly guarded against a null texture, so it attached the invalid handle as a color target. bgfx framebuffer validation then rejected it (Invalid texture attachment) and threw, which aborted the entire headless Playground sweep, not just the offending test.How
Detect the request (non-null texture whose bgfx handle is invalid) and instead:
BGFX_TEXTURE_RT(instead ofBGFX_TEXTURE_RT_WRITE_ONLY) so it can be sampled;ownsHandle = false, since the framebuffer owns/destroys the handle) so Babylon can sample it (e.g. fluid rendering's depth copy).Verification
Playground(Win32 D3D11, RelWithDebInfo) offmaster+ this change.Invalid texture attachmentand aborted the sweep now runs to completion with a clean exit (no abort, no stderr). RenderDoc confirms the aliased depth/stencil texture holds correct scene depth.Does this re-enable any excluded tests?
Honest answer: not on its own. I A/B-tested every crash-reason–excluded test (built with and without this change, run in isolation):
TEXCOORD7/i_data0, but the compiled shader readsPOSITION0from the per-vertex buffer), so all particles render zero-area quads and produce no fragments. That's tracked separately and needs a fix in NativeEngine's instanced-attribute routing.This PR is therefore a robustness/correctness fix: it removes a whole-sweep abort and makes standalone depth/stencil textures behave per Babylon's contract (sampleable), independent of the unrelated particle issue.
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com