Skip to content

Conversation

greeble-dev
Copy link
Contributor

@greeble-dev greeble-dev commented Mar 21, 2025

Objective

  • Avoid a subtle system ordering dependency between bevy_animation and bevy_render.
  • Optimise main world CPU and memory.
  • Make the morph target pipeline more flexible.

Summary

The morph target pipeline currently relies on copying morph target weights between components. This has three issues:

First, the copy requires a subtle system ordering dependency between bevy_animation and bevy_render, via a system set declared in bevy_mesh.

// bevy_animation
animate_targets.before(bevy_mesh::InheritWeights)
// bevy_render
app.add_systems(PostUpdate, inherit_weights.in_set(InheritWeights));

Second, the copy is arguably redundant - the weights will be copied again into render buffers.

Third, the copy requires certain entities to be children of other entities. This inflexibility is not a problem right now, but might be in future.

This PR removes the copy. Users who just import glTFs and use bevy_animation are not affected. Users who make lower level changes might need to update their code.

Background

The morph target pipeline lets main world entities set weights which are then automatically copied into per-mesh render buffers.

The current pipeline is:

  • bevy_animation::animate_targets or user code sets weights on a MorphWeights component (basically a Vec<f32>).
  • In the main world update, bevy_render::inherit_weights copies weights from MorphWeights to any MeshMorphWeights components in child entities. MeshMorphWeights is also a Vec<f32>.
  • In the render world extraction, bevy_render::extract_morphs finds any entities with both a Mesh3d and MeshMorphWeights, then copies the weights to per-mesh render buffers.

The separation between MorphWeights and MeshMorphWeights allows a single MorphWeights component in a parent entity to drive multiple meshes in child entities.

Changes

In the new pipeline, MeshMorphWeights is not a copy of MorphWeights. Instead it's a reference to the entity containing MorphWeights.

- struct MeshMorphWeights { weights: Vec<f32> }
+ struct MeshMorphWeights(Entity);

inherit_weights is no longer needed - instead extract_morphs uses MeshMorphWeights to find and copy MorphWeights directly into render buffers.

This is more flexible as the child entity requirement of inherit_weights is gone - a MeshMorphWeights can point to any entity. Performance improves due to reduced copies and the child search becoming a direct entity lookup.

Breakage

Changing MeshMorphWeights from a copy to a reference could affect some users.

Unaffected: Users who import a glTF scene and drive the MorphWeights component themselves or via bevy_animation.

Possibly Affected: Users who set up their own pipeline or modify the glTF pipeline. In particular, if they're manipulating MeshMorphWeights directly then they'll have to restructure to use MorphWeights.

I did a github code search and found only one case that would break. They can fix this by rearranging the components.

Performance

Tested on many_morph_targets (#18536):

  • extract_morphs goes from 42.5us to 48.9us (+6.4us).
  • inherit_weights goes from 30.3us to zero.
  • Overall, that means throughput goes from 14 to 21 meshes per us (+50%).

I expect a small memory saving due to one less copy of the weights per mesh.

There's room for further optimisation. The render buffers have a copy of the weights for each mesh, but multiple meshes could be sharing the same weights.

Testing

cargo run --example morph_targets
cargo run --example many_morph_targets
cargo run --example scene_viewer -- assets/models/animated/MorphStressTest.gltf

Also tested a few other glTFs, and hacked the glTF importer to simulate some valid and invalid combinations (missing weights, mismatched arrays).

Alternatives

The pipeline is a little awkward for someone who wants to put meshes and morph weights on the same entity - they have to add a MorphWeights component, and also a MeshMorphWeights component that simply references its own entity.

An alternative could be to make MeshMorphWeights an enum that's either a reference of an instance of weights:

-struct MeshMorphWeights(Entity)
+enum MeshMorphWeights {
+    Reference(Entity),
+    Instance(Vec<f32>)
+}

Now, the all-in-one entity just needs a MeshMorphWeights::Instance - no need for MorphWeights.

Comment on lines 147 to 152
pub fn clear_weights(&mut self) {
self.weights.clear();
}
pub fn extend_weights(&mut self, weights: &[f32]) {
self.weights.extend(weights);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were added so that MorphWeights supports everything that MeshMorphWeights did.

Personally I'd rather make MorphWeights members public and avoid the need for this interface.

Comment on lines 1564 to 1570
// Create `MorphWeights`. The weights will be copied from `mesh.weights()`
// if present. If not then the weights are zero.
//
// The glTF spec says that all primitives must have the same number
// of morph targets, and `mesh.weights()` should be equal to that
// number if present. We're more forgiving and take whichever is
// biggest, leaving any unspecified weights at zero.
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 played it safe here, to the point of accepting malformed glTFs. I wasn't sure if the importer is intended to be strict or permissive.

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-Animation Make things move and change over time X-Contentious There are nontrivial implications that should be thought through S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Mar 23, 2025
@greeble-dev greeble-dev marked this pull request as ready for review March 28, 2025 12:55
@greeble-dev
Copy link
Contributor Author

Changed from draft to ready for review. Documentation has been updated, and it now fully removes the bevy_animation dependency on bevy_render.

@greeble-dev greeble-dev added S-Needs-Review Needs reviewer attention (from anyone!) to move forward M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide A-Rendering Drawing game state to the screen and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 5, 2025
@atlv24 atlv24 added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed X-Contentious There are nontrivial implications that should be thought through S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 9, 2025
@atlv24
Copy link
Contributor

atlv24 commented Aug 9, 2025

Removed Contentious because I dont see why it would be contentious

@greeble-dev greeble-dev added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Aug 10, 2025
@greeble-dev
Copy link
Contributor Author

PR has been updated to fix conflicts with #20472.

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-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants