rust-gpu icon indicating copy to clipboard operation
rust-gpu copied to clipboard

Add arrays of buffer descriptors

Open Firestar99 opened this issue 1 year ago • 2 comments

This PR adds support for arrays of buffer descriptors, and represents them like this in rust-gpu:

#[spirv(fragment)]
pub fn main(
    #[spirv(descriptor_set = 0, binding = 4, storage_buffer)] sized_buffer: &mut RuntimeArray<SizedBuffer>,
    #[spirv(descriptor_set = 0, binding = 5, storage_buffer)] unsized_buffer: &mut RuntimeArray<[u32]>,
) {}

This is a breaking change to how RuntimeArray operates. However, on all previous versions using it in such a way would result in a warning similarly to below. So I'm hoping this should have discouraged anyone from using it like that, and this change does not actually break anyone' code.

  warning: use &[T] instead of &RuntimeArray<T>
   --> example-shader/src/image_shader.rs:8:78
    |
  8 |     #[spirv(descriptor_set = 0, binding = 0, storage_buffer)] _sized_buffer: &mut RuntimeArray<SizedBuffer>,
    |                                                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  warning: `example-shader` (lib) generated 1 warning

But while trying to record that warning, I've noticed that even with a #![deny(warnings)] in the lib.rs of my shader crate, it would still only emit as a warning, thus when building via a build script not even show up to the end user.

Background

Currently we can represent the following glsl buffer declarations in rust-gpu:

layout(set = 0, binding = 0) buffer SizedBuffer {
    Mat3 model_matrix;
    uint id;
} sizedBuffer;

layout(set = 0, binding = 1) buffer UnsizedBuffer {
    uint data[];
} unsizedBuffer;

layout(set = 0, binding = 2) uniform image2D singleImage;
layout(set = 0, binding = 3) uniform image2D manyImages[];

pub struct SizedBuffer {
    pub model_matrix: glam::Mat3,
    pub id: u32,
}

#[spirv(fragment)]
pub fn main(
    #[spirv(descriptor_set = 0, binding = 0, storage_buffer)] sized_buffer: SizedBuffer,
    #[spirv(descriptor_set = 0, binding = 1, storage_buffer)] unsized_buffer: &mut [u32],
    #[spirv(descriptor_set = 0, binding = 2)] single_image: &Image!(2D, type=f32, sampled),
    #[spirv(descriptor_set = 0, binding = 3)] many_images: &RuntimeArray<Image!(2D, type=f32, sampled)>,
) {}

While we can have arrays of image or sampler descriptors, we cannot represent arrays of buffer descriptors, like we can in glsl. The proposed syntax for rust-gpu at the very top of this PR equals the following in glsl:

layout(set = 0, binding = 4) buffer SizedBuffer {
   Mat3 model_matrix;
   uint id;
} manySizedBuffers[];

layout(set = 0, binding = 5) buffer UnsizedBuffer {
   uint data[];
} manyUnsizedBuffers[];

Firestar99 avatar Apr 30 '24 16:04 Firestar99

About concerns about breaking change, To me that warning is basically same as deprecated, use this instead. so I think it's fine to make breaking change.

tmvkrpxl0 avatar Jun 18 '24 03:06 tmvkrpxl0

Sorry if you didn't see this PR (which I should've landed a while back, oops!):

  • #1014

Crucially, it fully disambiguates the existence of a "buffer resource", and while it doesn't require it in the simple case, it is pretty important semantically.

The reason the SPIR-V types are weird and Block is needed, is because an OpTypeStruct with Block is closer to an OpTypeImage resource than to an actual data type. I really wish they had used pointer types instead, but it is what it is.

Using pointers (and ignoring the missing length for the outer one), the type of an array of buffer would be something like &[&[u32]], with the outer & being in the same address space as e.g. image types (UniformConstant in SPIR-V, WGSL does this better and calls it "handle" IIRC), and only the inner & being in the StorageBuffer address space (and pointing into the buffer).

Also, as another aside, for mutable buffers, the correct type would have to be something like &[&[AtomicU32]], because the outer & points into read-only memory (which just contains the buffer handles aka "descriptors", and if you could write to that memory, you could be corrupting the actual buffer handle representation etc.).

But for now, #1014 should work without breaking changes or ambiguous situations etc.

eddyb avatar Jul 11 '24 11:07 eddyb