Add some more defaults in AttachmentDescriptors by litherum · Pull Request #268 · gpuweb/gpuweb

litherum · 2019-04-15T23:24:51Z

The storeOp make sense to be "store" for two reasons:

Currently, that's the only option in the IDL
For the color buffer, even if we add a "lazy clear" option, an author just wanting to render something is usually going to want to see the results of that rendering. This isn't necessarily true for the depth buffer or the stencil buffer, so this patch doesn't modify the depthStoreOp or stencilStoreOp. When authors hook up multisampling, they can also explicitly disable storing, if they desire.

Similarly for clearDepth. The default is what most authors want, which is a normal-acting depth buffer. If developers want to do something more complicated, they can modify the clearDepth themselves.

Preview | Diff

kainino0x · 2019-04-16T00:16:42Z

    required GPULoadOp depthLoadOp;
    required GPUStoreOp depthStoreOp;
-    required float clearDepth;
+    float clearDepth = 1.0;


Dzmitry was going to have an alternative proposal for this (so clearDepth is not specified when it's not used). Can we defer this one?

kainino0x · 2019-04-16T00:33:22Z

lgtm. My only comment is that it's unintuitive that the defaults would be different for the different storeOps (even though it makes more sense for this one to have a default than for the others). But since this one has a default and the others don't, it's probably fine.

kvark · 2019-04-22T21:33:28Z

Somewhat conflicts with #276

Also, I agree with @kainino0x that it would be nicer to have color/depth consistent w.r.t store op defaults.

kainino0x · 2019-07-01T19:26:35Z

#276 is closed, this now is blocked on #283 (which is blocked on #284?)

kainino0x · 2019-07-12T21:42:34Z

Rebased over #365.

beaufortfrancois

I think required should not be required.

beaufortfrancois · 2019-07-16T13:18:55Z


    required GPULoadOp loadOp;
-    required GPUStoreOp storeOp;
+    required GPUStoreOp storeOp = "store";


Suggested change

required GPUStoreOp storeOp = "store";

GPUStoreOp storeOp = "store";

beaufortfrancois · 2019-07-16T13:19:25Z

    required GPULoadOp depthLoadOp;
    required GPUStoreOp depthStoreOp;
-    required float clearDepth;
+    required float clearDepth = 1.0;


Suggested change

required float clearDepth = 1.0;

float clearDepth = 1.0;

After #283, I think this line becomes obsolete.

If all that's left is GPUStoreOp storeOp = "store";, we should merge. What do you think @kainino0x @kvark ?

Let me upload a new PR for just that change since this one is stale.

kvark · 2019-07-16T14:34:31Z

IIRC the consensus was to have the clear value being optional, with default being nil instead of a concrete value. Defaulting to 1.0 for the depth is controversial.

Supersedes gpuweb#268.

Supersedes #268.

…web#376) Supersedes gpuweb#268.

* Add a component type for GPUBGLBinding compatiblity (#384) In shaders there are several texture types for each dimensionality depending on their component type. It can be either float, uint or sint, with maybe in the future depth/stencil if WebGPU allows reading such textures. The component type of a GPUTextureView's format must match the component type of its binding in the shader module. This is for several reasons: - Vulkan requires the following: "The Sampled Type of an OpTypeImage declaration must match the numeric format of the corresponding resource in type and signedness, as shown in the SPIR-V Sampled Type column of the Interpretation of Numeric Format table, or the values obtained by reading or sampling from this image are undefined." - It is also required in OpenGL for the texture units to be complete, a uint or sint texture unit used with a non-nearest sampler is incomplete and returns black texels. Similar constraints must exist in other APIs. To encode this compatibility constraint, a new member is added to GPUBindGroupLayoutBinding that is a new enum GPUTextureComponentType that give the component type of the texture. * Make GPUBGLBinding.textureDimension default to 2d. This is the most common case and avoids having an optional dictionary member with no default value (but that still requires a value for texture bindings). * Add a component type for GPUBGLBinding compatiblity (#384) In shaders there are several texture types for each dimensionality depending on their component type. It can be either float, uint or sint, with maybe in the future depth/stencil if WebGPU allows reading such textures. The component type of a GPUTextureView's format must match the component type of its binding in the shader module. This is for several reasons: - Vulkan requires the following: "The Sampled Type of an OpTypeImage declaration must match the numeric format of the corresponding resource in type and signedness, as shown in the SPIR-V Sampled Type column of the Interpretation of Numeric Format table, or the values obtained by reading or sampling from this image are undefined." - It is also required in OpenGL for the texture units to be complete, a uint or sint texture unit used with a non-nearest sampler is incomplete and returns black texels. Similar constraints must exist in other APIs. To encode this compatibility constraint, a new member is added to GPUBindGroupLayoutBinding that is a new enum GPUTextureComponentType that give the component type of the texture. * Make GPUBGLBinding.textureDimension default to 2d. This is the most common case and avoids having an optional dictionary member with no default value (but that still requires a value for texture bindings). * unifinished createBindGroupLayout algorithm * draft of BindGroupLayout details * draft of BindGroupLayout details * polish before PR * fix typo * replace u32/i32/u64 with normal int types or specific typedefs (#423) * Do not require vertexInput in GPURenderPipelineDescriptor (#378) * Add a default for GPURenderPassColorAttachmentDescriptor.storeOp (#376) Supersedes #268. * Initial spec for GPUDevice.createBuffer (#419) * Start writing spec for device/adapter, introduce internal objects (#422) * Move validation rules out of algorithm body and better describe GPUBindGroupLayout internal slots * Include limits for dynamic offset buffers * Rename 'dynamic' boolean to 'hasDynamicOffsets' * Fix indentation for ci bot * More indentation errors * Fix var typos * Fix method definition * Fix enum references * Missing </dfn> tag * Missing </dfn> tag * Remove bad [= =] * Fix old constant name * Half-formed new validation rule structure for createBindGroupLayout * An interface -> the interface * Remove old 'layout binding' reference * fix device lost validation reference * Fix 'dynamic' typo

litherum force-pushed the storeops branch from a04571b to 64a2617 Compare April 15, 2019 23:26

grorg approved these changes Apr 15, 2019

View reviewed changes

litherum force-pushed the storeops branch from 64a2617 to 91155a2 Compare April 15, 2019 23:27

kainino0x reviewed Apr 16, 2019

View reviewed changes

kainino0x mentioned this pull request May 31, 2019

Should uninitialized depth resources default to 1.0 or 0.0? #312

Closed

litherum force-pushed the storeops branch 2 times, most recently from 2480b7e to 61163f8 Compare July 2, 2019 21:48

store ops

d5c0c23

kainino0x force-pushed the storeops branch from 61163f8 to d5c0c23 Compare July 12, 2019 21:42

beaufortfrancois requested changes Jul 16, 2019

View reviewed changes

kainino0x closed this Jul 24, 2019

kainino0x added a commit to kainino0x/gpuweb that referenced this pull request Jul 24, 2019

Add a default for GPURenderPassColorAttachmentDescriptor.storeOp

eec68d4

Supersedes gpuweb#268.

kainino0x mentioned this pull request Jul 24, 2019

Add a default for GPURenderPassColorAttachmentDescriptor.storeOp #376

Merged

Kangz pushed a commit that referenced this pull request Aug 28, 2019

Add a default for GPURenderPassColorAttachmentDescriptor.storeOp (#376)

f3ea387

Supersedes #268.

JusSn pushed a commit to JusSn/gpuweb that referenced this pull request Sep 19, 2019

Add a default for GPURenderPassColorAttachmentDescriptor.storeOp (gpu…

ee0cd4f

…web#376) Supersedes gpuweb#268.

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

empty commit (gpuweb#268)

d72562e

	required GPUStoreOp storeOp = "store";
	GPUStoreOp storeOp = "store";

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kainino0x commented Apr 16, 2019

Uh oh!

kvark commented Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kainino0x commented Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kainino0x commented Jul 12, 2019

Uh oh!

beaufortfrancois left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvark commented Jul 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

litherum commented Apr 15, 2019 •

edited by pr-preview Bot

Loading

kvark commented Apr 22, 2019 •

edited

Loading

kainino0x commented Jul 1, 2019 •

edited

Loading