Add location decoration attribute

[msiglreith]

Allows to explicity specify locations decorations for shader interfaces.
It's implemented backwards compatible in the way that locations will still be incremented for relevant interface types but specifying it explicitly will adjust the counter.

Example

#[allow(unused_attributes)]
#[spirv(vertex)]
pub fn main_vs(
    #[spirv(location = 0)] v_pos_world: Input<f32x2>,
    #[spirv(location = 1)] v_pos_curve: Input<f32x2>,
    #[spirv(location = 2)] v_curve_range: Input<u32x2>,
    #[spirv(location = 1)] mut a_curve_range: Output<u32x2>, // manually reorder output attribute
    #[spirv(location = 0)] mut a_pos_curve: Output<f32x2>,
    #[spirv(position)] mut gl_Position: Output<f32x4>,
    #[spirv(location = 0)] u_viewport: UniformConstant<f32x4>, // on non-opaque uniform constants as well for GL
) {
... 

There is one case where this can lead to issues right now:

#[spirv(location = 1)] v_pos_world: Input<f32x2>,
#[spirv(location = 0)] v_pos_curve: Input<f32x2>,
v_curve_range: Input<u32x2>, // implicit location 1 -> conflicts with `v_pos_world`

[XAMPPRocky]

There is one case where this can lead to issues right now:
implicit location 1 -> conflicts with v_pos_world

Does this provide an error? Because if so, I think it's fine as is. This is also the case with enums in Rust, so having it behave similarly makes sense.

enum Foo {
    Bar = 1,
    Baz = 0,
    Test,
}

error[E0081]: discriminant value `1` already exists
 --> src/main.rs:4:5
  |
2 |     Bar = 1,
  |           - first use of `1`
3 |     Baz = 0,
4 |     Test,
  |     ^^^^ enum already has `1`

[msiglreith]

Does this provide an error? Because if so, I think it's fine as is. This is also the case with enums in Rust, so having it behave similarly makes sense.

I don't believe so, would probably clash after the build during validation or at runtime. But I can try to construct similar error outputs.

[khyperia]

I think I'd prefer this to be implemented in a way that either no arguments have an explicit location, or all of them do - mix and matching sounds like a really bad idea, and should be an error (I can't think of a good use case to do so). Also, having error checking for duplicate locations is important.

[khyperia]

@msiglreith Do you plan on updating this with the feedback, or should we just close this?

[msiglreith]

Oh sorry, gonna close it as I currently don't need it and interface might change in the future anyway

[expenses]

Hi, just popping in to say that this would be pretty useful for the specific case where you've got matrix attributes (e.g. for instancing).

at the moment if you have

#[spirv(vertex)]
pub fn vertex(
    position: Vec3,
    normal: Vec3,
    uv: Vec2,
    material: u32,
    translation: Vec3,
    rotation: Mat3,
    scale: f32,
    ...

Then you get a validation error:

UNASSIGNED-CoreValidation-Shader-InconsistentSpirv(ERROR / SPEC): msgNum: 7060244 - Validation Error: [ UNASSIGNED-CoreValidation-Shader-InconsistentSpirv ] Object 0: handle = 0x1d081379068, type = VK_OBJECT_TYPE_DEVICE; | MessageID = 0x6bbb14 | SPIR-V module not valid: Entry-point has conflicting input location assignment at location 6, component 0 OpEntryPoint Vertex %16 "vertex_instanced" %3 %4 %5 %10 %17 %18 %19 %11 %12 %13 %14 %gl_Position

as the location of scale is shared with rotation.

Having this would fix the problem:

#[spirv(vertex)]
pub fn vertex(
    position: Vec3,
    normal: Vec3,
    uv: Vec2,
    material: u32,
    translation: Vec3,
    rotation: Mat3,
    #[spirv(location = 8)] scale: f32,
    ...

The other way to handle this would be to skip some locations when an attribute is quite big, like a matrix.

[hrydgard]

Although if you are using Vulkan, instancing through attributes is essentially obsolete. Put all your instance data (matrices, whatever you want) in a storage buffer and index into it by instance ID, no need to bind extra vertex streams.

(Doesn't mean assigning locations manually is useless though, I'm sure there are cases where it's desirable).

[khyperia]

Just chiming in that the error around not properly auto-calculating location for large things (arrays, etc.) is known, and shouldn't be considered resolved by explicit location assignments - a fix was here, but unfortunately was closed due to inactivity (couldn't merge due to conflicts+failing tests, IIRC): #513

[expenses]

Oh nice. I might have a go at reviving that.