-
Notifications
You must be signed in to change notification settings - Fork 436
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
Conversation
Oh nice! I'll take a look at instance rate divisors on weekend. |
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. |
After starting from the 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![],
) |
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). |
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 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. |
You're talking about breaking changes to the P.S. There is also a possibility to add vertex/instance count as a field in CommandBuffer struct that you can modify with 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(); |
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 |
But what if vertex buffer is empty, max number of elements in this case is 0 and i want to draw more than maximum |
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. |
What if there is 0 elements in both vertex and instance buffer and shader just generates vertex info itself from gl_VertexIndex/gl_InstanceIndex |
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. |
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. |
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);
}
} |
@Rua Would you mind fixing merge conflicts, so I can proceed with review/merge? |
@Rua Thank you! Merged. |
…e rate divisors (vulkano-rs#1619) * Rework VertexDefinition trait, add BuffersDefinition * Add support for instance rate divisors
Changelog:
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:
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.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.