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

Rework VertexDefinition trait, add BuffersDefinition, support instance rate divisors #1619

Merged
merged 3 commits into from
Jul 5, 2021

Conversation

Rua
Copy link
Contributor

@Rua Rua commented Jun 22, 2021

Changelog:

- **Breaking** The `VertexDefinition` trait no longer has `AttribsIter` and `BuffersIter` as associated types. It instead returns a `Vec<VertexInputBinding>`.
- **Breaking** The various types for vertex definitions (except `BufferlessDefinition`) have been replaced with the new `BuffersDefinition` type, which can handle any number of vertex and instance buffers. Examples of how it's used can be seen in the "instancing" and "teapot" examples.
- **Breaking** `InputRate` has been renamed to `VertexInputRate` to match the Vulkan name. The `Instance` variant now has a `divisor` member.
- Added support for the `ext_vertex_attribute_divisor` extension, via the new `BuffersDefinition` type and the additions to `VertexInputRate`.

This PR makes it easier and more flexible for the user to define various configurations of vertex and instance buffers. I removed the old types, but I can add them back in and deprecate them if that's preferred.

It also fixes #1582 by implementing instance rate divisors. @Nhlest do you want to test this PR to ensure it works for your purposes?

There is still a problem with the way vertex and instance counts are currently specified in draw calls. Vulkano doesn't allow the user to specify the counts, instead they are figured out automatically from the sizes of the vertex and instance buffers that are provided. This works fine in most cases, but it breaks down quite badly in two cases:

  1. With bufferless drawing. In this case, there's no vertex and/or instance buffer, so the code can't figure out how many to draw. I brought this up long ago in No way to specify number of vertices with SingleInstanceBufferDefinition #1408. Vulkano currently defaults to drawing 1 vertex or 1 instance in this case, which is a bit silly. The BufferlessDefinition type alleviates some of it, but it only works when you draw with neither a vertex nor an instance buffer. It also specifies the counts as part of the pipeline, even though it's a per-draw setting.
  2. With the instance rate divisor that I just added support for. Since the instance count is based on buffer size, with a divisor of N you end up drawing N times as many instances for the same buffer (unintuitive!), and you can only draw a number of instances that's divisible by N. This becomes completely broken when the divisor is 0, which according to the Vulkan spec means that every instance should use the same instance data from the buffer. In Vulkano, this ends up meaning that your draw call draws usize::MAX instances, which is clearly not what anyone wants.

These problems could be solved most straightforwardly by adding parameters to the draw calls on AutoCommandBufferBuilder, to specify the number of vertices and instances. But these are very widely used functions, so changing their API would be a very large breaking change affecting almost all Vulkano users.

@Rua Rua changed the title Rework VertexDefinition trait, add BuffersDefinition Rework VertexDefinition trait, add BuffersDefinition, support instance rate divisors Jun 22, 2021
@Nhlest
Copy link

Nhlest commented Jun 24, 2021

Oh nice! I'll take a look at instance rate divisors on weekend.

@Nhlest
Copy link

Nhlest commented Jun 27, 2021

I couldn't quite adapt my old code to this new branch (too many things got changed and i got lost in new types and type arguments), i'll try to start fresh somewhen this week by following new changed examples.

@Eliah-Lakhin Eliah-Lakhin added the PR: Review next Sunday This PR is planned for FIRST review on the nearest Sunday. label Jun 28, 2021
@Eliah-Lakhin
Copy link
Contributor

@Rua Thank you for your work. To receive a feedback from @Nhlest I will postpone the final review/merge up to the end of this week.

@Nhlest
Copy link

Nhlest commented Jun 28, 2021

After starting from the instance example and adding my code on top i managed to make it work. So far this new Instance Divisor feature works looks and feels great to use, but one thing that i noticed is that i can't pass an empty vertex buffer (with all the information required being contained in my InstanceData buffer for example) because draw command calculates number of vertici to draw by looking at that (vertex) buffer size. And it being 0 it draws N instances with 0 vertices each.

