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

Skinned Mesh Example #349

Merged
merged 43 commits into from
Feb 28, 2024
Merged

Skinned Mesh Example #349

merged 43 commits into from
Feb 28, 2024

Conversation

cmhhelgeson
Copy link
Contributor

A port of the WebGLFundamentals skinned mesh tutorial to webgpu-samples.

This is what the skinned grid currently looks like.

Skinned Grid

And this is what the whale currently looks like:
Whale

I'm going to try to cut out a lot of the GLTF class cruft to get to the minimal viable version of this without having to deal with all the GLTF abstraction nonsense, but any additional advice would be appreciated.

Merge main into my fork
Merge main into my fork.
…d position that I don't think I initially intended
…ink this needs to be vastly oversimplified to be less generalizable and more focused on the specific mesh and skin we want to target. Which means getting rid of a lot of class shennanigins outside the Mesh -> Primitive -> Accessor pipeline probably
…ther than overriting the transformationMatrix with the value of the rotationMatrix : (
@austinEng
Copy link
Collaborator

I'm going to try to cut out a lot of the GLTF class cruft to get to the minimal viable version of this without having to deal with all the GLTF abstraction nonsense, but any additional advice would be appreciated.

@toji @greggman might have suggestions about this

@cmhhelgeson
Copy link
Contributor Author

cmhhelgeson commented Jan 24, 2024

I'm going to try to cut out a lot of the GLTF class cruft to get to the minimal viable version of this without having to deal with all the GLTF abstraction nonsense, but any additional advice would be appreciated.

@toji @greggman might have suggestions about this

Just wanted to heads-up that I probably won't be able to get to this today. But I'm going to try working with the simpleSkin GLTF example first to get that working before moving to the whale. Basic Mesh rendering works so the issue is just with the skin/node code at the moment.

@toji
Copy link
Contributor

toji commented Jan 24, 2024

Having implemented glTF skinning in WebGPU a couple of times now (in https://github.com/toji/burrow and https://github.com/toji/spookyball) and not being particularly satisfied with either of them, I'm not sure I have a lot of great advice because it's just kind of complicated no matter how you deal with it.

I guess one thing I would highly recommend is to not try to make a general purpose glTF loader, as those can get complicated fast, and dealing with all the edge cases is painful. I'd pick a mesh or two you want to render, push them through something like gltf-transform to make sure they're nicely normalized, and just focus on what you need to load that. Things like assuming that you'll always get a specific set of attributes in a specific format can significantly simplify the loading. Plaster the sample and the code with comments plainly stating that you don't intend it to be a robust glTF loader, so that people don't complain when it doesn't work on their mesh. (Or, more accurately: when they complain you can point them at the comments and recommend a more robust library.)

Also, these days compute skinning is all the rage, and WebGPU is perfectly capable of doing it (See: https://github.com/toji/burrow/blob/main/src/renderer/render-utils/compute-skinning.ts) but it introduces a lot of additional compute-specific complications (see: https://toji.dev/webgpu-best-practices/compute-vertex-data) and so I wouldn't recommend trying to demonstrate it here. At least not as a first step.

@cmhhelgeson
Copy link
Contributor Author

cmhhelgeson commented Jan 25, 2024

Having implemented glTF skinning in WebGPU a couple of times now (in https://github.com/toji/burrow and https://github.com/toji/spookyball) and not being particularly satisfied with either of them, I'm not sure I have a lot of great advice because it's just kind of complicated no matter how you deal with it.

I guess one thing I would highly recommend is to not try to make a general purpose glTF loader, as those can get complicated fast, and dealing with all the edge cases is painful. I'd pick a mesh or two you want to render, push them through something like gltf-transform to make sure they're nicely normalized, and just focus on what you need to load that. Things like assuming that you'll always get a specific set of attributes in a specific format can significantly simplify the loading. Plaster the sample and the code with comments plainly stating that you don't intend it to be a robust glTF loader, so that people don't complain when it doesn't work on their mesh. (Or, more accurately: when they complain you can point them at the comments and recommend a more robust library.)

Also, these days compute skinning is all the rage, and WebGPU is perfectly capable of doing it (See: https://github.com/toji/burrow/blob/main/src/renderer/render-utils/compute-skinning.ts) but it introduces a lot of additional compute-specific complications (see: https://toji.dev/webgpu-best-practices/compute-vertex-data) and so I wouldn't recommend trying to demonstrate it here. At least not as a first step.

Definitely not intending to make a general purpose loader, but I didn't know about the compute skinning, so maybe I'll look into that later.

I am finding a lot of pretty obvious bugs in my code, so maybe I'm closer than I think.

@cmhhelgeson cmhhelgeson marked this pull request as ready for review January 29, 2024 23:04
src/sample/skinnedMesh/main.ts Outdated Show resolved Hide resolved
src/sample/skinnedMesh/main.ts Outdated Show resolved Hide resolved
… uniform buffers rather than calculating them against jointMatrices on the CPU side. Also added additional comments and renamed vertex attributes in grid.wgsl to conform more closely to their counterparts in gltf.wgsl
@cmhhelgeson cmhhelgeson requested a review from toji January 30, 2024 21:52
@toji
Copy link
Contributor

toji commented Jan 30, 2024

I loaded this up locally and, welp...

Skinned.Mesh.-.WebGPU.Samples.-.Trim.mp4

As much as I honestly adore that little dance, I don't think it's quite what's intended here. 🐋🕺

I'll look through the code and see if I can figure anything else out. Thanks for your ongoing work on this!

@cmhhelgeson
Copy link
Contributor Author

cmhhelgeson commented Jan 30, 2024

I loaded this up locally and, welp...

Skinned.Mesh.-.WebGPU.Samples.-.Trim.mp4
As much as I honestly adore that little dance, I don't think it's quite what's intended here. 🐋🕺

I'll look through the code and see if I can figure anything else out. Thanks for your ongoing work on this!

Oh, I just changed the guy to rotate on the z axis instead of the x-axis because I thought it looked cuter.

I can keep it as is, change it back, or add an option for both.

Granted, there might still be something wrong if the skin animation doesn't quite match up with the webgl2 version, so I'll go back.

@cmhhelgeson
Copy link
Contributor Author

cmhhelgeson commented Feb 1, 2024

At the moment, I think the issues have more to do with the animation rather than the skinning of the mesh itself, as each node seems to properly align with the proper joint and such. Accordingly, I've changed the animation a little bit to be more natural, as I think that might have been the real issue (ie when everything joint was rotating the same amount at the same time, the movement of the mesh did not seem "natural").

NOTE: Natural is a relative term, I an not an animator.

@cmhhelgeson cmhhelgeson requested a review from toji February 6, 2024 22:10
@cmhhelgeson
Copy link
Contributor Author

Hello, just wanted to check and see if this needs more work.

@toji
Copy link
Contributor

toji commented Feb 26, 2024

I apologize for disappearing for a bit there. I just got back from a family vacation, so I'm catching up on various things.

I'm not sure if I can use the gltf viewer to bug test the animation, as the animation is not driven by the loaded gltf animations, but by code within main.ts that applies new position, rotation, and scale values to the BaseTransform of the given node.

I didn't realize initially that you were manually moving the joints in code and not parsing animations from the file, and I apologize for that because it likely made for some confusing review comments. While I'm a little torn on this I think ultimately avoiding the complexity of parsing the glTF animations is the right call for an example that wants to keep the focus on the technique of skinning.

With that in mind, I think it's likely fine, but give me a little bit longer to take one more pass through the code. I'll try to have that to you by end of day today or early tomorrow.

Copy link
Contributor

@toji toji left a comment

Choose a reason for hiding this comment

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

Overall I think this is in a pretty good place! Aside from a couple of extremely minor nits the biggest thing I'd like to see is some additional comments to help explain the code to developers, then I think we can land this!

public/assets/gltf/RiggedSimple.glb Outdated Show resolved Hide resolved
public/assets/gltf/simpleSkin.glb Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this needs to be solved before this lands, but at this point this file is so similar to gltf.wgsl that it would be really nice if we could combine the shaders somehow. A future update that allows them to be combined somehow would be nice (either by having some sort of preprocessor pass to include things like the normal math only when needed or by simply providing the grid with dummy normals.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to see this file have more comments. Since these examples are meant to be educational, anything that would help someone understand what is happening and why is beneficial.

In terms of audience, I think it's reasonable to assume that the reader has some knowledge of skinning as a concept (because there's lots of resources available that explain it in depth) and are most likely looking to this example to figure out how to execute that concept in WebGPU specifically.

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 added some more comments, though I'm not sure if there should be more.

src/sample/skinnedMesh/main.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@toji toji left a comment

Choose a reason for hiding this comment

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

Thank you for the updates and for your persistence in getting this landed. Looks good to me!

@ben-clayton
Copy link
Collaborator

I don't believe the PR author has permissions to land, so merging on their behalf.

@ben-clayton ben-clayton merged commit 41f3e5c into webgpu:main Feb 28, 2024
1 check passed
@cmhhelgeson cmhhelgeson deleted the skinned_mesh branch February 28, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants