Pass attachments state by value - #102
Conversation
| sequence<WebGPURenderPassColorAttachmentDescriptor> colorAttachments; | ||
| WebGPURenderPassDepthStencilAttachmentDescriptor depthStencilAttachment; | ||
| // Number of MSAA samples for rasterization | ||
| u32 samples; |
There was a problem hiding this comment.
Isn't that already encoded in the textures themselves? We have to check that all textures have the same sample count.
There was a problem hiding this comment.
that's the point: we'll go through the actual textures and check their sample counts:
- if it matches, good
- it this one is higher than 1, but the texture sample count is 1, then this is a resolve attachment
- otherwise, it's an error
There was a problem hiding this comment.
What if samples is 2 and the texture has a sample count of 4? I'm not sure that having magic values is the best way to describe multisample resolves. Didn't we have a whole GitHub investigation on multisampling?
There was a problem hiding this comment.
I am also confused by the samples field. Having one samples field prevents us from resolving some of the attachments to 4 and others to 2. I think we also need additional data to know which textures are resolve sources and which ones are resolve destinations.
| // Array of color attachments | ||
| sequence<WebGPUAttachment> colorAttachments; | ||
| // Optional depth/stencil attachment | ||
| WebGPUAttachment? depthStencilAttachment; |
There was a problem hiding this comment.
Not important for WebGPU, but this will translate to a nullable pointer to struct in Dawn, which our remoting layer doesn't have support for atm.
There was a problem hiding this comment.
How else do you express a proper Option? :)
| // Attachment data format | ||
| WebGPUTextureFormatEnum format; | ||
| // Number of MSAA samples | ||
| u32 samples; |
There was a problem hiding this comment.
This is pending discussion of multisampling, but I think we also need to add "boolean isResolvedAtEndOfRenderPass" because VkRenderPass compatibility includes "VkSubpassDescription:: pResolveAttachments". I'll open an issue on Vulkan-Docs to ask if this can be relaxed for single-subpass render passes.
There was a problem hiding this comment.
the point of this PR is that we can derive the need of those being resolved simply by looking at the sample counts
|
|
||
| interface WebGPUAttachmentsState { | ||
| // Description of the framebuffer attachments | ||
| dictionary WebGPUAttachmentsState { |
There was a problem hiding this comment.
Why make it a dictionary?
There was a problem hiding this comment.
The point of this PR is to remove the attachment state as a handle, because there is no backend-specific data to associate with it. The render pass details is derived from the actual data passed to render pipeline creation and the beginning of a render pass.
| sequence<WebGPURenderPassColorAttachmentDescriptor> colorAttachments; | ||
| WebGPURenderPassDepthStencilAttachmentDescriptor depthStencilAttachment; | ||
| // Number of MSAA samples for rasterization | ||
| u32 samples; |
There was a problem hiding this comment.
What if samples is 2 and the texture has a sample count of 4? I'm not sure that having magic values is the best way to describe multisample resolves. Didn't we have a whole GitHub investigation on multisampling?
| WebGPUInputState inputState; | ||
| WebGPUAttachmentsState attachmentsState; | ||
| // Number of MSAA samples for | ||
| u32 samples; |
There was a problem hiding this comment.
so this u32 samples is compared to the u32 samples below? I'm a little confused.
| sequence<WebGPUTextureFormatEnum> formats; | ||
| // TODO other stuff like sample count etc. | ||
| // Description of a single attachment | ||
| dictionary WebGPUAttachment { |
There was a problem hiding this comment.
Is this eventually going to be the place where we add things like blending operations?
There was a problem hiding this comment.
We might want to have separate types for color attachments and depth/stencil attachments before we add blending.
|
I removed the MSAA bits from everything except |
Kangz
left a comment
There was a problem hiding this comment.
Thanks for the update, LGTM
RafaelCintron
left a comment
There was a problem hiding this comment.
@kvark 's latest change LGTM.
25: Proper render pass support r=grovesNL a=kvark This PR implements the API scheme of gpuweb/gpuweb#102 and caches both render passes and framebuffers properly in the device. Co-authored-by: Dzmitry Malyshau <dmalyshau@mozilla.com>
This is an alternative to #92. I like it less, but I need this problem to be solved so here is what the other position can look like. Please do form an opinion on which approach works better for us!
Fixes #103
The way this proposal works is: attachments state is not baked. We provide it as a dictionary when creating a render pipeline, and it has the necessary information to figure out what attachments need to be resolved by the end of an implicit pass. When beginning a render pass, we have the actual textures, so we can derive the same formats and sample counts, plus we provide the MSAA sample count of rasterization, which should allow us to reconstruct/derive the render pass object to use.
The reason I like this solution less is: there are more implicit bits, and more work for the backend. We need to document that the attachment formats and sample counts have to exactly match between render pipelines and render passes where they are used, and the backend needs to check for them. In #92, the idea was that the actual handle to baked
WebGPUAttachmentsStatehas to match, which is easier to document, and the backend doesn't have to keep a global map between the attachment descriptions and actual native render passes.