[PATCH v3 5/5] wined3d: Create a null vertex binding and attributes for unbound shader inputs.

[Jan Sikorski]

> On 20 Apr 2021, at 19:25, Henri Verbeet <hverbeet at gmail.com> wrote:
> 
> On Tue, 20 Apr 2021 at 15:08, Jan Sikorski <jsikorski at codeweavers.com> wrote:
>> @@ -2071,6 +2072,46 @@ static bool wined3d_context_vk_update_graphics_pipeline_key(struct wined3d_conte
>>             }
>>         }
>> 
>> +        if (attribute_count < signature->element_count)
>> +        {
> I don't think that check is safe. Specifically, there may be multiple
> input signature elements for the same input register. (E.g., mapping
> v0.xy to "COLOUR0", and v0.zw to "COLOUR1".)
> 
>> +                for (i = 0; i < signature->element_count; ++i)
>> +                {
>> +                    struct wined3d_shader_signature_element *element = &signature->elements[i];
>> +                    uint32_t location = element->register_idx;
>> +                    VkVertexInputAttributeDescription *a;
>> +
>> +                    if ((stream_info.use_map & (1 << location)))
>> +                        continue;
>> +
>> +                    a = &key->attributes[attribute_count++];
>> +                    a->location = location;
>> +                    a->binding = null_binding;
>> +                    a->format = VK_FORMAT_R32G32B32A32_UINT;
>> +                    a->offset = 0;
>> +                }
> Similarly, this may end up creating multiple attribute descriptions
> for the same input location if we don't keep track of which ones we've
> already processed here. We should probably skip elements with
> "sysval_semantic" set, although I wouldn't expect anything bad to
> happen if we don't.
> 
> Somewhat similar to my comment to patch 4/5, we could alternatively
> use the "shader->reg_maps.input_registers" bitmap by doing something
> like "uint32_t map = vs->reg_maps.input_registers &
> ~stream_info.use_map;", and then iterating over that map.

Right, thanks for pointing that out.
Another thing I noticed is that I should set the attribute format according to component type, or else the validation layer complains about mismatch. Luckily that’s straightforward and I don’t think I need to match number of components.
There’s also an issue that wined3d_context_vk_bind_vertex_buffers() only gets called if STATE_STREAMSRC is dirty, while now a pipeline change might necessitate adding the null binding.
Calling it each time the pipeline changes would be wasteful. It also smells fishy to invalidate STATE_STREAMSRC inside wined3d_context_vk_update_graphics_pipeline_key for that case, although I guess it would work. I can think of a few ways to resolve it, my favourite is to directly bind the null buffer next to vkCmdBindPipeline() if we need one (and perhaps if STATE_STREAMSRC is clean). We’d probably still need to do the thing in wined3d_context_vk_bind_vertex_buffers() in case someone binds a NULL vertex buffer. Although looks like now if STATE_STREAMSRC is dirty we always go to vkCmdBindPipeline(), so maybe not, but that's a bit brittle.

- Jan