Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(metal): clear only bound textures #3310

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

blaztinn
Copy link
Contributor

clearQuad() can be operating on framebuffers that don't have all the color/depth/stencil buffers attached.

For example, if there is no stencil buffer in the framebuffer, the Metal driver asserts with:

validateDepthStencilState: failed assertion `MTLDepthStencilDescriptor uses
frontFaceStencil but MTLRenderPassDescriptor has a nil stencilAttachment
texture'

Copy the logic from setFrameBuffer() to check which buffers we can clear. Using this info set the pipeline state that is valid for currently bound buffers.


The issue appeared when we created a render texture with no stencil. When trying to clear the render texture it crashed with the above message (the clear flags included BGFX_CLEAR_STENCIL).

The crash does not happen when the code path inside RendererContextMtl::submit() sets the clearWithRenderPass variable and the clearing happens elsewhere.

Is it invalid to set BGFX_CLEAR_STENCIL clear flag when there is no stencil bound? On other platforms it seems to work. In our engine we normally always set both depth and stencil clear since we don't always have the info what is bound. If it is invalid to send BGFX_CLEAR_STENCIL if there is no stencil, please ignore this PR.

I don't have a simple repro example but if needed I can try creating one.

@bkaradzic
Copy link
Owner

Is it invalid to set BGFX_CLEAR_STENCIL clear flag when there is no stencil bound? On other platforms it seems to work. In our engine we normally always set both depth and stencil clear since we don't always have the info what is bound. If it is invalid to send BGFX_CLEAR_STENCIL if there is no stencil, please ignore this PR.

Yes, it should be valid to pass BGFX_CLEAR_STENCIL and then noop it internally.

I don't have a simple repro example but if needed I can try creating one.

It would be actually great if you could create small repro (for example by modifying 13-stencil).

@blaztinn
Copy link
Contributor Author

Thx, I currently don't have enough time but I'll try to do it in a week or two.

`clearQuad()` can be operating on framebuffers that don't have all the
color/depth/stencil buffers attached.

For example, if there is no stencil buffer in the framebuffer, the Metal
driver asserts with:

```
validateDepthStencilState: failed assertion `MTLDepthStencilDescriptor uses
frontFaceStencil but MTLRenderPassDescriptor has a nil stencilAttachment
texture'
```

Copy the logic from `setFrameBuffer()` to check which buffers we can clear.
Using this info set the pipeline state that is valid for currently bound
buffers.
@blaztinn blaztinn force-pushed the fix/metal-clear-quad-stencil-issue branch from f4018da to 628d98c Compare June 20, 2024 12:25
@blaztinn
Copy link
Contributor Author

blaztinn commented Jun 20, 2024