I can work around that by providing a vertex buffer with a dummy datatype of needed size but it would be so much better if i could pass number of vertici / instances directly to the CommandBufferBuilder's draw command somehow.

Back of a napkin example (drawing is done in triangle_fan mode):

#[derive(Default, Debug, Clone)]
struct InstanceData {
  xy: [f32; 2],
  tile: [f32; 2],
}
impl_vertex!(InstanceData, xy, tile);

#[derive(Default, Debug, Clone)]
struct VertexDummy {
  dummy: u32
}
impl_vertex!(VertexDummy);

mod vs {
  vulkano_shaders::shader! {
            ty: "vertex",
            src: r#"
#version 450
layout(location = 0) in vec2 xy;
layout(location = 1) in vec2 tile;
layout(location = 0) out vec2 uv0;

const int tileset_side = 2;
const int tilemap_side = 10;

void main() {
  vec2 vertex = vec2(gl_VertexIndex % 2, gl_VertexIndex / 2);
  uv0 = (vertex+tile)/tileset_side;
  gl_Position = vec4((vertex+xy)/tilemap_side*2-vec2(1.0, 1.0), 0.0, 1.0);
}"#}
}

mod fs {
  vulkano_shaders::shader! {
            ty: "fragment",
            src: r#"
#version 450

layout(location = 0) in vec2 uv0;
layout(location = 0) out vec4 f_color;
layout(set = 0, binding = 0) uniform sampler2D tex;
void main() {
  f_color = vec4(texture(tex, uv0).rgba);
}"#}
}

<...>
  let triangle_vertex_buffer = {
    CpuAccessibleBuffer::from_iter(
      device.clone(),
      BufferUsage::all(),
      false,
      [
        VertexDummy::default(),
        VertexDummy::default(),
        VertexDummy::default(),
        VertexDummy::default(),
      ]
        .iter()
        .cloned(),
    )
      .unwrap()
  };

  let instance_data_buffer = {
    let mut data = vec![
      InstanceData{ xy: [3.0, 3.0], tile: [1.0, 0.0] },
      InstanceData{ xy: [1.0, 0.0], tile: [1.0, 0.0] },
      InstanceData{ xy: [2.0, 2.0], tile: [0.0, 1.0] },
      InstanceData{ xy: [3.0, 5.0], tile: [0.0, 0.0] },
    ];
    CpuAccessibleBuffer::from_iter(
      device.clone(),
      BufferUsage::all(),
      false,
      data.iter().cloned(),
    ).unwrap()
  };
<...>
          .draw(
            pipeline.clone(),
            &dynamic_state,
            (triangle_vertex_buffer.clone(), instance_data_buffer.clone()),
            set.clone(),
            (),
            vec![],
          )

@Nhlest
Copy link

Nhlest commented Jun 28, 2021

Nevermind i should have read rest of the initial comment on this PR, it raises same exact issue i explained.

In other words - Instance Divisors implementation is more than satisfactory for the needs i had in #1582 👍 (But the issue with empty vertex buffer is still up in the air for some niche use cases).

@Rua
Copy link
Contributor Author

Rua commented Jun 28, 2021

I want to fix the issue by adding some way to specify the number of vertices and instances with a draw call. The most straightforward way IMO would be to add two parameters to the draw calls, specifying a Range of vertices and instances. But there's other possibilities as well, such as specifying a first element and a count, like Vulkan does. Suggestions would be appreciated.

I could add this change onto this existing PR, but I'd rather not make this PR too big, so I'll wait for this one to merge and then work on the followup.

@Nhlest
Copy link

Nhlest commented Jun 28, 2021

You're talking about breaking changes to the draw command itself or adding a new one draw_with_vert_count? I don't see why there would be any reason to not add a separate function for that usecase that overrides automatic vertex counting with an explicit number given by the calee. (Also you can logically make this function unsafe since it goes against runtime safety provided by automatic deduction of vertex number by calculating size of the buffer data).

P.S. There is also a possibility to add vertex/instance count as a field in CommandBuffer struct that you can modify with fn with_vertex_count(&self, u32) -> Self and modify draw call as

