Implement mesh skinning #2359

Looooong · 2021-06-19T03:24:48Z

Objective

Implement mesh skinning.
Resolve Skeletal Animation #95.
The feature can work independently and it takes one responsibility off the animation crate in Full featured property based Animation System #1429.

Solution

Modify bevy_gltf to extract skins information as well as the joint data in the mesh.
Implement SkinnedMesh that stores:
- A handle to SkinnedMeshInverseBindposes.
- Entities list that addresses the joint entities.
- Joint transform matrices (glTF reference).
Implement SkinnedMeshInverseBindposes to store inverse bindposes data which can be shared between multiple SkinnedMesh component.
Implement a system to update SkinnedMesh's joint transform matrices from SkinnedMeshInverseBindposes and joint entities' GlobalTransform.
These joint transform matrices can be binded directly to shader storage buffer by implementing RenderResource and RenderResources trait for SkinnedMesh, then by adding RenderResourceNode<SkinnedMesh> to render graph.
Create a modified version of PBR vertex shader that combines joint weights with joint matrices (glTF reference).

Compare to the counterpart in #1429

The code is smaller and simpler.
This feature can work independently from the animation crate.
By using RenderResourcesNode instead of AssetRenderResourcesNode, it doesn't need to create custom material asset for each skinned mesh instance that shares the same mesh and skin.

alice-i-cecile · 2021-06-19T03:58:59Z

That's much simpler than I expected! Could we get an example or two of how this works in practice?

I'm interested in how this handles both animations from the gltf as well as creating new animations from code.

alice-i-cecile · 2021-06-19T03:59:41Z

crates/bevy_animation_rig/Cargo.toml

@@ -0,0 +1,24 @@
+[package]
+name = "bevy_animation_rig"


IMO this should be the start of a general bevy_animation crate instead.

The original bevy_animation is meant to animate the component properties. So I created this crate to separate the "skinning" from the "animation".

crates/bevy_animation_rig/src/skinned_mesh.rs

alice-i-cecile · 2021-06-19T04:02:40Z

crates/bevy_gltf/src/loader.rs

