Add slot into VertexBuffer - #314

This is ok with me.

For history, see #249, #186 and the linked minutes for 2019-03-25 where we decided to use the sparse array.

This was indeed discussed quite a bit.

One motivation for making the slot explicit was to match the set_vertex_buffers call, where the slot is already implicit. If we expose the slot in one but not the other, it would be inconsistent and cause confusion.

I also don't buy the argument that "I think it's not very good for webgpu developers" with the provided examples (that are possible but hypothetical). I think we'll mostly see 2 scenarios:

1. hand-written code that does pipeline creation and vbuf binding, most likely using a few sequential slots starting from 0
2. data-driven code that knows about slots and figures out how to communicate this semantics to WebGPU. It's not complex:

var vertexBuffers = [];
for (i=0; i<myBuffers.length; ++i) {
  vertexBuffers[myBuffers[i].slot] = {
    ... // fills in the buffer entry
  };
}

This was indeed discussed quite a bit.

One motivation for making the slot explicit was to match the set_vertex_buffers call, where the slot is already implicit. If we expose the slot in one but not the other, it would be inconsistent and cause confusion.

I also don't buy the argument that "I think it's not very good for webgpu developers" with the provided examples (that are possible but hypothetical). I think we'll mostly see 2 scenarios:

1. hand-written code that does pipeline creation and vbuf binding, most likely using a few sequential slots starting from 0
2. data-driven code that knows about slots and figures out how to communicate this semantics to WebGPU. It's not complex:

var vertexBuffers = [];
for (i=0; i<myBuffers.length; ++i) {
  vertexBuffers[myBuffers[i].slot] = {
    ... // fills in the buffer entry
  };
}

Yes. Web developers can do this. However, this is wired that developers are using the "slot" in JS code (and in their mind), but try to find such a way to avoid "slot" because the interface doesn't provide it. It looks like a workaround for a bad design. I am not quite clear about the performance impact or other benefit about removing "slot" though.

Currently we 1) remove "slot", 2) tell web developers to use the index of the sequence as buffer slot in the upcoming formal spec (otherwise it's confusing if we don't clearly say this in spec), and 3) use such a code snippet to fill the hole because vertex buffer descriptor doesn't have an explicit "slot". This is not straight forward. Why not add "slot" in vertex buffer and don't beat around the bush...

@Richard-Yunchao

tell web developers to use the index of the sequence as buffer slot in the upcoming formal spec (otherwise it's confusing if we don't clearly say this in spec)

Could you clarify this bit? Unlike vertex attributes and resource bindings, there is no such semantics as "vertex buffer slot" at the shader level. It's all just on the API side: all we need to specify is that there is an array of vertex buffers expected by a pipeline, and this array is bound (with set_vertex_buffers) when one is recording a render pass. Introducing a "slot" semantic would make this slightly more complex. We'd have to validate the slots to not have collisions, also specify how the slots correspond to the arguments of set_vertex_buffers (which is the problem today as well, to an extent, since we can provide buffers starting from position N).

@Richard-Yunchao

tell web developers to use the index of the sequence as buffer slot in the upcoming formal spec (otherwise it's confusing if we don't clearly say this in spec)

Could you clarify this bit? Unlike vertex attributes and resource bindings, there is no such semantics as "vertex buffer slot" at the shader level. It's all just on the API side: all we need to specify is that there is an array of vertex buffers expected by a pipeline, and this array is bound (with set_vertex_buffers) when one is recording a render pass. Introducing a "slot" semantic would make this slightly more complex. We'd have to validate the slots to not have collisions, also specify how the slots correspond to the arguments of set_vertex_buffers (which is the problem today as well, to an extent, since we can provide buffers starting from position N).

Whoops. "vertex buffer slot" is not accurate. I was saying the slot at API side. What I want to say is: the fact that the index of sequence is used as the API side slot is obscure. We need to explicitly state that in a formal specification (right now the spec only lists interfaces without details, it is far from a formal spec).

Adding slot also has its downsides, just like you said. For example, we need to validate the slots don't have collisions. The upsides is that it is more straight-forward and user-friendly, and might save a little bit memory because we don't need to construct null objects. So it's a trade-off.

@Richard-Yunchao I'm still missing something from your argument.

What I want to say is: the fact that the index of sequence is used as the API side slot is obscure.

There is nothing to link to. There is a sequence provided when creating a pipeline, and there is a sequence passed when binding the vertex buffers. The only place where "slot" shows up is the start index for set_vertex_buffers, but this one is kinda tricky: if we convert the pipeline definition to use slots, then suddenly the sequence passed to set_vertex_buffers becomes obscuring...

Ultimately, there are 2 ways to make this entirely clean:

1. remove the start index from set_vertex_buffers, let the users to set them individually, just like they set the bind groups.
2. add "slot" to the vertex buffer definition of a pipeline, and also turn the argument of set_vertex_buffers from a sequence to a map that corresponds "slot" to a buffer.

I definitely prefer having the slot defined explicitly and without sparse arrays.

@kvark Is this what you're describing as option 2?

Although, rather than passing a map, why not pass a sequence of slots?

void setVertexBuffers(sequence<u32> inputSlots, sequence<GPUBuffer> buffers, sequence<u64> offsets);

dictionary GPUVertexBufferDescriptor {
    required u64 stride;
    required u32 inputSlot;
    GPUInputStepMode stepMode = "vertex";
    required sequence<GPUVertexAttributeDescriptor> attributeSet;
};

dictionary GPUVertexAttributeDescriptor {
    u64 offset = 0;
    required GPUVertexFormat format;
    required u32 shaderLocation;
};

Which would effectively be the same as this (although I'm not necessarily suggesting this):

dictionary SetVertexBufferDescriptor {
    u32 inputSlot;
    buffer GPUBuffer;
    offset u64;
}

void setVertexBuffers(sequence<SetVertexBufferDescriptor> buffers);

Discussed at 25 Jun 2019 teleconference

Resolution: close.