floooh / sokol-tools

Command line tools for use with sokol headers
MIT License
229 stars 57 forks source link

Validation Error: D3D11 missing vertex attribute semantics in shader #68

Closed DctrNoob closed 2 years ago

DctrNoob commented 2 years ago

Hi,

I get the validation error "D3D11 missing vertex attribute semantics in shader" when compiling the following shader for HLSL5 using D3D under Windows.

@module debug

// Uniform type definitions on CPU side
@ctype mat4 ab::mat4_t
@ctype vec2 ab::vec2_t
@ctype vec3 ab::vec3_t
@ctype vec4 ab::vec4_t

// Vertex shader
@vs vs
layout(binding = 0) uniform vs_uniforms {
    mat4 u_projMat;
    mat4 u_modelMat;
};

layout(location = 0) in vec2 a_position;
layout(location = 1) in vec2 a_texCoords;
layout(location = 2) in vec4 a_color;

out vec4 v_color;

void main() {
  v_color      = a_color;
  gl_Position  = u_projMat * u_modelMat * vec4(a_position, 0, 1);
  gl_PointSize = 10.0;
}
@end

// Fragment shader
@fs fs
in vec4 v_color;

out vec4 out_color;

void main() {
    out_color = v_color;
}
@end

#pragma sokol @program prog vs fs

I debugged the problem a bit and found that the generated shader description is problematic:

  if (backend == SG_BACKEND_D3D11) {
    static sg_shader_desc desc;
    static bool valid;
    if (!valid) {
      valid = true;
      desc.attrs[0].sem_name = "TEXCOORD";
      desc.attrs[0].sem_index = 0;
      desc.attrs[2].sem_name = "TEXCOORD";
      desc.attrs[2].sem_index = 2;
      desc.vs.source = debug_vs_source_hlsl5;
      desc.vs.d3d11_target = "vs_5_0";
      desc.vs.entry = "main";
      desc.vs.uniform_blocks[0].size = 128;
      desc.vs.uniform_blocks[0].layout = SG_UNIFORMLAYOUT_STD140;
      desc.fs.source = debug_fs_source_hlsl5;
      desc.fs.d3d11_target = "ps_5_0";
      desc.fs.entry = "main";
      desc.label = "debug_prog_shader";
    }
    return &desc;
  }

The problem, it seems, is that the texture coordinates are unused in this particular shader which yields an incomplete semantic description:

      desc.attrs[0].sem_name = "TEXCOORD";
      desc.attrs[0].sem_index = 0;
      desc.attrs[2].sem_name = "TEXCOORD";
      desc.attrs[2].sem_index = 2;

For example, if I manually add

      desc.attrs[1].sem_name = "TEXCOORD";
      desc.attrs[1].sem_index = 1;

then the validation passes and everything runs as expected. These manually added lines are actually correctly generated if I enforce usage of the texture coordinates in the shader. For obvious reasons, I don't want to do that. Also, the shader is perfectly accepted when having a GL or Metal backend.

Is this a straight up bug or does somebody have a painless workaround?

Thanks!

floooh commented 2 years ago

This looks like a bug in sokol-shdc and is probably triggered because the SPIRVTools optimizer is more 'aggressive' about removing unused things since the last update.

The sokol-shdc bug is that this array shouldn't have a gap:

      desc.attrs[0].sem_name = "TEXCOORD";
      desc.attrs[0].sem_index = 0;
      desc.attrs[2].sem_name = "TEXCOORD";
      desc.attrs[2].sem_index = 2;

...it should look like this:

      desc.attrs[0].sem_name = "TEXCOORD";
      desc.attrs[0].sem_index = 0;
      desc.attrs[1].sem_name = "TEXCOORD";
      desc.attrs[1].sem_index = 2;

I'll have a look...

PS: No wait, it's not as simple as that, because the CPU-side code (rightly) expects that there are 3 vertex attributes, it shouldn't need to know that one attribute was dropped by the shader compiler (only the sokol-gfx backend needs to care about this when matching the vertex attributes from the shader descriptions against the 'expected' vertex layout in the pipeline description). This looks more like a bug in the sokol-gfx D3D backend. In any case I'll have a look (and also check that the Metal and GL backends do the right thing).

PPS: I'll move the ticket over to the sokol repository.

floooh commented 2 years ago