@@ -263,6 +311,24 @@ async fn load_gltf<'a, 'b>(
        if let Some(Err(err)) = err {
            return Err(err);
        }
+
+        for (&entity, &skin_index) in &entity_to_skin_index_map {


So it looks like this grabs animation data from the .gltf file, creating one entity for each joint?

Technically, it's only skin data, without any animation clip. Entity joints are already created since they are part of the glTF nodes. This code just map the created entity to the joint defined in glTF.

alice-i-cecile · 2021-06-19T04:07:38Z

crates/bevy_gltf/src/loader.rs

@@ -142,6 +146,23 @@ async fn load_gltf<'a, 'b>(
                mesh.set_attribute(Mesh::ATTRIBUTE_UV_0, vertex_attribute);
            }

+            if let Some(vertex_attribute) = reader.read_joints(0).map(|v| {


The new code in this function doesn't seem to be feature gated.

This code is in bevy_gltf crate, and it depends on the bevy_animation_rig. Making bevy_animation_rig optional in this crate is kinda hard as bevy_gltf is also optional for bevy. I don't know how to deal with this. Do you have any suggestion?

crates/bevy_internal/src/default_plugins.rs

alice-i-cecile

Very simple! I have a few nits, but mostly I want to see end-user examples of how this is used before I can comment further.

Looooong · 2021-06-19T04:11:09Z

@alice-i-cecile Sure, I will sort these things out.

NathanSWard

Here a two very small thing.
I'll give this PR a more formal review tomorrow/this weekend :)

crates/bevy_animation_rig/src/skinned_mesh.rs

crates/bevy_animation_rig/src/lib.rs

crates/bevy_animation_rig/src/skinned_mesh.rs

NathanSWard · 2021-06-19T06:39:34Z

crates/bevy_animation_rig/src/skinned_mesh.rs

+    }
+
+    fn write_buffer_bytes(&self, buffer: &mut [u8]) {
+        let transform_size = std::mem::size_of::<[f32; 16]>();


If we're checking for the transform size, why are use using [f32; 16] instead of Mat4?
(I might not be understanding something so please correct me if I'm wrong haha :)

I got this std::mem::size_of::<[f32; 16]>() from RenderResource implementation for Mat4. I'm not sure why as well. Maybe Mat4 may have internal states so we don't rely on the size of Mat4?

crates/bevy_animation_rig/src/skinned_mesh.rs

Looooong · 2021-06-19T11:13:43Z

@alice-i-cecile I have added animation/custom_skinned_mesh example and animation/gltf_skinned_mesh example. You can check them out.

examples/animation/gltf_skinned_mesh.rs

crates/bevy_animation_rig/src/skinned_mesh.rs

NathanSWard · 2021-06-20T02:32:11Z

@Looooong
I'm curious if #2215 is related to the reflection bug we saw.

NathanSWard · 2021-06-20T02:26:36Z

crates/bevy_animation_rig/src/skinned_mesh.rs

+
+/// Manually implement [`bevy_reflect::Reflect`] for [`SkinnedMeshJoint`] to work around an issue,
+/// where spawning a scene with a component containings a vector of structs would result in runtime panic.
+unsafe impl Reflect for SkinnedMeshJoint {


I'm curious, is deriving Reflect on the SkinnedMeshJoint enough?
(Instead of manually implementing it?)

It's not just normal manual implementation but more like a monkey-patch. I patched the clone_value() method to work around this issue.

Looooong · 2021-06-20T04:02:33Z

@Looooong
I'm curious if #2215 is related to the reflection bug we saw.

Yes, it is.

tasgon · 2021-10-30T08:29:49Z

Hi, just was wondering, are there any issues still blocking this?

alice-i-cecile · 2022-01-06T21:33:59Z

@Looooong could you comment in #2373? There's interest in reviving this and it would be a shame to have to start from scratch.

Looooong · 2022-01-07T21:34:05Z

@alice-i-cecile I have left the comment. I haven't touch this for a while. Is there anything else I need to do to get this shipped?

alice-i-cecile · 2022-01-07T22:00:42Z

Is there anything else I need to do to get this shipped?

It's looking quite good! Code quality is solid, and I'd be happy to ship this as an initial building block. This PR is currently just waiting on editorial attention: animation wasn't a focus for the 0.6 release. I would expect it to receive attention during the 0.7 cycle; animation seems to be one of the bigger stumbling blocks now that rendering is cleaned up.

cart · 2022-01-07T22:18:20Z

Some quick notes (I'll do a review soon after 0.6 releases):

This would need to be updated to the new renderer
I think skeletal animation is "core" enough that it should be a part of our built-in mesh shader (which lives in the new bevy_pbr at the moment), to ensure that user-defined custom shaders can be animated easily.

nicopap · 2022-01-18T13:19:57Z

As someone who has been heavily reliant on this PR for my own prototypes (really thank you for your work!) I have a some comments regarding the API:

There should be an example for picking gltf node entities for the joints. I've spent way too much time on figuring this out. Here is a gist of what I came up with.
Spawning the joint nodes as entities in the world with a Transform and GlobalTransform is perfect! Please keep it this way.
The NickJBenson PR on this fork adds an example file that implements a clean API to manage multiple animations and transitions between them. It might be worth picking it up.

Looooong · 2022-01-21T18:50:24Z

@cart

I think skeletal animation is "core" enough that it should be a part of our built-in mesh shader (which lives in the new bevy_pbr at the moment), to ensure that user-defined custom shaders can be animated easily.

Now that you mention about it, this is a really good idea that I have never considered before. I will move this feature into bevy_pbr crate.

However, there are a few things I need some advices on:

Before this PR, I only had experience writing shader in ShaderLab in Unity. I'm fairly new at writing OpenGL code. I need directions and suggestions on best practices in writing OpenGL code, so that other people can write custom shader with ease.
Attempted to push invalid value of type DynamicStruct, expected Handle<T> #2215 is the reason I need to implement Reflect manually to workaround the problem. Is this bug fixed? If so, then I can remove this monkey-patch code.

robclu · 2022-02-02T10:56:08Z

Hi, just wanted to check what still needs to be done for this? I need skinning for something that I'm working out so would like to help if possible.

james7132 · 2022-03-21T00:00:40Z

Closing this in favor for #4238, which is compatible with the new renderer. @Looooong is credited in that PR as it was started by merging this PR into main.

For those who left a review here, please give a once-over for that PR.

@nicopap: re: the @nickjbenson PR to this one could be a good start of an actual animation crate. I'd suggest that they open a PR with it after #4238 gets merged.

# Objective Load skeletal weights and indices from GLTF files. Animate meshes. ## Solution - Load skeletal weights and indices from GLTF files. - Added `SkinnedMesh` component and ` SkinnedMeshInverseBindPose` asset - Added `extract_skinned_meshes` to extract joint matrices. - Added queue phase systems for enqueuing the buffer writes. Some notes: - This ports part of # #2359 to the current main. - This generates new `BufferVec`s and bind groups every frame. The expectation here is that the number of `Query::get` calls during extract is probably going to be the stronger bottleneck, with up to 256 calls per skinned mesh. Until that is optimized, caching buffers and bind groups is probably a non-concern. - Unfortunately, due to the uniform size requirements, this means a 16KB buffer is allocated for every skinned mesh every frame. There's probably a few ways to get around this, but most of them require either compute shaders or storage buffers, which are both incompatible with WebGL2. Co-authored-by: james7132 <contact@jamessliu.com> Co-authored-by: François <mockersf@gmail.com> Co-authored-by: James Liu <contact@jamessliu.com>

# Objective Load skeletal weights and indices from GLTF files. Animate meshes. ## Solution - Load skeletal weights and indices from GLTF files. - Added `SkinnedMesh` component and ` SkinnedMeshInverseBindPose` asset - Added `extract_skinned_meshes` to extract joint matrices. - Added queue phase systems for enqueuing the buffer writes. Some notes: - This ports part of # bevyengine#2359 to the current main. - This generates new `BufferVec`s and bind groups every frame. The expectation here is that the number of `Query::get` calls during extract is probably going to be the stronger bottleneck, with up to 256 calls per skinned mesh. Until that is optimized, caching buffers and bind groups is probably a non-concern. - Unfortunately, due to the uniform size requirements, this means a 16KB buffer is allocated for every skinned mesh every frame. There's probably a few ways to get around this, but most of them require either compute shaders or storage buffers, which are both incompatible with WebGL2. Co-authored-by: james7132 <contact@jamessliu.com> Co-authored-by: François <mockersf@gmail.com> Co-authored-by: James Liu <contact@jamessliu.com>

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jun 19, 2021

alice-i-cecile added A-Animation Make things move and change over time C-Enhancement A new feature and removed S-Needs-Triage This issue needs to be labelled labels Jun 19, 2021

alice-i-cecile self-requested a review June 19, 2021 03:57

alice-i-cecile reviewed Jun 19, 2021

View reviewed changes

crates/bevy_animation_rig/src/skinned_mesh.rs Show resolved Hide resolved

alice-i-cecile reviewed Jun 19, 2021

View reviewed changes

crates/bevy_animation_rig/src/skinned_mesh.rs Show resolved Hide resolved

alice-i-cecile reviewed Jun 19, 2021

View reviewed changes

crates/bevy_animation_rig/src/skinned_mesh.rs Show resolved Hide resolved

alice-i-cecile reviewed Jun 19, 2021

View reviewed changes

crates/bevy_internal/src/default_plugins.rs Show resolved Hide resolved

alice-i-cecile reviewed Jun 19, 2021

View reviewed changes

NathanSWard suggested changes Jun 19, 2021

View reviewed changes

crates/bevy_animation_rig/src/skinned_mesh.rs Outdated Show resolved Hide resolved

crates/bevy_animation_rig/src/skinned_mesh.rs Outdated Show resolved Hide resolved

Looooong marked this pull request as draft June 19, 2021 05:57

Looooong force-pushed the skeleton-animation branch 2 times, most recently from b018cad to 280c755 Compare June 19, 2021 06:36

NathanSWard suggested changes Jun 19, 2021

View reviewed changes

Looooong force-pushed the skeleton-animation branch 5 times, most recently from 8d9371b to ca83297 Compare June 19, 2021 11:11

Looooong force-pushed the skeleton-animation branch 3 times, most recently from 38ab695 to 28c6b74 Compare June 19, 2021 12:16

Looooong changed the title ~~Implement skeleton animation~~ Implement mesh skinning Jun 19, 2021

alice-i-cecile reviewed Jun 19, 2021

View reviewed changes

examples/animation/gltf_skinned_mesh.rs Outdated Show resolved Hide resolved

Looooong force-pushed the skeleton-animation branch from 3cd5ad1 to c9edce7 Compare June 20, 2021 01:36

NathanSWard suggested changes Jun 20, 2021

View reviewed changes

crates/bevy_animation_rig/src/skinned_mesh.rs Outdated Show resolved Hide resolved

NathanSWard reviewed Jun 20, 2021

View reviewed changes

Looooong force-pushed the skeleton-animation branch from c9edce7 to 242e7d9 Compare June 22, 2021 02:06

Implement mesh skinning

366742c

Looooong force-pushed the skeleton-animation branch from 242e7d9 to 366742c Compare June 22, 2021 08:28

NathanSWard mentioned this pull request Jun 23, 2021

[Merged by Bors] - Add FromReflect trait to convert dynamic types to concrete types #1395

Closed

mockersf added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 16, 2021

vabrador mentioned this pull request Sep 25, 2021

Implement gltf animation loading, playback example Looooong/bevy#1

Open

mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jan 7, 2022

alice-i-cecile approved these changes Jan 7, 2022

View reviewed changes

mockersf mentioned this pull request Jan 24, 2022

[Merged by Bors] - Gltf animations #3751

Closed

james7132 mentioned this pull request Feb 23, 2022

Tracking Issue: MVP Animation #4026

Open

14 tasks

james7132 mentioned this pull request Mar 17, 2022

[Merged by Bors] - Mesh Skinning. Attempt #3 #4238

Closed

james7132 closed this Mar 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement mesh skinning #2359

Implement mesh skinning #2359

Looooong commented Jun 19, 2021

alice-i-cecile commented Jun 19, 2021

alice-i-cecile Jun 19, 2021

Looooong Jun 19, 2021

alice-i-cecile Jun 19, 2021

Looooong Jun 19, 2021 •

edited

Loading

alice-i-cecile Jun 19, 2021

Looooong Jun 19, 2021

alice-i-cecile left a comment

Looooong commented Jun 19, 2021

NathanSWard left a comment

NathanSWard Jun 19, 2021

Looooong Jun 19, 2021

Looooong commented Jun 19, 2021

NathanSWard commented Jun 20, 2021

NathanSWard Jun 20, 2021

Looooong Jun 20, 2021 •

edited

Loading

Looooong commented Jun 20, 2021

tasgon commented Oct 30, 2021

alice-i-cecile commented Jan 6, 2022

Looooong commented Jan 7, 2022

alice-i-cecile commented Jan 7, 2022

cart commented Jan 7, 2022

nicopap commented Jan 18, 2022

Looooong commented Jan 21, 2022

robclu commented Feb 2, 2022

james7132 commented Mar 21, 2022

Implement mesh skinning #2359

Implement mesh skinning #2359

Conversation

Looooong commented Jun 19, 2021

Objective

Solution

Compare to the counterpart in #1429

alice-i-cecile commented Jun 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Looooong Jun 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

Looooong commented Jun 19, 2021

NathanSWard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Looooong commented Jun 19, 2021

NathanSWard commented Jun 20, 2021

Choose a reason for hiding this comment

Looooong Jun 20, 2021 • edited Loading

Choose a reason for hiding this comment

Looooong commented Jun 20, 2021

tasgon commented Oct 30, 2021

alice-i-cecile commented Jan 6, 2022

Looooong commented Jan 7, 2022

alice-i-cecile commented Jan 7, 2022

cart commented Jan 7, 2022

nicopap commented Jan 18, 2022

Looooong commented Jan 21, 2022

robclu commented Feb 2, 2022

james7132 commented Mar 21, 2022

Looooong Jun 19, 2021 •

edited

Loading

Looooong Jun 20, 2021 •

edited

Loading