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.
| 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.
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.
| 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.
What does your suggestion change? The lines look identical to me.
There was a problem hiding this comment.
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.
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.
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.
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.
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.
|
Adopted in #11633 |
Objective
Solution
Changelog