            self.inner.draw(
                self.vcount.unwrap_or(vb_infos.vertex_count as u32),
                self.icount.unwrap_or(vb_infos.instance_count as u32),
                0,
                0,
            );
        builder
          .begin_render_pass(
            framebuffers[image_num].clone(),
            SubpassContents::Inline,
            clear_values,
          )
          .unwrap()
          .with_explicit_vertex_count(3)
          .draw(
            pipeline.clone(),
            &dynamic_state,
            (triangle_vertex_buffer.clone(), instance_data_buffer.clone()),
            set.clone(),
            (),
            vec![],
          )
          .unwrap()
          .end_render_pass()
          .unwrap();

@Rua
Copy link
Contributor Author

Rua commented Jun 28, 2021

A separate function can work, but I'm not the biggest fan of that idea. If it's added though, it doesn't need to be unsafe. The function is still able to check that the buffers contain enough data for all the draws. The buffer size just puts an upper limit on the amount of elements, but you can draw less than that. The problem with the current code is essentially that the user can only draw the maximum amount of elements.

@Nhlest
Copy link

Nhlest commented Jun 28, 2021

But what if vertex buffer is empty, max number of elements in this case is 0 and i want to draw more than maximum

@Rua
Copy link
Contributor Author

Rua commented Jun 28, 2021

In theory, the maximum should be determined by the shortest buffer. If there is no buffer at all, then there's also no maximum. That's not how Vulkano currently works, but it's definitely how it's supposed to work.

@Nhlest
Copy link

Nhlest commented Jun 28, 2021

What if there is 0 elements in both vertex and instance buffer and shader just generates vertex info itself from gl_VertexIndex/gl_InstanceIndex

@Rua
Copy link
Contributor Author

Rua commented Jun 28, 2021

There is a difference between having a vertex/instance buffer of zero length, and no buffer of a given type at all. With zero length, your maximum is zero, so you can't actually draw anything. With no buffer at all, your maximum is unbounded. It comes down to how the input assembly retrieves data from the buffer; you have to make sure there's no out-of-bounds accesses. If there's no buffer access to begin with, there's not going to be out-of-bounds accesses. But if there is a buffer that's going to be accessed, then you have to make sure the vertex/instance indices don't go past the end of the buffer.

@Nhlest
Copy link

Nhlest commented Jun 28, 2021

Oh i see now, thanks for explaining. I am not that knowledgeable in OpenGL/Vulkan to know what's going on on GPU in these corner cases so i was just assuming a lot of things here.

@Rua
Copy link
Contributor Author

Rua commented Jun 28, 2021

Very crudely, in a Rust-like pseudocode, you can imagine a draw call as something like this:

for instance_id in first_instance..(first_instance+instance_count) {
    let per_instance_data = instance_buffers.iter().map(|buf| buf[instance_id / instance_divisor]);
    for vertex_id in first_vertex..(first_vertex+vertex_count) {
        let per_vertex_data = vertex_buffers.iter().map(|buf| buf[vertex_id]);
        run_pipeline(vertex_id, instance_id, per_vertex_data, per_instance_data);
    }
}

@Eliah-Lakhin
Copy link
Contributor

@Rua Would you mind fixing merge conflicts, so I can proceed with review/merge?

@Eliah-Lakhin Eliah-Lakhin removed the PR: Review next Sunday This PR is planned for FIRST review on the nearest Sunday. label Jul 5, 2021
@Eliah-Lakhin Eliah-Lakhin merged commit 0eebf0c into vulkano-rs:master Jul 5, 2021
@Eliah-Lakhin
Copy link
Contributor

@Rua Thank you! Merged.

Eliah-Lakhin added a commit that referenced this pull request Jul 5, 2021
@Rua Rua deleted the vertex-definition branch August 10, 2021 08:38
blacksailer pushed a commit to blacksailer/vulkano that referenced this pull request Nov 25, 2021
…e rate divisors (vulkano-rs#1619)

* Rework VertexDefinition trait, add BuffersDefinition

* Add support for instance rate divisors
blacksailer pushed a commit to blacksailer/vulkano that referenced this pull request Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants