Refactor the GPURenderPipelineDescriptor according to aspects by kvark · Pull Request #1352 · gpuweb/gpuweb

kvark · 2021-01-15T17:15:06Z

Groups the properties of a pipeline according to the aspects. Related things are kept together, even if they can affect multiple stages of a pipeline.

Closes #1307
Closes #936

I also want to re-order the sections of the pipeline, but this can be done as a follow-up in order to make this PR easier.

Preview | Diff

github-actions · 2021-01-15T17:17:30Z

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
_9510cc1

toji · 2021-01-15T18:12:04Z

On the last call you had an example that showed every possible descriptor property in one big struct. Could you link that again here or paste it into the comments? It's slightly tricky to get the complete picture from the various diffed chunks spread throughout the spec and it would serve as a useful reference for the intent.

kvark · 2021-01-15T18:23:35Z

device.createRenderPipeline({
    vertex: {
        inputs: [{
            arrayStride: 16,
            stepMode: "vertex",
            attributes: [{
                offset: 0,
                format: "float2",
            }],
        }],
        module: module,
        entryPoint: "vs_main",
    },
    primitive: {
        topology: "triangle-strip",
        stripIndexFormat: "uint16",
        frontFace: "ccw",
        cullMode: "back",
    },
    depthStencil: {
        format: "depth24plus",
        depthWriteEnabled: true,
        depthCompare: "less",
        depthBias: 2,
        depthBiasSlopeScale: 1.0,
        depthBiasClamp: 8.0,
        stencilReadMask: 0xF,
        stencilWriteMask: 0x1,
        stencilFront: {
            compare: "greater",
            failOp: "keep",
            depthFailOp: "invert",
            passOp: "zero",
        },
        stencilBack: {},
    },
    multisample: {
        count: 4,
        mask: 0xF,
        alphaToCoverage: true,
    },
    fragment: {
        outputs: [{
            format: 'rgba8unorm',
            writeMask: 0xF,
            blend: {
                color: {
                    srcFactor: "one",
                    dstFactor: "src-alpha",
                    operation: "add",
                },
                alpha: {},
            },
        }],
        module: module,
        entryPoint: "fs_main",
    },
});

toji

This is looking really good! Thanks!

toji · 2021-01-15T18:26:52Z

