Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add morph targets #8158
Add morph targets #8158
Changes from 1 commit
85634b3
e14d164
0317470
603956d
e817b44
7baf474
fe79752
d9c62ca
faa9e9a
0934898
d338d61
b17c345
0386f56
5f44424
11567bc
b7c9051
d1d9c2b
5063b86
2de9951
d40a694
c48c36c
62e4b68
80b85a8
4e115a5
14718bc
68542f8
719413a
ea1ebb1
c21ed92
8a91994
2720a52
3b9755d
09437d6
e0918a4
4d08449
615de1a
edb4b56
1e8eba1
573d0ca
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is duplicated in
prepass.wgsl
because I didn't want to move thestruct Vertex
definition tomesh_types.wgsl
(which probably should be the place whereVertex
is defined)Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The various abstractions here (VisitMorphTargets, store_image, MorphAttributesImage) introduce too much complexity relative to their value.
store_image
fights standard dataflow in a way that indicates to me that this needs rethinking.I've put together a refactor that I think significantly improves legibility, code size, and extensibility. It removes
1,700187 lines ofcodetext and makes this interface much more straightforward to work with. The core ideas being:VisitMorphTargets
in favor of iterators overMorphAttributes
images.get(handle).map(|i| i.texture_view.clone()
). This is called exactly once ... I don't think it merits abstraction.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction: only 187 lines apparently I grossly misused
git diff --stat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea behind
MorphAttributesImage
was to avoid publicly exposing the internal representation of a morph target. It's very "bytely" typed (like stringly typed but with bytes). What if we change the way each weight is laid out in the image? What if instead of an image we switch to a buffer or a more complex representation that can be spliced (for example, to handle subsetting the image to only relevant weights to reduce GPU bandwidth usage)The
Image
is also fairly difficult to construct and tightly bound to our shader, so it becomes a perilous path to tread for the user. Though I tend to fall on the side "let the user do whatever they want"I really like the rest of your proposed changes. I didn't know it was possible to nest
impl
s like that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect splitting this in two different components is preferable. Otherwise it might become a bit awkward for the user to Query MorphWeights for specific mesh (as glTF mesh) or mesh (as bevy mesh)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The traits are a bit involved here. But two things to keep in mind:
bevy_gltf
already has a visitor implementation.VisitMorphTarget
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API is a bit involved. I think maybe the
Handle<AnimationClip>
could be stored in theAnimationPlayer
, or a different component. But I didn't want to revamp the whole ofbevy_animation
because of #8357, so I decided to just provide thiscompatible_with
method. to make it possible to use animation clips correctly with morph targets.The issue already exists. Morph targets exacerbates it by causing a panic when they are misused. I just think it's not part of the scope of this already large PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The big list of "available keys" is a bit of a ugly hack, but I'm not sure there is a better way of handling it. Any suggestion?