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

Do not require vertexInput in GPURenderPipelineDescriptor #378

Merged
merged 3 commits into from
Aug 27, 2019

Conversation

beaufortfrancois
Copy link
Contributor

@beaufortfrancois beaufortfrancois commented Jul 25, 2019

In the case of a vertex shader with no input,vertexInput doesn't make sense. It would be great if we wouldn't require it in GPURenderPipelineDescriptor.

I believe these "do not require X" PRs will improve the learning phase for developers interested in WebGPU:

  • Step 1: Draw a triangle (introducing vertexStage)
  • Step 2: Draw a colored triangle (introducing fragmentStage)
  • Step 3: Draw a triangle without hardcoding vertices values (introducing vertexInput)

Preview | Diff

@kvark
Copy link
Contributor

kvark commented Jul 25, 2019

I find it not very useful (given that most render pipelines will have vertex input) and not harmful at the same time :) So I'll abstain from the vote.

@magcius
Copy link

magcius commented Jul 25, 2019

It's helpful for the FS tri trick seen in postprocessing shaders ( https://web.archive.org/web/20140719063725/http://www.altdev.co/2011/08/08/interesting-vertex-shader-trick/ ). +1 from me.

@kvark
Copy link
Contributor

kvark commented Jul 25, 2019

@magcius if you know this trick, you are unlikely going to be confused or burdened to specify the empty vertex input declaration. It's not the point of the PR to support anything, the point, as I understand it, is to make the tutorials a tiny bit nicer.

@kainino0x
Copy link
Contributor

2019-08-26 meeting resolution: merge.

@kainino0x
Copy link
Contributor

Hm, if we do this then I think it also makes sense to change:

GPUVertexInputDescriptor vertexInput;

to

GPUVertexInputDescriptor vertexInput = {};

(and we need to do the same elsewhere), and change:

required sequence<GPUVertexBufferDescriptor?> vertexBuffers;

to

sequence<GPUVertexBufferDescriptor?> vertexBuffers = [];

@beaufortfrancois
Copy link
Contributor Author

@kainino0x I've added your suggestions to this PR. Thanks!

@kainino0x kainino0x merged commit cb3f5db into master Aug 27, 2019
@kainino0x kainino0x deleted the beaufortfrancois-patch-1 branch August 27, 2019 20:21
aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 30, 2019
Following WebGPU spec change at gpuweb/gpuweb#378,
vertexInput descriptor from GPURenderPipelineDescriptor should not be
required anymore.

Bug: 877147
Change-Id: Ifdd0ba7ff6af2b8648ad45d029e5ef7b8484f8a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1776023
Commit-Queue: François Beaufort <beaufort.francois@gmail.com>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#691978}
JusSn pushed a commit to JusSn/gpuweb that referenced this pull request Sep 19, 2019
JusSn added a commit that referenced this pull request Sep 19, 2019
* 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
@kainino0x kainino0x mentioned this pull request Oct 30, 2019
13 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.

None yet

4 participants