+
+<script type=idl>
+dictionary GPUFragmentState: GPUProgrammableStage {
+    required sequence<GPUColorState> outputs;


RE: my suggestion about changing inputs on the vertex stage, maybe this could be colorOutputs to keep them more aligned?

Sure! Let's come back to this once we figure out what needs to be done with inputs, if anything

toji · 2021-01-15T20:26:17Z


-            - [$validating GPUProgrammableStageDescriptor$]({{GPUShaderStage/VERTEX}},
-                |descriptor|.{{GPURenderPipelineDescriptor/vertexStage}},
+            - [$validating GPUProgrammableStage$]({{GPUShaderStage/VERTEX}},


Since the vertexState is a programmable stage now should this step be moved into the validating GPUVertexState algorithm? Same for fragmentState below.

We can do it for sure! I think we can also consider this to be a follow-up.

kvark

Much detailed review, thank you!

kvark · 2021-01-15T22:33:22Z


-            - [$validating GPUProgrammableStageDescriptor$]({{GPUShaderStage/VERTEX}},
-                |descriptor|.{{GPURenderPipelineDescriptor/vertexStage}},
+            - [$validating GPUProgrammableStage$]({{GPUShaderStage/VERTEX}},


We can do it for sure! I think we can also consider this to be a follow-up.

kvark · 2021-01-15T22:35:19Z

+
+<script type=idl>
+dictionary GPUFragmentState: GPUProgrammableStage {
+    required sequence<GPUColorState> outputs;


Sure! Let's come back to this once we figure out what needs to be done with inputs, if anything

github-actions · 2021-01-15T22:45:10Z

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
_a418892

Kangz

This change is amazing! Hopefully we don't need to move stuff around the GPURenderPipelineDescriptor a second time >_>

github-actions · 2021-01-18T15:10:52Z

Previews, as seen at the time of posting this comment:
WebGPU | IDL
WGSL
_4c911d9

kainino0x · 2021-01-21T00:14:04Z

This change is amazing! Hopefully we don't need to move stuff around the GPURenderPipelineDescriptor a second time >_>

FYI I want to consider a change after this one to change alphaToCoverageEnabled: true to multisampleMode: "alpha-to-coverage" because sample-rate shading would rightly go in that slot if we had it.

kainino0x

looks great!

kvark · 2021-01-21T02:32:07Z

In the last commit the changes are the following:

tried to clean up the distinction between "xxxState" and "xxxDescriptor". Settled on the latter being used only for the top-level structures. Therefore, VertexBufferLayoutDescriptor is renamed to just VertexBufferState, and same for VertexAttributeState.
renamed GPUColorState to GPUColorTargetState, since it's not a state of a "color"
inputs became inputBuffers and outputs became outputTargets.

kvark · 2021-01-21T02:42:08Z

Also, rebased and squashed. It's very easy to run into conflicts with this.

kainino0x · 2021-01-21T04:00:31Z

Therefore, VertexBufferLayoutDescriptor is renamed to just VertexBufferState, and same for VertexAttributeState.

I like the word Layout, and think State isn't really important, could they just be VertexBufferLayout and VertexAttribute?

Also, rebased and squashed. It's very easy to run into conflicts with this.

sg but a merge commit would have been easier for incremental review if possible.

toji · 2021-01-21T04:02:49Z

inputs became inputBuffers and outputs became outputTargets

That's perfect! 😄 This change LGTM now.

FYI I want to consider a change after this one to change alphaToCoverageEnabled: true to multisampleMode: "alpha-to-coverage"

Nit on Kai's follow up request: That would actually be multisample.mode, right?

kainino0x · 2021-01-21T04:18:10Z

Nit on Kai's follow up request: That would actually be multisample.mode, right?

Yeah, probably, unless we have a better name than mode.

kvark · 2021-01-21T14:22:41Z

Let's have the multisample discussion in a separate issue, perhaps? It sounds interesting, but I don't want it to drag this change.
@kainino0x your suggestion sounds appropriate, I've added the commit doing this.

As for the merge commit - there was a conflict already. I generally resolve conflicts by rebasing, but in this case it's much easier to resolve it on the whole thing than trying to resolve conflicts on each commit one by one. Just squashed to save time.

Kangz · 2021-01-21T15:18:33Z

inputs became inputBuffers and outputs became outputTargets.

I don't think the longer names were necessary, but unless more people feel the same I don't think my opinion should block this change.

kainino0x

LGTM modulo that final bikeshedding (I don't prefer the new names over inputs/outputs but they're okay)

Let's have the multisample discussion in a separate issue, perhaps?

Yeah, was planning to open a PR after this lands.

toji · 2021-01-21T18:46:33Z

My sole reason for advocating for a name like inputBuffers is that it helps developers (especially ones new to the API) reason about what it describes better and disambiguates it from other types of shader input, but I think that's worthwhile.

I don't think the longer names were necessary

We have some serious refactoring to do if "reduce the amount of typing required" is an API goal. 😉

kvark · 2021-01-21T19:26:10Z

@kainino0x oh, wait, I misunderstood. I thought you wanted these longer names, based on #1352 (comment)
If neither you or @Kangz are happy with these names, we should reconsider. What names would you want?

kainino0x · 2021-01-21T19:30:03Z

@kvark It's not that I have anything against slightly longer names, I just am not sure whether these are actually clearer - e.g. inputBuffers doesn't clarify that it doesn't include uniform buffers (though this probably doesn't matter that much). But if Brandon thinks so I think it's a good signal.

kainino0x · 2021-01-22T23:30:11Z

FYI I want to consider a change after this one to change alphaToCoverageEnabled: true to multisampleMode: "alpha-to-coverage" because sample-rate shading would rightly go in that slot if we had it.

Now that this PR landed I started looking into this. I learned that in fact alpha-to-coverage and sample-rate-shading are not exclusive with one another, so this idea doesn't make sense.

Sample-rate-shading means shading more than once per pixel, but can be fewer than once per sample (interpolation may occur for other samples, I think). In Vulkan I think the final coverage mask is the AND of all three mask values (pipeline sample mask, shader output sample mask, and alpha-to-coverage sample mask*), then only the relevant bits of the final mask are used (those for the samples covered by the shader invocation).

* though there is no guarantee about the alpha-to-coverage mask produced. According to #267, D3D12 disables alpha-to-coverage if SV_Coverage is used. I think this is why in WebGPU we don't allow alpha-to-coverage with SV_Coverage (added in #724), though given the leniency of the alpha-to-coverage mask I think we could simply allow it and stay within spec.

Additionally I learned that alpha-to-one is a separate (and optional) feature which is not turned on by enabling alpha-to-coverage.

kainino0x · 2021-01-22T23:32:07Z

though given the leniency of the alpha-to-coverage mask I think we could simply allow it and stay within spec.

I should say, I don't think this is necessary. Being strict here is probably best as it sets expectations that the two aren't likely to actually work together.

This CL adds unimplemented stub tests for the `trunc` builtin. Issues: gpuweb#1243

kvark requested review from kainino0x and toji January 15, 2021 17:15

toji reviewed Jan 15, 2021

View reviewed changes

kvark commented Jan 15, 2021

View reviewed changes

Kangz approved these changes Jan 18, 2021

View reviewed changes

Comment thread spec/index.bs Outdated

kainino0x approved these changes Jan 21, 2021

View reviewed changes

Comment thread spec/index.bs Outdated

Refactor the GPURenderPipelineDescriptor according to aspects

fa71b9b

kvark force-pushed the pipeline-aspect branch from f186ef5 to fa71b9b Compare January 21, 2021 02:41

toji approved these changes Jan 21, 2021

View reviewed changes

Rename vertex buffer layout and attributes some more

c4e761d

kainino0x closed this Jan 21, 2021

kainino0x reopened this Jan 21, 2021

kainino0x approved these changes Jan 21, 2021

View reviewed changes

toji reviewed Jan 21, 2021

View reviewed changes

Comment thread spec/index.bs Outdated

kvark added 2 commits January 21, 2021 15:36

Convert the remanining Descriptor things into State

472284a

Rename inputBuffers to buffers, outputTargets to targets

5a0b8d9

kainino0x approved these changes Jan 21, 2021

View reviewed changes

kainino0x requested a review from toji January 21, 2021 23:50

toji approved these changes Jan 22, 2021

View reviewed changes

kvark merged commit 0e7b94e into gpuweb:main Jan 22, 2021

kvark deleted the pipeline-aspect branch January 22, 2021 04:11

kvark mentioned this pull request Jan 24, 2021

Update RenderPipelineDescriptor API according to aspects gfx-rs/wgpu#1166

Closed

ben-clayton pushed a commit to ben-clayton/gpuweb that referenced this pull request Sep 6, 2022

wgsl: Stub tests for trunc builtin. (gpuweb#1352)

e817a56

This CL adds unimplemented stub tests for the `trunc` builtin. Issues: gpuweb#1243

Conversation

kvark commented Jan 15, 2021 • edited by pr-preview Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jan 15, 2021

Uh oh!

toji commented Jan 15, 2021

Uh oh!

kvark commented Jan 15, 2021

Uh oh!

toji left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

toji Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

kvark Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

toji Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

kvark Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

kvark Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

kvark Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jan 15, 2021

Uh oh!

Kangz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jan 18, 2021

Uh oh!

kainino0x commented Jan 21, 2021

Uh oh!

kainino0x left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kvark commented Jan 21, 2021

Uh oh!

kvark commented Jan 21, 2021

Uh oh!

kainino0x commented Jan 21, 2021

Uh oh!

toji commented Jan 21, 2021

Uh oh!

kainino0x commented Jan 21, 2021

Uh oh!

kvark commented Jan 21, 2021

Uh oh!

Kangz commented Jan 21, 2021

Uh oh!

kainino0x left a comment

Choose a reason for hiding this comment

Uh oh!

toji commented Jan 21, 2021

Uh oh!

Uh oh!

kvark commented Jan 21, 2021

Uh oh!

kainino0x commented Jan 21, 2021

Uh oh!

kvark commented Jan 15, 2021 •

edited by pr-preview Bot

Loading