bkaradzic / bgfx

Cross-platform, graphics API agnostic, "Bring Your Own Engine/Framework" style rendering library.
https://bkaradzic.github.io/bgfx/overview.html
BSD 2-Clause "Simplified" License
14.59k stars 1.92k forks source link

fix(metal): clear only bound textures #3310

Open blaztinn opened 2 weeks ago

blaztinn commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

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

blaztinn commented 1 week ago

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.