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.

Sign up for GitHub

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

Jump to bottom

sokol_gfx.h: add explicit depth / stencil formats #1124

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

sokol_gfx.h: add explicit depth / stencil formats #1124

wants to merge 2 commits into from

Conversation

kcbanner
Copy link
Contributor

@kcbanner kcbanner commented Oct 15, 2024 •
edited
Loading

Closes #1122.

@kcbanner kcbanner marked this pull request as ready for review October 17, 2024 02:44
kcbanner
kcbanner commented Oct 17, 2024
View reviewed changes
sokol_gfx.h
case SG_PIXELFORMAT_RGBA32F: return DXGI_FORMAT_R32G32B32A32_FLOAT;
case SG_PIXELFORMAT_DEPTH: return DXGI_FORMAT_R32_TYPELESS;
case SG_PIXELFORMAT_DEPTH_STENCIL: return DXGI_FORMAT_R24G8_TYPELESS;
case SG_PIXELFORMAT_DEPTH24PLUS: return DXGI_FORMAT_R24G8_TYPELESS;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find a DirectX format that mapped to a 24-bit depth buffer with no stencil - so SG_PIXELFORMAT_DEPTH24PLUS is equivalent to SG_PIXELFORMAT_DEPTH24PLUS_STENCIL8. Is this the correct approach, or should this format not be supported at all on DirectX?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know either but will also try to find more info on that (and also test on Windows).

kcbanner
kcbanner commented Oct 17, 2024
View reviewed changes
sokol_gfx.h
@@ -11693,6 +11768,10 @@ _SOKOL_PRIVATE MTLPixelFormat _sg_mtl_pixel_format(sg_pixel_format fmt) {
case SG_PIXELFORMAT_RGBA32F: return MTLPixelFormatRGBA32Float;
case SG_PIXELFORMAT_DEPTH: return MTLPixelFormatDepth32Float;
case SG_PIXELFORMAT_DEPTH_STENCIL: return MTLPixelFormatDepth32Float_Stencil8;
case SG_PIXELFORMAT_DEPTH24PLUS: return MTLPixelFormatDepth24Unorm_Stencil8;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to the problem on DirectX, there is no format that maps directly to just a 24-bit depth buffer.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...I wonder if this is actually the reason why WebGPU calls it 'Depth24Plus'... as with D3D11, I'll check and test... if there's not pure Depth24 format I think it's ok to have the unused stencil buffer bits hanging around (it kinda makes sense because 24 bits are 3 bytes, so there would be 8 unused bits anyway).

@kcbanner kcbanner force-pushed the depth_stencil_formats branch from 5255953 to f24e71d Compare October 17, 2024 02:52
@kcbanner kcbanner changed the title sokol_gfx.h: add SG_PIXELFORMAT_DEPTH32F_STENCIL8 sokol_gfx.h: add explicit depth / stencil formats Oct 17, 2024
@kcbanner
Copy link
Contributor Author

Moving this into review now that the other formats have been added. See the comments above - I'm not sure how to handle SG_PIXELFORMAT_DEPTH24PLUS on Metal and DirectX.

@floooh
Copy link
Owner

floooh commented Oct 18, 2024

Sorry, didn't get around yet to look into the PR. I'll try to make some time over the weekend :)

I'll most like need to do a little research first for the other 3D backends.

There's also always the option to split support for the different backends over multiple smaller PRs, might speed things up a bit :)

floooh
floooh reviewed Oct 27, 2024
View reviewed changes
Copy link
Owner

@floooh floooh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I finally made time to look into this PR. Will do some tests in a local merge branch across D3D11, GL, Metal, WebGL and WebGPU and figure out what to do about those pure Depth24 formats.

sokol_gfx.h
case SG_PIXELFORMAT_RGBA32F: return DXGI_FORMAT_R32G32B32A32_FLOAT;
case SG_PIXELFORMAT_DEPTH: return DXGI_FORMAT_R32_TYPELESS;
case SG_PIXELFORMAT_DEPTH_STENCIL: return DXGI_FORMAT_R24G8_TYPELESS;
case SG_PIXELFORMAT_DEPTH24PLUS: return DXGI_FORMAT_R24G8_TYPELESS;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know either but will also try to find more info on that (and also test on Windows).

