Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better custom vertex buffer layouts support #2119

Merged
merged 11 commits into from Jan 3, 2023

Conversation

trevex
Copy link
Contributor

@trevex trevex commented Dec 28, 2022

The goal of this PR is to make creating and using custom vertex buffer layouts easier and allow them to leverage a pre-existing VertexDefinition implementation. This will also allow vertex types derived from Vertex to interact with custom runtime created ones.

Here is some pseudo-code to illustrate the idea:

#[repr(C)]
#[derive(Clone, Copy, Debug, Default, Zeroable, Pod, Vertex)]
struct InstanceData {
    #[format(R32G32_SFLOAT)]
    position_offset: [f32; 2],
    #[format(R32_SFLOAT)]
    scale: f32,
}

// There is no runtime vertex buffer builder helper yet, but just image there is :)
const ATTRIBUTE_POSITION: VertexAttribute = VertexAttribute::new("position", Format::R32G32_SFLOAT);
const ATTRIBUTE_COLOR: VertexAttribute = VertexAttribute::new("color", Format::R32G32B32_SFLOAT);

let (buffer, info) = RuntimeVertexBufferBuilder::new()
        .add(
            ATTRIBUTE_POSITION,
            vec![
                Vec2::new(-0.5, -0.25),
                Vec2::new(0.0, 0.5),
                Vec2::new(0.25, -0.1),
            ],
        )
       .build();

// Both can be used together and leverage the matching logic previously implemented by `BuffersDefinition`
let pipeline = GraphicsPipeline::start()
        .vertex_input_state([info.per_vertex(), InstanceData::per_instance()])
  1. Update documentation to reflect any user-facing changes - in this repository.

  2. Make sure that the changes are covered by unit-tests.

  3. Run cargo fmt on the changes.

  4. Please put changelog entries in the description of this Pull Request
    if knowledge of this change could be valuable to users. No need to put the
    entries to the changelog directly, they will be transferred to the changelog
    file by maintainers right after the Pull Request merge.

    Please remove any items from the template below that are not applicable.

  5. Describe in common words what is the purpose of this change, related
    Github Issues, and highlight important implementation aspects.

Changelog:

### Breaking changes
Deprecates `BuffersDefinition`:
- Use `VertexBufferDescription` directly.

### Additions
- Introduction of `VertexBufferDescription` makes it easy to create custom description and leverage the matching logic previously provided by `BuffersDefinition`.

While this change does not directly implement runtime interleaved vertex buffers. There is nothing stopping users from implementing them as demonstrated in Discord by @marc0246 and on the runtime-vertex-* branches of my fork.

Fixes #736

…ure to return VertexInfo and implement VertexInput-trait helper for Vertex to construct VertexBufferInfo objects from Vertex types. Updated teapot example to show usage.
@trevex trevex changed the title Refactor Vertex and VertexDefinition Better custom vertex buffer layouts support Dec 29, 2022
@trevex
Copy link
Contributor Author

trevex commented Dec 29, 2022

Updated the description and reimplemented BuffersDefinition, although it would be deprecated by these changes.

If you look at the code please careful review the names I have chosen for structs. I am not sure the *Info is a great idea considering they have no relation to Vulkan and might confuse users, but I have yet to come up with better ones. Opinions appreciated.

…BufferInfo methods on Vertex and use std HashMap transparently instead.
@trevex
Copy link
Contributor Author

trevex commented Dec 29, 2022

Thanks @Rua, I will go through your feedback.

As the changes of this PR make the implementation of runtime vertex formats possible I wanted to give it a try. The last commit implements a way to create interleaved runtime vertex formats and buffers. I added a simple test and example and seems to work fine.

Non-interleaved vertex buffers would already be possible either way, but this fixes the open issues regarding interleaved runtime buffers.

I expect the code to need careful review regarding allocations and the iterator logic.

Should I include those as part of this PR or move those changes elsewhere?

@Rua
Copy link
Contributor

Rua commented Dec 29, 2022

The runtime stuff is probably better left to a separate PR.

If this is meant to deprecate BuffersDefinition, can you add a deprecation notice to it? And can you add documentation to the methods of Vertex and VertexBufferInfo?

@trevex
Copy link
Contributor Author

trevex commented Dec 31, 2022

Before continuing to update the examples:
I would like to get some feedback on the struct-name VertexBufferInfo. Would VertexFormat or VertexBufferFormat be a better name?

@Rua
Copy link
Contributor

Rua commented Dec 31, 2022

It seems to describe the contents of a vertex buffer, so how about VertexBufferDescription?

@trevex trevex marked this pull request as ready for review January 3, 2023 11:20
@Rua Rua merged commit 834cc1e into vulkano-rs:master Jan 3, 2023
Rua added a commit that referenced this pull request Jan 3, 2023
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
* Implement VertexDefinition for VertexBufferInfo, change Vertex signature to return VertexInfo and implement VertexInput-trait helper for Vertex to construct VertexBufferInfo objects from Vertex types. Updated teapot example to show usage.

* Reimplement BuffersDefinition leveraging VertexBufferInfo

* Reduce the amount of types required, remove VertexInfo, expose VertexBufferInfo methods on Vertex and use std HashMap transparently instead.

* Fix teapot example

* Fix outdated docs and use full path to Vertex trait to avoid issues

* Fix formatting of imports

* Remove clone and directly instantiate VertexInputBindingDescription.

* Rename VertexBufferInfo to VertexBufferDescription and being update of examples to use it

* Run cargo clippy fix to cleanup imports in examples

* Remove obsolete comment

* cargo fmt
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runtime VertexAttributes
3 participants