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

Double MAX_JOINTS and MAX_MORPH_WEIGHTS #15972

Closed
wants to merge 1 commit into from

Conversation

victorb
Copy link

@victorb victorb commented Oct 17, 2024

Objective

I tried to import a 3D model that were slightly above the existing MAX_JOINTS and MAX_MORPH_WEIGHTS limits, got this error:

Failed to load asset 'models/humanoid.glb' with asset loader 'bevy_gltf::loader::GltfLoader':
failed to generate morph targets: Bevy only supports up to 64 morph targets (individual poses),
tried to create a model with 77 morph targets

Solution

So tried the simplest solution possible, doubling the limits I hit when trying to load the model. It seems to work without any issues, everything still works exactly the same as far as I can tell. @alice-i-cecile asked me to PR the changes, so here we are :)

Ideally, I'd would have liked the comments for these two values to describe why those exact limits have been chosen, but I don't know the answer to that, maybe someone else could help me explain why the limits are there in the first place, and how the specific numbers were chosen.

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added A-Animation Make things move and change over time X-Contentious There are nontrivial implications that should be thought through A-glTF Related to the glTF 3D scene/model format labels Oct 17, 2024
@alice-i-cecile
Copy link
Member

Do you have a convenient link to a model that previously failed but now works that we can use to test this?

@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Oct 17, 2024
@alice-i-cecile
Copy link
Member

This has been moved several times, and I'm trying to piece together the history. It was moved to bevy_mesh, and before that split off of mesh.rs in db1e3d3

@alice-i-cecile
Copy link
Member

Ah, @mockersf touched this back in #9351, and may have more context about why this exists and why it was set to that value.

From @IDEDARY on Discord:

[1:31 PM]! 1D3D4RY: Hmm, no errors so far?
[1:31 PM]! 1D3D4RY: I once tried to increase that number
[1:31 PM]! 1D3D4RY: but I got some weird overflow errors in WGPU
[1:31 PM]! 1D3D4RY: If it could be increased I could load my VRChat models to Bevy 😄
[1:32 PM]! 1D3D4RY: (there are face expressions mapped for like 100 blend targets)

@victorb
Copy link
Author

victorb commented Oct 17, 2024

@alice-i-cecile

Do you have a convenient link to a model that previously failed but now works that we can use to test this?

Unfortunately not, it's a purchased & proprietary model and license doesn't allow me to redistribute it like that.

I'm wondering if maybe just the weight/joint data however could be extracted and used for a test case, would just be data and should be fine in theory. If this could work for you/Bevy and be beneficial, I can reach out to the original model author and ask if it's OK with them.

Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

This values are used to allocate buffers. Their maximum sizes depend on the GPU/platform, the current values are the ones guaranteed to work everywhere.

To allow bigger values, you would have to check against the limits of the GPU at runtime, which would mean not using consts anymore.

@alice-i-cecile alice-i-cecile added S-Needs-Design This issue requires design work to think about how it would best be accomplished and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 17, 2024
@victorb
Copy link
Author

victorb commented Oct 18, 2024

To allow bigger values, you would have to check against the limits of the GPU at runtime, which would mean not using consts anymore.

How would that realistically work like? I'm guessing we can't dynamically figure that out, as that would have to take into consideration other things that go into the GPU, or not? Or would it just be a question of reading a value then setting the limits based on that?

Alternatively, would it make sense to have the default values as they were before this patch, and then allow users to set the limit themselves via passed in values? Would be a workaround at best I suppose.

@victorb
Copy link
Author

victorb commented Jan 3, 2025

Closing this in favor of Be able to customize MAX_JOINTS and MAX_MORPH_WEIGHTS #17128

@victorb victorb closed this Jan 3, 2025
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-glTF Related to the glTF 3D scene/model format S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants