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

Batch skinned meshes #10094

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

superdump
Copy link
Contributor

Objective

  • Make it possible to batch/instance skinned meshes

Solution

  • Skinned meshes were excluded from batching/instancing because each skinned mesh entity has its own joint matrix 'skin uniform' binding of a uniform buffer at a dynamic offset. As such, each entity has to be drawn separately in order to set the dynamic offset for each draw.
  • This PR adds support for using a storage buffer for the joint matrices where available.

Changelog

  • Enable batching/instancing of skinned meshes

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times A-Animation Make things move and change over time labels Oct 12, 2023
@JMS55 JMS55 added this to the 0.12 milestone Oct 12, 2023
@JMS55
Copy link
Contributor

JMS55 commented Oct 12, 2023

@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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn from(mesh_transforms: &MeshTransforms, skin_index: u32) -> Self {
pub fn new(mesh_transforms: &MeshTransforms, skin_index: u32) -> Self {

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Comment on lines +30 to +32
pub enum SkinUniform {
Uniform(BufferVec<Mat4>),
Storage(StorageBuffer<Vec<Mat4>>),
Copy link
Contributor

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)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@superdump superdump modified the milestones: 0.12, 0.13 Oct 13, 2023
@james7132 james7132 self-requested a review October 14, 2023 14:12
@alice-i-cecile alice-i-cecile removed this from the 0.13 milestone Jan 24, 2024
@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Jan 24, 2024
@tygyh tygyh mentioned this pull request Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Animation Make things move and change over time A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times S-Adopt-Me The original PR author has no intent to complete this work. Pick me up!
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

5 participants