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

Relax render pass color_attachments validation #2778

Merged
merged 6 commits into from
Jun 27, 2022

Conversation

jinleili
Copy link
Contributor

Connections
#2469

Description

  1. We need to make the color attachments Option-al in render pipelines, render passes, and render bundles

I tried Option-al all of these, but that makes the code changes look not so good.
Since &[] can express None, is it still necessary to use Option?

Testing
Tested locally

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

The rename looks good - could we split that out into its own PR?

However, the WebIDL and the spec language make it clear that colorAttachments can be an array with holes, like [x, null, y], so it really does need to be an Cow<'a, [Option<RenderPassColorAttachment>]> to match the spec.

wgpu-core/src/command/render.rs Outdated Show resolved Hide resolved
@jinleili jinleili force-pushed the color-attachment branch 2 times, most recently from 1f2d807 to 823bd0a Compare June 25, 2022 12:06
@jinleili
Copy link
Contributor Author

jinleili commented Jun 25, 2022

Tested color_attachments: [Some], [Some, None], [None, Some], [Some, None, Some] on metal, dx12, vulkan backends, wasm target and gl backend only support non-hole attachments currently.

Since a new version of the aligned WebGPU spec web-sys was recently released, sparse attachments support must be placed after the web-sys upgrade.

TypeError: GPUDevice.createRenderPipeline: Missing required 'format' member of GPUColorTargetState.
    __wbg_createRenderPipeline_9112d655cc89bd45 http://localhost:8000/skybox2.js:599

Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

Thank you so much for undertaking this! That turned out to be a massive change.

wgpu-hal/src/dx12/mod.rs Outdated Show resolved Hide resolved
wgpu-hal/src/dx12/command.rs Outdated Show resolved Hide resolved
wgpu-hal/src/gles/command.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
wgpu-hal/src/vulkan/adapter.rs Outdated Show resolved Hide resolved
wgpu-core/src/command/render.rs Outdated Show resolved Hide resolved
wgpu-core/src/device/mod.rs Outdated Show resolved Hide resolved
wgpu-core/src/device/mod.rs Show resolved Hide resolved
wgpu-core/src/device/mod.rs Show resolved Hide resolved
wgpu-core/src/device/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

This looks great - thank you!

The only thing that still concerns me is the handling of rtv_pool. Based on discussion in Matrix, I think we can avoid having to share rtv_pool altogether, by allocating the null handle we need earlier, and just passing it in to command buffer creation. See this comment.

wgpu-hal/src/dx12/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@jimblandy jimblandy left a comment

Choose a reason for hiding this comment

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

superb

@jimblandy jimblandy merged commit 61796b1 into gfx-rs:master Jun 27, 2022
@jinleili jinleili deleted the color-attachment branch June 27, 2022 23:17
crowlKats added a commit to crowlKats/wgpu that referenced this pull request Jul 20, 2022
cwfitzgerald pushed a commit that referenced this pull request Jul 20, 2022
* upstream GPUAutoLayoutMode

* clean up symbols and fix miscalled op

* fix #2778
@crowlKats crowlKats mentioned this pull request Jul 21, 2022
37 tasks
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