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

Abstract some of RDPipelineDepthStencilState into a new RDStencilOpState class #8882

Open
DevPoodle opened this issue Jan 14, 2024 · 2 comments

Comments

@DevPoodle
Copy link

DevPoodle commented Jan 14, 2024

Describe the project you are working on

Writing documentation for RDPipelineDepthStencilState (see godotengine/godot#87156)

Describe the problem or limitation you are having in your project

RDPipelineDepthStencilState has a bunch of parameters for the front stencil buffer and the back stencil buffer. In Vulkan, these each are an instance of VkStencilOpState, but in Godot, all of those parameters are just lumped in with RDPipelineDepthStencilState. It makes the documentation less clean, as there has to be a bunch of repeated property descriptions. It makes it more annoying to use, as you have to manually set all of the front and back parameters, even if you want them to be the same. Also, it might make it a little harder for inexperienced users to know what to search up when trying to learn how to use the class.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Remove all of the front and back parameters from RDPipelineDepthStencilState, replacing them with properties called front_stencil_op and back_stencil_op. These each are an instance of a new RDStencilOpState class. This allows us to have specific documentation about the difference between the front and back buffers, and more general documentation about how stencil operations work, without having too many duplicate descriptions. It more closely matches how Vulkan handles these objects, making it easier to cross-reference with official Vulkan documentation. Also, it means users can duplicate an RDStencilOpState, if they want to have the back and front buffer settings be the same, or if they want to use some of the same settings between different pipelines.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

RDStencilOpState will be defined in the same place as RDPipelineDepthStencilState. It has all of the properties that you'd expect. Ideally, we'd also have some helper functions in the Vulkan driver that allow you to convert RDStencilOpState directly into it's associated Vulkan class, which would help simplify render_pipeline_create. In the documentation, front_stencil_op and back_stencil_op would have fairly in-depth descriptions, describing their differences and what they're used for. The main description of the new RDStencilOpState class would have links to explain what the stencil buffer is, and how it can be used. Each of it's propertys' descriptions would also be fairly in depth.

(I know this would be a bit annoying, but I think it would also be nice if we could move the StencilOp enum into RDStencilOpState. RenderingDevice already has quite a lot of enums, and it would be nice if we could simplify it a bit.)

If this enhancement will not be used often, can it be worked around with a few lines of script?

No, this is about the structure of a built-in class.

Is there a reason why this should be core and not an add-on in the asset library?

This improves the main documentation and usability of RenderingDevice.

@clayjohn
Copy link
Member

Have you looked at how this will interact with D3D12/Metal as well?

@DevPoodle
Copy link
Author

DevPoodle commented Jan 15, 2024

Both D3D12 and Metal have a similar structure to Vulkan with a DepthStencilState object that contains two separate StencilOp objects for the front and back buffers (they're named slightly differently, but same idea).

D3D12: https://learn.microsoft.com/en-us/windows/win32/api/d3d12/ns-d3d12-d3d12_depth_stencil_desc1
Metal: https://developer.apple.com/documentation/metal/mtldepthstencildescriptor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants