Change how GraphicsPipelineInfo::add_vertex_attribute works

[Silverlan]

I feel like the way add_vertex_attribute works at the moment is very arbitrary, counter-intuitive and it actually makes certain cases impossible as far as I can tell.

Example:

struct Vertex
{
	Vector4 position;
	Vector2 uv;
};

struct VertexWeight
{
	Vector4i boneIds;
	Vector4 boneWeights;
};

pipelineInfo.add_vertex_attribute(
	0u, /* location */
	static_cast<VkFormat>(vk::Format::eR32G32B32A32Sfloat),
	0u, /* start offset */
	sizeof(Vertex), /* stride */
	static_cast<VkVertexInputRate>(vk::VertexInputRate::eVertex)
);
pipelineInfo.add_vertex_attribute(
	1u, /* location */
	static_cast<VkFormat>(vk::Format::eR32G32Sfloat),
	sizeof(Vector4), /* start offset */
	sizeof(Vertex), /* stride */
	static_cast<VkVertexInputRate>(vk::VertexInputRate::eVertex)
);

pipelineInfo.add_vertex_attribute(
	2u, /* location */
	static_cast<VkFormat>(vk::Format::eR32G32B32A32Sint),
	0u, /* start offset */
	sizeof(VertexWeight), /* stride */
	static_cast<VkVertexInputRate>(vk::VertexInputRate::eVertex)
);
pipelineInfo.add_vertex_attribute(
	3u, /* location */
	static_cast<VkFormat>(vk::Format::eR32G32B32A32Sfloat),
	sizeof(Vector4i), /* start offset */
	sizeof(VertexWeight), /* stride */
	static_cast<VkVertexInputRate>(vk::VertexInputRate::eVertex)
);

After baking that would result in 4 vertex attributes and 2 vertex bindings, which is what I want.
However, if I were to extend the Vertex-struct in the future, e.g. like so:

struct Vertex
{
	Vector4 position;
	Vector2 uv;
	Vector2 someNewProperty;
};

then I suddenly end up with 4 vertex attributes and 1 vertex binding (because Vertex and VertexWeight now have the same size/stride).
In fact, as far as I can tell this case would make it impossible to end up with 2 vertex bindings.
Even if I try using explicit binding indices like so:

pipelineInfo.add_vertex_attribute(
	0u, /* location */
	static_cast<VkFormat>(vk::Format::eR32G32B32A32Sfloat),
	0u, /* start offset */
	sizeof(Vertex), /* stride */
	static_cast<VkVertexInputRate>(vk::VertexInputRate::eVertex),
	0u /* explicit binding index */
);
pipelineInfo.add_vertex_attribute(
	1u, /* location */
	static_cast<VkFormat>(vk::Format::eR32G32Sfloat),
	sizeof(Vector4), /* start offset */
	sizeof(Vertex), /* stride */
	static_cast<VkVertexInputRate>(vk::VertexInputRate::eVertex),
	0u /* explicit binding index */
);

pipelineInfo.add_vertex_attribute(
	2u, /* location */
	static_cast<VkFormat>(vk::Format::eR32G32B32A32Sint),
	0u, /* start offset */
	sizeof(VertexWeight), /* stride */
	static_cast<VkVertexInputRate>(vk::VertexInputRate::eVertex),
	1u /* explicit binding index */
);
pipelineInfo.add_vertex_attribute(
	3u, /* location */
	static_cast<VkFormat>(vk::Format::eR32G32B32A32Sfloat),
	sizeof(Vector4), /* start offset */
	sizeof(VertexWeight), /* stride */
	static_cast<VkVertexInputRate>(vk::VertexInputRate::eVertex),
	1u /* explicit binding index */
);

I end up with 4 vertex attributes and 4 vertex bindings (Maybe I'm using explicit binding indices incorrectly?).

I don't think the stride should be used in the first place to determine the number of vertex bindings that will be created. To be honest, I prefer the original way of defining vertex attributes/bindings over how it's done in Anvil, I find the way Anvil tries to "merge" attributes and bindings misleading (maybe that's just me though).

[DominikWitczakAMD]

Sounds like explicit binding index support issue. I'll try having a look at this later this week.

Aside from this, agreed that an automated solution is not going to fit the bill for all possible use cases. Would letting the application select the vertex binding assignment strategy at GFX pipeline creation time help here? Two "algorithms" that would be exposed would be:

• Automated (meaning what is implemented right now + a brief description of the current mechanics ind oxygen)
• Explicit (meaning you add vertex attribute and vertex bindings manually)

Others could be added in the future, too.

[Silverlan]

Sounds like explicit binding index support issue. I'll try having a look at this later this week.

I thought it might be a bug, but I wasn't sure. I think the issue is in GraphicsPipelineData::bake_vk_attributes_and_bindings(), in this code block:

if (current_attribute_explicit_vertex_binding_index == UINT32_MAX)
{
    for (auto vk_input_binding_iterator  = vk_input_bindings.begin();
              vk_input_binding_iterator != vk_input_bindings.end();
            ++vk_input_binding_iterator)
    {
        if (vk_input_binding_iterator->inputRate == current_attribute_rate  &&
            vk_input_binding_iterator->stride    == current_attribute_stride)
        {
            has_found           = true;
            n_attribute_binding = static_cast<uint32_t>(vk_input_binding_iterator - vk_input_bindings.begin());

            break;
        }
    }
}
else
{
    has_found           = false;
    n_attribute_binding = current_attribute_explicit_vertex_binding_index;
} 

I believe the else branch should look something like this instead:

else
{
    n_attribute_binding = current_attribute_explicit_vertex_binding_index;
    has_found           = std::find_if(vk_input_bindings.begin(),vk_input_bindings.end(),[n_attribute_binding](const VkVertexInputBindingDescription &inputBindingDesc) {
        return inputBindingDesc.binding == n_attribute_binding;
    }) != vk_input_bindings.end();
}

to prevent it from creating a new binding if the binding index already exists.

Would letting the application select the vertex binding assignment strategy at GFX pipeline creation time help here?

That would make the most sense I think, keep the automatic method to prevent backwards incompatibility, but also provide an alternative, more explicit method. That way all possible cases should be covered without losing convenience.

[DominikWitczakAMD]

After giving this issue some thought, I agree the existing mechanism is unwieldy and needs to be abandoned. I'm going to replace GraphicsPipelineCreateInfo::add_vertex_attribute() with GraphicsPipelineCreateInfo::add_vertex_binding() which will take all properties of the binding, as well as an array of attributes that will consume the binding.

I don't see much value in retaining existing mechanism and porting Anvil applications to use the new solution should be straightforward.

[DominikWitczakAMD]

Addressed internally. Fix will be included in next week's update.

[DominikWitczakAMD]

Fixed in the latest version.