-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Batch skinned meshes #10094
base: main
Are you sure you want to change the base?
Batch skinned meshes #10094
Conversation
@nicopap @superdump we should figure out how to combine this with #9902 |
impl From<&MeshTransforms> for MeshUniform { | ||
fn from(mesh_transforms: &MeshTransforms) -> Self { | ||
impl MeshUniform { | ||
pub fn from(mesh_transforms: &MeshTransforms, skin_index: u32) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub fn from(mesh_transforms: &MeshTransforms, skin_index: u32) -> Self { | |
pub fn new(mesh_transforms: &MeshTransforms, skin_index: u32) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with this change, as from
has a special meaning in Rust wrt the From
-trait (I am aware that you're aware, but potential new contributors might not)
pub(super) fn skinning(binding: u32) -> BindGroupLayoutEntry { | ||
buffer(binding, JOINT_BUFFER_SIZE as u64, ShaderStages::VERTEX) | ||
pub(super) fn skinning(render_device: &RenderDevice, binding: u32) -> BindGroupLayoutEntry { | ||
let is_storage = render_device.limits().max_storage_buffers_per_shader_stage > 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let is_storage = render_device.limits().max_storage_buffers_per_shader_stage > 0; | |
let is_storage = render_device.limits().max_storage_buffers_per_shader_stage > 0; |
At this point, I think we should add a method like "storage_buffers_supported()" to RenderDevice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does your suggestion change? The lines look identical to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't change anything sorry. This was meant to be a regular comment on that line.
pub enum SkinUniform { | ||
Uniform(BufferVec<Mat4>), | ||
Storage(StorageBuffer<Vec<Mat4>>), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered defining this struct as a standalone type in bevy_render
? Similarly to GpuArrayBuffer
? Even if it is only used here, it helps separating concerns. It will also make it easier to remediate conflicts with #9002 (independently of who has to fix the merge conflicts)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that it's very specific to Mat4 at the moment. Ideally it feels like this should be a different 'mode' of GpuArrayBuffer. One mode for batching as many instances of data into one binding, and one where you have a known max binding size that you use for a uniform buffer, but you may not use it all so you align to the next dynamic offset binding as needed and make sure the last dynamic offset has a full binding of data after it. That requires more thought to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if it weren't specific to Mat4, we could use an affine matrix instead and save space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured out how to do this, and would like to. But I think it has to wait until after 0.12. I'll move this to 0.13.
Objective
Solution
Changelog