-
Notifications
You must be signed in to change notification settings - Fork 345
[wgsl] Adding textures to the spec. #909
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
Conversation
This CL starts to add textures into the WGSL spec. Issue #573
dneto0
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass. Thanks for progressing this!
wgsl/index.bs
Outdated
| ## Texture Functions ## {#texture-functions} | ||
|
|
||
| <pre class='def'> | ||
| vec4<type> texture_load(texture_ro, f32 | vec(2|3)<f32> coords, i32 level_of_detail) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need a function to fetch a single texel from a sampled image.
In the spirit of not overloading names, I suggest this:
vec4<type> texture_fetch(texture_sampled_*, f32 | vec(2|3)<f32> coords)
(Level of detail is not used, so no explicit LOD param, no LOD-bias, no explicit gradient).
Maps to OpImageFetch in SPIR-V. Can't be a cube image of any form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also want a function to get a single sample from a multi-sampled image.
(See "Sample" image operand in SPIR-V)
Suggest:
vec4<type> texture_fetch_sample(texture_sampled_2d_ms, vec2<f32> coords, u32 sample_index)
vec4<type> texture_load_sample(texture_ro_2d_ms, vec2<i32> coords, u32 sample_index)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dneto0 do we really need different methods here? Since the semantics of them is the same, we could have the same texture_load() function working (overloaded) with both texture_sampled* and texture_ro. Would this be a concern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added todos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overloading on the texture type works for me.
wgsl/index.bs
Outdated
|
|
||
| <pre class='def'> | ||
| vec4<type> texture_load(texture_ro, f32 | vec(2|3)<f32> coords, i32 level_of_detail) | ||
| %32 = OpImageFetch %v4float %31 %29 Lod %int_2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: allow a small constant offset to the (non-arrayed) coordinate for image-sample operations. Not allowed for cube images.
See SPIR-V "ConstOffset" image operand.
From MSL:
The texture sample, sample_compare, gather, and gather_compare functions take an
offset argument for a 2D texture, 2D texture array, and 3D texture. The offset is an integer
value applied to the texture coordinate before looking up each pixel. This integer value can be
in the range -8 to +7. The default value is 0.
Vulkan has similar limits. See minTexelOffset and maxTexelOffset limits (-8 and +7 respectively).
Must be a compile-time constant for SPIR-V.
(There is an "Offset" and "ConstOffsets" variant which are only used with image gather operations)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a texture_load_offset(texture_ro, f32 | vec(2|3)<f32> coords, i32 level_of_detail, i32 offset) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs some investigation on how portable this is. I guess it's good, but we need to double-check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added todo
wgsl/index.bs
Outdated
| vec4<type> texture_sample(texture_sampled, sampler, f32 | vec(2|3)<f32> coords) | ||
| %24 = OpImageSampleImplicitLod %v4float %19 %23 | ||
|
|
||
| vec4<type> texture_sample_level(texture_sampled, sampler, f32 | vec(2|3)<f32> coords, f32 lod) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Provide the level-of-detail via an explicit gradient. See "Grad" image operand in SPIR-V, and gradient2d constructor in MSL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Investigation: SPIR-V has a MinLod image operand to clamp the minimum computed level-of-detail.
But that's an optional Vulkan feature "shaderResourceMinLod", and that has almost no support in the Android ecosystem.
https://vulkan.gpuinfo.org/listdevicescoverage.php?feature=shaderResourceMinLod&platform=android
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added TODO
wgsl/index.bs
Outdated
| vec4<type> texture_sample_level(texture_sampled, sampler, f32 | vec(2|3)<f32> coords, f32 lod) | ||
| %25 = OpImageSampleExplicitLod %v4float %19 %23 Lod %float_0 | ||
|
|
||
| vec4<type> texture_sample_bias(texture_sampled, sampler, f32 | vec(2|3)<f32> coords, f32 bias) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The level-of-detail bias is only usable in fragment shaders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide some references? I only see this:
A following operand is the bias added to the implicit level of detail. Only valid with implicit-lod instructions. It must be a floating-point type scalar. This can only be used with an OpTypeImage that has a Dim operand of 1D, 2D, 3D, or Cube, and the MS operand must be 0.
If you are talking about the limitation that an implicit lod is only available in fragment shaders, than yes, but it also applies to the regular texture_sample
kvark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for writing this down! A few notes below
wgsl/index.bs
Outdated
|
|
||
| <pre class='def'> | ||
| vec4<type> texture_load(texture_ro, f32 | vec(2|3)<f32> coords, i32 level_of_detail) | ||
| %32 = OpImageFetch %v4float %31 %29 Lod %int_2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs some investigation on how portable this is. I guess it's good, but we need to double-check
wgsl/index.bs
Outdated
| vec4<type> texture_sample_level(texture_sampled, sampler, f32 | vec(2|3)<f32> coords, f32 lod) | ||
| %25 = OpImageSampleExplicitLod %v4float %19 %23 Lod %float_0 | ||
|
|
||
| vec4<type> texture_sample_bias(texture_sampled, sampler, f32 | vec(2|3)<f32> coords, f32 bias) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide some references? I only see this:
A following operand is the bias added to the implicit level of detail. Only valid with implicit-lod instructions. It must be a floating-point type scalar. This can only be used with an OpTypeImage that has a Dim operand of 1D, 2D, 3D, or Cube, and the MS operand must be 0.
If you are talking about the limitation that an implicit lod is only available in fragment shaders, than yes, but it also applies to the regular texture_sample
wgsl/index.bs
Outdated
| `texture_ro_3d<image_storage_type>` | ||
| %1 = OpTypeImage %type 3D 0 0 0 2 Unknown | ||
|
|
||
| `texture_ro_cube<image_storage_type>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in HLSL, cube textures can't do either loads or stores.
I think we just need them for sampled variants, therefore, not for storages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, no depth variant either (looks like it from that cube texture page)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HLSL doesn't need depth variants, but cubes in HLSL do support SampleCmp, so we should allow that too
wgsl/index.bs
Outdated
| `texture_ro_3d<image_storage_type>` | ||
| %1 = OpTypeImage %type 3D 0 0 0 2 Unknown | ||
|
|
||
| `texture_ro_cube<image_storage_type>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, no depth variant either (looks like it from that cube texture page)
| `texture_ro_1d_array<image_storage_type>` | ||
| %1 = OpTypeImage %type 1D 0 1 0 2 Unknown | ||
|
|
||
| `texture_wo_2d<image_storage_type>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set the types here to be void as they can't be sampled or read.
kvark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are almost there!
kvark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good, just need to get some more input on
- access flags instead of separate types, to avoid ro/wo/rw
textureshorthand fortexture_sampled
|
Discussed at the 2020-07-14 meeting. |
kdashg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This moves us forward, and we should investigate different naming solutions separately.
|
Resolution was to accept this PR and open new issues. |
|
Filed #923 for the question of naming. |
This CL starts to add textures into the WGSL spec.
Issue #573