Skip to content

Allow sparse color attachments and ignored FS outputs#2562

Merged
kainino0x merged 2 commits intogpuweb:mainfrom
Kangz:ignore-fs-outputs
Feb 10, 2022
Merged

Allow sparse color attachments and ignored FS outputs#2562
kainino0x merged 2 commits intogpuweb:mainfrom
Kangz:ignore-fs-outputs

Conversation

@Kangz
Copy link
Copy Markdown
Contributor

@Kangz Kangz commented Feb 3, 2022 •
edited by pr-preview bot
Loading

Fixes #1250
Fixes #2060

Preview | Diff

@Kangz Kangz requested review from kainino0x, kvark and toji February 3, 2022 10:37
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 3, 2022

Previews, as seen when this build job started (4db41e6):
WebGPU | IDL
WGSL
Explainer

kvark
kvark requested changes Feb 4, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@kvark kvark left a comment

Choose a reason for hiding this comment

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

blocking on investigation, which wasn't done yet

@toji
Copy link
Copy Markdown
Member

toji commented Feb 5, 2022

No qualms about the API shape, assuming the group agrees this feature is feasible.

@kvark: Doing a quick review of the linked issues it's not clear to me what needs to be investigated still? #1250 (comment) seems to confirm that it's technically feasible at least.

@Kangz
Copy link
Copy Markdown
Contributor Author

Kangz commented Feb 7, 2022

Yeah I should have more clearly linked to that investigation. The behavior works on all APIs we targeted, even running with the debug layers.

kainino0x
kainino0x reviewed Feb 7, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

I just realized that using nullable here means both null and undefined work, which is great because I think it should mean actual sparse arrays work (don't have to explicitly specify null). OTOH I may prefer @kvark's suggestion to add a location member to these dictionaries similar to vertex attributeLocation.

Also need to update:

  • For each |colorTarget| in |descriptor|.{{GPURenderPipelineDescriptor/fragment}}.{{GPUFragmentState/targets}}:

  • In alpha-to-coverage mode, an additional alpha-to-coverage mask
    of MSAA samples is generated based on the |alpha| component of the
    fragment shader output value of the {{GPURenderPipelineDescriptor/fragment}}.{{GPUFragmentState/targets}}[0].

  • Definition of colorAttachments should explain how it works

  • derive render targets layout from pass

kvark
kvark reviewed Feb 8, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Investigation approved:)
Considered alternative approach - not going to work out.
So we can proceed with this in principle, although @kainino0x had ideas on making the shape of the API more usable.
One last thing to address

@kainino0x
Copy link
Copy Markdown
Contributor

kainino0x commented Feb 8, 2022 •
edited
Loading

So we can proceed with this in principle, although @kainino0x had ideas on making the shape of the API more usable.

If we want to allow a {0: x, 2: y} syntax we could use a record<> but I don't think it's important. Like I said above it introduces "optional" syntax and that's ugly. Maybe we could add it later as a nonbreaking change (sequence<> or record<>) if we really wanted to.

kainino0x
kainino0x approved these changes Feb 8, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

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

no further comments, happy to go with this design option

@Kangz
Copy link
Copy Markdown
Contributor Author

Kangz commented Feb 9, 2022

Sorry, I'm not going to be able to address review comments anytime soon. Can someone take over when/if the group decides to go forward with this?

@kvark
Copy link
Copy Markdown
Contributor

kvark commented Feb 9, 2022

@Kangz I'll push to this PR and merge

@kvark kvark self-assigned this Feb 9, 2022
@kvark kvark force-pushed the ignore-fs-outputs branch from 4db41e6 to e822402 Compare February 9, 2022 21:51
@kvark kvark mentioned this pull request Feb 9, 2022
Closed
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 9, 2022

Previews, as seen when this build job started (e822402):
WebGPU | IDL
WGSL
Explainer

@kainino0x kainino0x merged commit b9b28d6 into gpuweb:main Feb 10, 2022
github-actions bot added a commit that referenced this pull request Feb 10, 2022
@Kangz @github-actions
Allow sparse color attachments and ignored FS outputs (#2562)
24f33a1 
SHA: b9b28d6
Reason: push, by @kainino0x

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@kainino0x
Copy link
Copy Markdown
Contributor

kainino0x commented Feb 10, 2022

Ah, merging this closed 2 issues. Are they both fully resolved? #2060 in particular.

github-actions bot added a commit that referenced this pull request Feb 10, 2022
@Kangz @github-actions
Allow sparse color attachments and ignored FS outputs (#2562)
0992df2 
SHA: b9b28d6
Reason: push, by @kainino0x

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request Feb 10, 2022
@Kangz @github-actions
Allow sparse color attachments and ignored FS outputs (#2562)
e32906f 
SHA: b9b28d6
Reason: push, by @kainino0x

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@Kangz Kangz deleted the ignore-fs-outputs branch February 10, 2022 08:19
@Kangz
Copy link
Copy Markdown
Contributor Author

Kangz commented Feb 10, 2022

Yeah #2060 is fixed: there's the note, and the fragment processing description says:

Set fragment.colors to the user-specified pipeline output values from the shader.

Which is imprecise bit supports sparse attachments correctly. (ideally it should be a loop that says, for each attachment color, take the value if it is present, or fill with (0, 0, 0, 1)).

@kainino0x kainino0x added the needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet label Jun 29, 2022
juj added a commit to juj/wasm_webgpu that referenced this pull request Aug 17, 2022
@juj
IDL: Allow color targets to be null to specify sparse render targets. g…
2ef293f 
…puweb/gpuweb#2562
@lokokung lokokung removed the needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet label Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kvark kvark kvark approved these changes

@kainino0x kainino0x kainino0x approved these changes

@toji toji toji approved these changes

Assignees

@kvark kvark

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature request: ignore shader writes to color attachments Sparse colorAttachments in a render pass

5 participants

@Kangz @toji @kainino0x @kvark @lokokung