Vertex shader with multiple vertex data buffers outputs incorrect buffer indices in Metal backend

I've got this shader input structure:

public struct VertexInput
{
    [PositionSemantic] public Vector3 Position;
    [NormalSemantic] public Vector3 Normal;

    [TextureCoordinateSemantic] public Vector2 UV;
}

On the application side, the data comes from two separate buffers:

One buffer for Position and Normal
One buffer for UV

I also have some uniform buffers in this shader. To simplify, let's say there's one buffer named GlobalConstants.

ShaderGen writes out (something like) the following Metal code:

vertex VertexOutput VS(
    VertexInput input [[ stage_in ]],
    constant GlobalConstantsType &GlobalConstants [[ buffer(1) ]]) ...

The problem is in the binding location for GlobalConstants. It should be [[ buffer(2) ]], because the first two slots are taken up by the vertex data.

Here's the code responsible:

ShaderGen/src/ShaderGen/Metal/MetalBackend.cs

Lines 80 to 84 in 246e4b2

    
           if (function.Type == ShaderFunctionType.VertexEntryPoint 
        
               && function.Parameters.Length > 0) 
        
           { 
        
               bufferBinding = 1; 
        
           }

For this example, it needs to offset bufferBinding by 2, not 1.

I don't seen an obvious solution. Perhaps there could be an attribute on struct VertexInput to tell ShaderGen how many vertex buffers are used (with a default of 1 if there's no attribute)? Something like:

[VertexBufferCount(2)]
public struct VertexInput { ... }

Once we have a solution, I can do a PR for it.

This is a problem I've known about for a while. There's basically two solutions that I've thought of, but only one is suitable as a "quick fix".

The first is the solution you mentioned: adding an attribute which describes how many vertex buffers are used for the structure. This will work fine and will match how Veldrid currently expects things to be laid out. Unfortunately, it prevents you from using the same shader multiple times with a different number of vertex buffers. All the other language backends can work with a variable number of vertex buffers, because the binding space for them is different. In Metal all kinds of buffers (vertex, uniform, etc.) use the same binding space.

The second solution is one that is more elegant, but would be a breaking change and would need some kind of adaptation in Veldrid to allow it to continue working there. Instead of assuming the vertex buffers begin at index 0, we could instead place them at the END of the binding list. So a shader with 5 uniform buffers would have buffer indices 0, 1, 2, 3, and 4 assigned to the uniform buffer resources, and the [[stage_in]] buffers could instead be assigned to whatever you want. You could use buffer index 5, or 5/6, or 5/6/7, etc. This would break how Veldrid expects things, so there would need to be some kind of "switch" that you could activate to get this new behavior.

Semi-relatedly, I would also like the GLSL 300/330 backends to start using explicit binding indices for resources, rather than relying on the OpenGL code to assign them by name upon Pipeline creation. This could probably be done without breaking Veldrid at all (since it could just continue re-assigning them anyways), but Veldrid could also be augmented to directly support those explicit binding locations if the behavior was attached to that special "switch" mentioned above.

Going with the first solution could work for now, but I'm worried that it would become obsolete once I have time to implement the second feature. Luckily, adding the first one should only be a few lines of code (recognizing the attribute and then using the count in the place you mentioned).

Sorry about the wall of text 😆. I've thought about this a decent bit so I'd like to do things the right way (at least eventually).

The second solution does sound better. Should it be based on a switch in ShaderGen as well, to avoid breaking ShaderGen users?

(And any ideas on what the switch should be called? If it's going to cover GLSL explicit binding indices too, I'm struggling to think of a good name.)

Given that ShaderGen is still churning a lot, I think we should just make the change here without a switch. I would like to start thinking about a stable release at some point soon, though, because it's becoming increasingly important to my projects (and others' as well, I think).

	if (function.Type == ShaderFunctionType.VertexEntryPoint
	&& function.Parameters.Length > 0)
	{
	bufferBinding = 1;
	}