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

Add WGSL examples to documentation #2888

Merged
merged 17 commits into from Aug 2, 2022
Merged

Add WGSL examples to documentation #2888

merged 17 commits into from Aug 2, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jul 15, 2022

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.
  • Add Features example(s).
  • Add BufferBindingType example(s).
  • Add BindingType example(s).
  • Add StorageTextureAccess example(s).
  • Add TextureSampleType example(s).
  • Add TextureViewDimension example(s).

Connections
Partly addresses #2849.

Description
Adds WGSL examples to wgpu documentation to complement the existing examples written in GLSL.

Testing
Run cargo doc to observe that the documentation is updated, and compile the WGSL examples (e.g. with naga) to confirm they are valid.

@ghost
Copy link
Author

ghost commented Jul 15, 2022

Hm. Comparing the WGSL and GLSL specs, it seems that WGSL is far more restrictive than GLSL in terms of access modes. For example, according to this table, I don't think it's possible to define a read-write or write-only uniform, texture, or sampler. However, I see no such restrictions in this rather minimal explanation of access modes offered by the GLSL spec.

If my understanding is correct, then some documentation items may be necessarily devoid of a WGSL example due to the limitations of the language. In particular, I think this is the case with StorageTextureAccess::WriteOnly and StorageTextureAccess::ReadWrite.

@ghost
Copy link
Author

ghost commented Jul 15, 2022

I have added explicit readonly annotations to some of the GLSL examples. This is because I do not want to harbor any confusion regarding WGSL vs. GLSL access modes. GLSL is read-write by default in all address spaces and, as explained in a previous comment above, some WGSL address spaces only allow read-only. Thus, for WGSL variables declared in such read-only address spaces, I have added readonly to the GLSL example such that the two examples are semantically identical.
readonly is unnecessary because, while GLSL is technically read-write by default, this is (presumably) automagically coerced to readonly for uniforms.

@cwfitzgerald
Copy link
Member

GLSL may allow you to put the keywords there, but uniform buffers, textures, and samplers are always read-only, there is no way to actually write to them. In those examples, I think it's perfectly fine to do normal wgsl declarations.

We offer read-write and read storage texture support as a native extension, so using a wgsl example (using the extension) is totally fine.

@ghost
Copy link
Author

ghost commented Jul 15, 2022

GLSL may allow you to put the keywords there, but uniform buffers, textures, and samplers are always read-only, there is no way to actually write to them. In those examples, I think it's perfectly fine to do normal wgsl declarations.

Ah. I will remove the explicit readonly annotations then because it should be read-only for both WGSL and GLSL.

We offer read-write and read storage texture support as a native extension, so using a wgsl example (using the extension) is totally fine.

Hm. naga accepts that? I don't think the WGSL syntax technically permits that, even if it is possible hardware-wise. Should it be noted somewhere that that's not an official WGSL extension (the only one I see is f16 support) but is supported by wgpu/naga?

@ghost
Copy link
Author

ghost commented Jul 15, 2022

As an aside: I think some of my confusion arose from storage textures, as defined by the WGSL spec, being write-only, but their handles being read-only. Apparently there is an issue to clarify this already: gpuweb/gpuweb#3047.

@cwfitzgerald
Copy link
Member

Should it be noted somewhere that that's not an official WGSL extension (the only one I see is f16 support) but is supported by wgpu/naga?

Wouldn't hurt, though we already do this to an extent by separating "native" and "webgpu" extensions.

@ghost ghost marked this pull request as ready for review July 16, 2022 16:32
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

By and large looks great, thank you! Some nits

wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
wgpu-types/src/lib.rs Outdated Show resolved Hide resolved
ani and others added 4 commits July 17, 2022 14:14
Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

Thanks!

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) July 19, 2022 03:28
@ghost
Copy link
Author

ghost commented Jul 19, 2022

It seems rustfmt does not like the two trailing spaces after 'ex.'. I'm not sure which lint is causing that, but we could replace the double-space line breaks with backslashes to escape the newline.

@cwfitzgerald
Copy link
Member

Yeah that would work - sorry for the delay in responding.

auto-merge was automatically disabled August 2, 2022 17:20

Head branch was pushed to by a user without write access

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) August 2, 2022 19:13
@cwfitzgerald cwfitzgerald merged commit 29f5f8f into gfx-rs:master Aug 2, 2022
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.

1 participant