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

animations: convert skinning weights from unorm8x4 to float32x4 #9338

Merged
merged 2 commits into from
Aug 16, 2023

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Aug 2, 2023

Objective

Solution

WEIGHTS_n: float, or normalized unsigned byte, or normalized unsigned short

@mockersf mockersf added the A-Animation Make things move and change over time label Aug 2, 2023
@nicopap nicopap self-requested a review August 2, 2023 15:39
@nicopap
Copy link
Contributor

nicopap commented Aug 3, 2023

This doesn't fix it in the case of my modified snow model.

It's 24MB here (sorry Microsoft for eating your storage space 🙏):

snow_mini.zip

There is a lot of mess in the file, but yeah bevy still shouldn't panic with a cryptic error message.

@alice-i-cecile alice-i-cecile added the C-Bug An unexpected or incorrect behavior label Aug 3, 2023
@mockersf
Copy link
Member Author

mockersf commented Aug 3, 2023

There is a lot of mess in the file, but yeah bevy still shouldn't panic with a cryptic error message.

As the error obviously says for your model, it has too many joints (1484, Bevy supports up to 256 joints)
the error:

thread '<unnamed>' panicked at 'wgpu error: Validation Error

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(0, 33, Metal)>`
    In a draw command, indexed:true indirect:false
      note: render pipeline = `<RenderPipeline-(5, 1, Metal)>`
    The pipeline layout, associated with the current render pipeline, contains a bind group layout at index 2 which is incompatible with the bind group layout associated with the bind group at 2

So... obvious. If I raise the MAX_JOINTS here:

const MAX_JOINTS: usize = 256;

I get something that doesn't crash, but still not what you expect...
Screenshot 2023-08-04 at 00 06 06

So this PR doesn't fix your issue, but it still need to be done to follow the GLTF spec

@eastside
Copy link

I can confirm that this solved an issue importing a .gltf generated from Unreal Engine for me! Looks great!

@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 16, 2023
@mockersf
Copy link
Member Author

merging as trivial as this PR follows glTF spec without introducing new concept to do so

@mockersf mockersf added this pull request to the merge queue Aug 16, 2023
Merged via the queue into bevyengine:main with commit 3aad5c6 Aug 16, 2023
22 checks passed
robtfm pushed a commit to robtfm/bevy that referenced this pull request Sep 1, 2023
…engine#9338)

# Objective

- Fixes part of  bevyengine#9021 

## Solution

- Joint mesh are in format `Unorm8x4` in some gltf file, but Bevy
expects a `Float32x4`. Converts them. Also converts `Unorm16x4`
- According to gltf spec:
https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#skinned-mesh-attributes
> WEIGHTS_n: float, or normalized unsigned byte, or normalized unsigned
short
vprime pushed a commit to vprime/bevy that referenced this pull request Sep 22, 2023
…engine#9338)

# Objective

- Fixes part of  bevyengine#9021

## Solution

- Joint mesh are in format `Unorm8x4` in some gltf file, but Bevy
expects a `Float32x4`. Converts them. Also converts `Unorm16x4`
- According to gltf spec:
https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html#skinned-mesh-attributes
> WEIGHTS_n: float, or normalized unsigned byte, or normalized unsigned
short

(cherry picked from commit 3aad5c6)
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 C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants