Bindings proposal

[Jasper-Bekkers]

This uses the template in #18, it's a small proposal to get the discussion started on how we want to end up doing bindings in this project.

I'm personally not entirely keen on the proposed syntax; at least the RuntimeArray type, for example, I wouldn't want to have if we can get rid of it -it's pretty closesly modeled after SPIR-V directly for example and is quite verbose.

Things to also think about is how do we access buffers and textures - but that can be another RFC after we agree on this.

[Jasper-Bekkers]

Rendered: https://github.com/EmbarkStudios/rust-gpu/blob/binding-rfc/rfcs/001-resource-binding-syntax.md

[khyperia]

At first glance, declaring globals with attributes seems more flexible than implicitly deriving data from a location in a parameter list - probably want to explore that option in the Alternatives section?

[Tobski]

I have been talking to @Jasper-Bekkers about the "struct arguments" option and IMO that's the most ergonomic and future proof option - it matches what most consider "state of the art" in this language design space AFAIK. Also there's nothing that really needs to change to enable descriptor indexing; it's based only on how the shader is coded. For instance:

struct ShadingInputs {
    albedo: Texture::<f32>,
    normal_map: Texture::<f32>,
    smoothness: Texture::<f32>,
}

fn main(inputs: &ShadingInputs[3]) {
    let mut T = brdf(&inputs[0]);
}

Enabling an array here works all the way back to Vulkan 1.0, assuming static indexing. Dynamic indexing could be made to work on Vulkan 1.0 for a subset of functionality - but it would involve putting switch statements into the generated spir-v. This might instead serve as a useful cutoff for requiring the dynamic indexing extension though, assuming this can be detected. Arrays could also just be the deciding factor here.

The only wrinkle is that in Vulkan this would mean translating each element of the struct to an array individually, but that shouldn't be too hard to deal with?

So I guess that option has my vote at least 😅

[Tobski]

One other thing we talked about was inline constants in those inputs, a la:

struct ShadingInputs {
    albedo: Texture::<f32>,
    normal_map: Texture::<f32>,
    smoothness: Texture::<f32>,
    someParameter: f32,         // <------ LOOK HERE
}

fn main(inputs: &ShadingInputs[3]) {
    let mut T = brdf(&inputs[0]);
}

There's a Vulkan extension - VK_EXT_inline_uniform_block - which lets you put those directly in blocks in descriptor sets. Using that is an option, but as with dynamic indexing, it doesn't have wide support on mobile; and even desktop isn't fully covered.

The fallback would be to reserve a single uniform buffer (cbuffer) binding to collect any inline constants and source them from there - this will require some remapping, but it seems like it'd be feasible.

[Jasper-Bekkers]

At first glance, declaring globals with attributes seems more flexible than implicitly deriving data from a location in a parameter list - probably want to explore that option in the Alternatives section?

In my experience, in larger code-bases (100k-1mln lines of GPU code) having globals becomes a /huge/ mess. You don't end up known what uses them, which ones are available, which ones should be bound under what conditions etc. The biggest thing is when you're modifying some shader code, and you decide to invoke a function somewhere which in one of the leaf-functions that it calls suddenly needs a new binding to be available but you don't know because it's a global.

In smaller shaders where this is all in one file that doesn't really matter, but if you're talking about having libraries of shaders that are potentially published and created by third-party users it quickly becomes a thing if all bindings are implicit like that.

I can add this argument to the RFC directly too, since I think it ties into our higher level goals quite a bit more too.

[khyperia]

Does anyone have objections to this proposal? Let's merge it if it's roughly on track with what we want.

I'm pretty happy with it, aside from some minor nitpicks, e.g. I think using attributes instead of constructor arguments for the globals case would be much better, but, that's a minor detail that can be changed later.

[Jasper-Bekkers]

I'm pretty happy with it, aside from some minor nitpicks, e.g. I think using attributes instead of constructor arguments for the globals case would be much better, but, that's a minor detail that can be changed later.

Just to be clear, the proposal recommends against having global bindings.

The constructors are there because rust wants these variables to be initialized.

[nhaehnle]

I like it. It would be good to see the CPU-side as well, though: ultimately, the bindings are about tying the CPU and the GPU together, so it's kind of odd to see only one side here ;)

[khyperia]

Actually, in the parameters proposal, how do you expect attributes like Flat, NoPerspective, or Centroid to be applied? What about Index? DescriptorSet and Offset? And others that I likely missed when scanning through the spec

[Jasper-Bekkers]

I like it. It would be good to see the CPU-side as well, though: ultimately, the bindings are about tying the CPU and the GPU together, so it's kind of odd to see only one side here ;)

I've left the CPU side out for now because we haven't discussed the CPU part of the API at all yet, most of that will have to come once we start on our rendering abstraction / render graph, but it definitely warrants attention. My ultimate hope for this would be that this is relatively minimal / easy to use at the start, I'd love the CPU side to closely represent a function call, or at least represent cuLaunchKernel in terms of ease of use.

Actually, in the parameters proposal, how do you expect attributes like Flat, NoPerspective, or Centroid to be applied? What about Index? DescriptorSet and Offset? And others that I likely missed when scanning through the spec

I'll add something to the doc, my initial suggestion would be to have #[flat], #[noperspective] and #[centroid] on members. Have all auto-generated DescriptorSet and Index. Right now the proposal doesn't allow for regular data to be passed in along with bindings like @Tobski suggested so I don't think Offset would be needed yet.

Edit:

We'll probably need Location as well to pass data between stages, so I'll add a suggestion for that too, if we need it.

[fu5ha]

A few thoughts:

Have all auto-generated DescriptorSet and Index

Is the idea here to basically just map each "ShaderInput struct" as a DescriptorSet and each item within as an Index? And then the indexing/order of these DescriptorSets is implicitly defined by their parameter ordering within the shader function?

If so, two concerns:

1. Are we going to be deriving semantic meaning (i.e. being able to match shader input structs with CPU-side bindings) from these through only the CPU-side (reflection?) implementation?
2. How does this work with inter-stage arguments? I guess a fairly logical extension of the current syntax would be to add a restriction that stage inputs be defined in a separate struct which is always the first argument to the main function, and then stage outputs are defined in a separate struct that is the return type of the main function.

struct StageInputs {
    vs_world_pos: Vec3,
    #[flat]
    vs_material_index: usize,
}
    
struct ViewGlobals {
    model: Mat4,
    view_proj: Mat4,
}
    
struct MaterialInputs {
    albedo: Texture<f32>,
    normal_map: Texture<f32>,
    smoothness: Texture<f32>,
    someParameter: f32,
}

struct StageOutputs {
    out_color: Vec4, // potentially add an RGBA type? 
}

fn main(
    stage_inputs: &StageInputs,
    view_inputs: &ViewGlobals,
    material_inputs: &MaterialInputs[3]
) -> StageOutputs {
    // this would require an extension for dynamic indexing
    let mut col = brdf(&material_inputs[stage_inputs.vs_material_index]);

    StageOutputs {
        out_color: col,
    }
}

This ends up feeling pretty similar to the way HLSL handles stage inputs/outputs, which I think is positive because it's both IMO nice to use and is familiar to many people already.

[Jasper-Bekkers]

Is the idea here to basically just map each "ShaderInput struct" as a DescriptorSet and each item within as an Index? And then the indexing/order of these DescriptorSets is implicitly defined by their parameter ordering within the shader function?

Since several platforms have a very low maximum number of descriptor sets that can be bound, my initial suggestion would be to bind them all to descriptor set 0.

Are we going to be deriving semantic meaning (i.e. being able to match shader input structs with CPU-side bindings) from these through only the CPU-side (reflection?) implementation?

Essentially yes - I would love for a follow-up PR to potentially propose a #[descriptor_set(23)] attribute on structs.

How does this work with inter-stage arguments? I guess a fairly logical extension of the current syntax would be to add a restriction that stage inputs be defined in a separate struct which is always the first argument to the main function, and then stage outputs are defined in a separate struct that is the return type of the main function.

In this proposal I didn't include "other data types" other then bindings (e.g. floats between bindings etc) on purpose, same for inter-stage arguments. I think those are important but potentially require a follow-up PR where we think through those a bit more.

For example, if we want to have types of data between the bindings in ShaderInput structs, we'll need some story for constant buffers and filling them as well (potentially also in the runtime).

Same for inter-stage arguments; do we want to take control of packing these (optimizations possible) into vec4's or do we let the user do that. Can those structs also contain bindings (does it even make sense to), etc.