I updated the code with a force push because it didn't compile. The original code was copied from some older commit, I guess I didn't try it on the branch I was making a PR for :(


I managed to reproduce the issue on example 21-deferred commit 61c770b0f5f57cf10547107974099e604358bf69 (commit right before this PR). The change to the example is:

diff --git a/examples/21-deferred/deferred.cpp b/examples/21-deferred/deferred.cpp
index 16c36fe7a..f6a4584ff 100644
--- a/examples/21-deferred/deferred.cpp
+++ b/examples/21-deferred/deferred.cpp
@@ -235,6 +235,15 @@ public:
 
                // Set light pass view clear state.
                bgfx::setViewClear(kRenderPassLight
+                // The next BGFX_CLEAR_DEPTH at commit 61c770b0f5f57cf10547107974099e604358bf69 produces:
+                //   [MTLDebugRenderCommandEncoder validateCommonDrawErrors:]:5775: failed assertion `Draw Errors Validation
+                //   MTLDepthStencilDescriptor has depthWriteEnabled but MTLRenderPassDescriptor has a nil depthAttachment texture
+                // at
+                //   #11    0x0000000101b0d7ac in -[CaptureMTLRenderCommandEncoder drawPrimitives:vertexStart:vertexCount:instanceCount:] ()
+                //   #12    0x000000010016dc40 in bgfx::mtl::RenderCommandEncoder::drawPrimitives(MTLPrimitiveType, unsigned long, unsigned long, unsigned long) at /Users/blaz.tomazic/Projects/bgfx/src/renderer_mtl.h:541
+                //   #13    0x000000010016a85c in bgfx::mtl::RendererContextMtl::clearQuad(bgfx::ClearQuad&, bgfx::Rect const&, bgfx::Clear const&, float const (*) [4]) at /Users/blaz.tomazic/Projects/bgfx/src/renderer_mtl.mm:1820
+                //   #14    0x0000000100166140 in bgfx::mtl::RendererContextMtl::submit(bgfx::Frame*, bgfx::ClearQuad&, bgfx::TextVideoMemBlitter&) at /Users/blaz.tomazic/Projects/bgfx/src/renderer_mtl.mm:4413
+
                                , BGFX_CLEAR_COLOR|BGFX_CLEAR_DEPTH
                                , 1.0f
                                , 0
@@ -496,7 +505,7 @@ public:
                                bgfx::dbgTextPrintf(0, 0, blink ? 0x4f : 0x04, " MRT not supported by GPU. ");
 
                                // Set view 0 default viewport.
-                               bgfx::setViewRect(0, 0, 0, uint16_t(m_width), uint16_t(m_height) );
+                               bgfx::setViewRect(0, 1, 1, uint16_t(m_width), uint16_t(m_height) );
 
                                // This dummy draw call is here to make sure that view 0 is cleared
                                // if no other draw calls are submitted to view 0.
@@ -594,12 +603,12 @@ public:
                                float vp[16];
                                float invMvp[16];
                                {
-                                       bgfx::setViewRect(kRenderPassGeometry,     0, 0, uint16_t(m_width), uint16_t(m_height) );
-                                       bgfx::setViewRect(kRenderPassClearUav,     0, 0, uint16_t(m_width), uint16_t(m_height) );
-                                       bgfx::setViewRect(kRenderPassLight,        0, 0, uint16_t(m_width), uint16_t(m_height) );
-                                       bgfx::setViewRect(kRenderPassCombine,      0, 0, uint16_t(m_width), uint16_t(m_height) );
-                                       bgfx::setViewRect(kRenderPassDebugLights,  0, 0, uint16_t(m_width), uint16_t(m_height) );
-                                       bgfx::setViewRect(kRenderPassDebugGBuffer, 0, 0, uint16_t(m_width), uint16_t(m_height) );
+                                       bgfx::setViewRect(kRenderPassGeometry,     1, 1, uint16_t(m_width), uint16_t(m_height) );
+                                       bgfx::setViewRect(kRenderPassClearUav,     1, 1, uint16_t(m_width), uint16_t(m_height) );
+                                       bgfx::setViewRect(kRenderPassLight,        1, 1, uint16_t(m_width), uint16_t(m_height) );
+                                       bgfx::setViewRect(kRenderPassCombine,      1, 1, uint16_t(m_width), uint16_t(m_height) );
+                                       bgfx::setViewRect(kRenderPassDebugLights,  1, 1, uint16_t(m_width), uint16_t(m_height) );
+                                       bgfx::setViewRect(kRenderPassDebugGBuffer, 1, 1, uint16_t(m_width), uint16_t(m_height) );
 
                                        if (!m_useUav)
                                        {

To force the example into calling into clearQuad() I needed to offset the view rects.

The example produces:

[MTLDebugRenderCommandEncoder validateCommonDrawErrors:]:5775: failed assertion `Draw Errors Validation
MTLDepthStencilDescriptor has depthWriteEnabled but MTLRenderPassDescriptor has a nil depthAttachment texture

The depth texture is not bound/attached. The PR fixes the issue for each attachment separately (color, depth and stencil).


Removing the BGFX_CLEAR_DEPTH at the commented place from the example makes the example work without the PR. My PR fixes it so that the example works with BGFX_CLEAR_DEPTH present.

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.

2 participants