sokol_gfx.h
@@ -11693,6 +11768,10 @@ _SOKOL_PRIVATE MTLPixelFormat _sg_mtl_pixel_format(sg_pixel_format fmt) {
case SG_PIXELFORMAT_RGBA32F: return MTLPixelFormatRGBA32Float;
case SG_PIXELFORMAT_DEPTH: return MTLPixelFormatDepth32Float;
case SG_PIXELFORMAT_DEPTH_STENCIL: return MTLPixelFormatDepth32Float_Stencil8;
case SG_PIXELFORMAT_DEPTH24PLUS: return MTLPixelFormatDepth24Unorm_Stencil8;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...I wonder if this is actually the reason why WebGPU calls it 'Depth24Plus'... as with D3D11, I'll check and test... if there's not pure Depth24 format I think it's ok to have the unused stencil buffer bits hanging around (it kinda makes sense because 24 bits are 3 bytes, so there would be 8 unused bits anyway).

@floooh
Copy link
Owner

floooh commented Oct 27, 2024

PS: don't worry about the merge conflict in CHANGELOG.md, I'll take care of that in my merge branch.

@floooh
Copy link
Owner

floooh commented Oct 27, 2024 •
edited
Loading

Ok, some Metal info:

The Depth24Unorm formats are only supported on macOS, but not on iOS. That's why WebGPU maps those to the Float32 formats: https://github.com/google/dawn/blob/e1c1d1cdf81dd6af3c74caf7aac55295f0e26bc5/src/dawn/native/metal/UtilsMetal.mm#L252-L258 - I think we should do the same. It's interesting that the iOS Ci pipeline builds, but I guess things will start to fail at runtime when using the Depth24Unorm formats on iOS devices.

(I guess that's another reason why WebGPU calls those formats Depth24Plus.

@floooh
Copy link
Owner

floooh commented Oct 27, 2024 •
edited
Loading

...and on D3D it's a bit more complicated in Dawn:

  • Depth24Plus is always mapped to DXGI_FORMAT_D32_FLOAT, so Depth32Float and Depth24Plus is identical in Dawn's D3D backend...
  • Depth24PlusStencil8 depends on a runtime flag. It's either mapped to DXGI_FORMAT_D32_FLOAT_S8X24_UINT or DXGI_FORMAT_D24_UNORM_S8_UINT

https://github.com/google/dawn/blob/e1c1d1cdf81dd6af3c74caf7aac55295f0e26bc5/src/dawn/native/d3d/UtilsD3D.cpp#L329-L340

...same in the Vulkan backend, Depth24Plus is always mapped to Float32, and Depth24PlusStencil8 depends on a runtime toggle and is either mapped to VK_FORMAT_D32_SFLOAT_S8_UINT or VK_FORMAT_D24_UNORM_S8_UINT.

WebGPU's cross-platform features are very well researched, so when they do this there's must be a very good reason.

TBH, I wonder if it's worth the trouble adding explicit Depth24 formats when at least WebGPU maps them to Float32 anyway.

The only advantage seems to be that on some platforms depth + stencil can be packed into 4 bytes (e.g. Depth24Stencil8) instead of taking 8 bytes (Depth32FloatStencil8).

But for what is the stencil buffer actually useful these days? :)

I would suggest:

  • let's drop the Depth24Plus format, since there's Depth32Float anyway and both take 4 bytes
  • and the remaining question is: is Depth24PlusStencil8 still worth the trouble if we need to implement a similar runtime flag like WebGPU to map this to different underlying formats...

...I guess I was at that point before already a couple of years ago, and that's why there's only the generic DEPTH and DEPTH_STENCIL formats in sokol-gfx :D

...looking at the current sokol_gfx.h code, SG_PIXELFORMAT_DEPTH_STENCIL is mapped differently in different backends, which isn't great either:

  • GL: GL_UNSIGNED_INT_24_8
  • D3D11: DXGI_FORMAT_R24G8_TYPELESS / DXGI_FORMAT_D24_UNORM_S8_UINT / DXGI_FORMAT_R24_UNORM_X8_TYPELESS
  • Metal: MTLPixelFormatDepth32Float_Stencil8
  • WebGPU: WGPUTextureFormat_Depth32FloatStencil8

...this different mapping would be the only good reason to have explicit pixel formats for SG_PIXELFORMAT_DEPTH24PLUS_STENCIL8 and SG_PIXELFORMAT_DEPTH32F_STENCIL8...

@floooh
Copy link
Owner

floooh commented Oct 27, 2024

@kcbanner ^^^ discuss :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@floooh floooh floooh left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

gfx proposal: support for floating point depth buffer with a stencil component
2 participants
@kcbanner @floooh