...hm turns out it's a bit more complicated... there's also a problem on the sokol-shdc side, that there's no attribute location define generated for the dropped attribute (which figures, because at the time when the shader reflection is read, the attribute already no longer exists):

#define ATTR_debug_vs_a_position (0)
#define ATTR_debug_vs_a_color (2)

...this should be:

#define ATTR_debug_vs_a_position (0)
#define ATTR_debug_vs_a_texcoords (1)
#define ATTR_debug_vs_a_color (2)

...I may need to disable that particular SPIRVTools optimizer pass (hopefully it is one specific pass). But I'll look into the D3D backend first, it should accept gaps in the shader vertex attribute description.

DctrNoob commented 2 years ago

@floooh Thanks for looking into it.

floooh commented 2 years ago

Hmm... I may need to paddle back on the "the shader desc vertex description doesn't need to match the pipeline vertex layout", while this seems to work on GL, Metal and D3D11 (the error you're seeing is just because of the sokol-gfx validation layer), this may not be true in WebGPU, which has very strict validation.

This area is currently too weakly defined in sokol-gfx, I would prefer that the shader vertex layout doesn't need to be a perfect match, but instead just a "subset" of the pipeline vertex layout, but I need to defer this decision until I get around to check that in the WIP sokol-gfx WebGPU backend.

Until then I'll keep the sokol-gfx behaviour as it is, and instead try to fix the problem in sokol-shdc.

floooh commented 2 years ago

...moving the ticket back to sokol-tools :)

floooh commented 2 years ago

Ok, I have found a good solution. The whole issue is caused by the SPIRVTools optimizer function CreateAggressiveDCEPass(). This doesn't just remove dead code, but also unused 'shader interface items'. There's a second version of this function which takes a bool param which controls whether unused interface items should be removed, and I'm calling that now.

This way, unused vertex attributes (and I guess also other things, like texture samplers) are not removed from the shader, so that the generated sokol-gfx shader desc looks as expected.

The question whether the vertex attributes in the shader desc should perfectly match the pipeline vertex layout is a different problem which I need to tackle later (but needs to be decided until the WebGPU backend is merged).

floooh commented 2 years ago

PS: the sokol-shdc binaries are currently building, you can try them when this job has finished:

https://github.com/floooh/sokol-tools/actions/runs/2085303355

...if you are using the fips integration for sokol-shdc, you'll need to run a './fips clean' on the project, so that the generated sources are re-generated (I forgot to bump the version stamp).

floooh commented 2 years ago

...oh great, another python vs python3 problem in the Github Actions job, hrmpf. I'll take care of that now.

floooh commented 2 years ago

...ok new attempt, when this is done successfully, you can update sokol-tools-bin to get the new executables:

https://github.com/floooh/sokol-tools/actions/runs/2085353842

DctrNoob commented 2 years ago

@floooh Great. Thanks for all the effort!

DctrNoob commented 2 years ago

So I tried every one of my builds now and the original issue with D3D is gone with the new executables. I also don't notice any issues with the Metal backend. However, with the GL(CORE33) backend, I now get the following message during pipeline creation:

Vertex attribute not found in shader: a_texCoords

Apparently, this means that glGetAttribLocation failed for a_texCoords though I don't notice any visual issues afterwards. The generated glsl330 snippet looks like this:

    #version 330

    uniform vec4 vs_uniforms[8];
    out vec4 v_color;
    layout(location = 2) in vec4 a_color;
    layout(location = 0) in vec2 a_position;
    layout(location = 1) in vec2 a_texCoords;

    void main()
    {
        v_color = a_color;
        gl_Position = (mat4(vs_uniforms[0], vs_uniforms[1], vs_uniforms[2], vs_uniforms[3]) * mat4(vs_uniforms[4], vs_uniforms[5], vs_uniforms[6], vs_uniforms[7])) * vec4(a_position, 0.0, 1.0);
        gl_PointSize = 10.0;
    }

which looks ok despite the somewhat weird attribute order.

floooh commented 2 years ago

Hm ok, the message is from here:

https://github.com/floooh/sokol/blob/a18031313cdf7dab461baa296c38b07337ab4d47/sokol_gfx.h#L6866-L6869

Happens when glGetAttribLocation() returns -1. I think this happens because the unused vertex attribute is now removed by the shader compiler of the OpenGL driver. There's not much that can be done except removing that warning message.

DctrNoob commented 2 years ago

Alright, makes sense, thanks!