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

Make the animated_mesh example more intuitive #17421

Merged

Conversation

greeble-dev
Copy link
Contributor

@greeble-dev greeble-dev commented Jan 18, 2025

Objective

Make the animated_mesh example more intuitive and easier for the user to extend.

Solution

The animated_mesh example shows how to spawn a single mesh and play a single animation. The original code is roughly:

  1. In setup_mesh_and_animation, spawn an entity with a SceneRoot that will load and spawn the mesh. Also record the animation to play as a resource.
  2. Use play_animation_once_loaded to detect when any animation players are spawned, then play the animation from the resource.

When I used this example as a starting point for my own app, I hit a wall when trying to spawn multiple meshes with different animations. play_animation_once_loaded tells me an animation player spawned somewhere, but how do I get from there to the right animation? The entity it runs on is spawned by the scene so I can't attach any data to it?

The new code takes a different approach. Instead of a global resource, the animation is recorded as a component on the entity with the SceneRoot. Instead of detecting animation players spawning wherever, an observer is attached to that specific entity.

This feels more intuitive and localised, and I think most users will work out how to get from there to different animations and meshes. The downside is more lines of code, and the "find the animation players" part still feels a bit magical and inefficient.

Side Notes

  • The solution was mostly stolen from AnimationPlayer doesn't explain how it is created #14852 (comment).
  • The example still feels too complicated.
    • "Why do I have to make this graph to play one animation?"
    • "Why can't I choose and play the animation in one step and avoid this temporary component?"
    • I think this requires engine changes.
  • I originally started on a separate example of multiple meshes (branch).
    • I decided that the user could probably work this out themselves from the single animation example.
    • But maybe still worth following through.

Testing

cargo run --example animated_mesh

Copy link
Contributor

@rparrett rparrett left a comment

Choose a reason for hiding this comment

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

I dig the major changes here. Have some minor thoughts about comment tweaks.

Comment on lines 46 to 60
// Create a SceneRoot component that will spawn our mesh after it has loaded.
let mesh_scene = SceneRoot(asset_server.load(GltfAssetLabel::Scene(0).from_asset(GLTF_PATH)));

// Create a component that records which animation we want to play on the
// mesh once it has spawned.
let animation_to_play = AnimationToPlay {
graph_handle,
index,
});
};

// Tell the engine to start loading our mesh and animation, and then spawn
// them as a scene when ready.
commands.spawn(SceneRoot(
asset_server.load(GltfAssetLabel::Scene(0).from_asset(GLTF_PATH)),
));
// Spawn an entity with our components and connect it to our
// play_animation_once_loaded trigger.
commands
.spawn((mesh_scene, animation_to_play))
.observe(play_animation_once_loaded);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nits:

Why create these bindings that are used only once? Just for tidyness of the comments? I think that something like below looks fine.

Some suggestions for the comments themselves down there too.

  • I don't think we need to explain that spawn creates an entity with the given components at this stage
  • It would be great to mention that the mesh will be buried in a scene hierarchy
    commands
        .spawn((
            // The SceneRoot component will spawn the scene hierarchy, including our mesh
            // after the scene has loaded.
            SceneRoot(asset_server.load(GltfAssetLabel::Scene(0).from_asset(GLTF_PATH))),
            // Store the specific animation we want to play once the mesh has spawned.
            AnimationToPlay {
                graph_handle,
                index,
            },
        ))
        // This observer is triggered `SceneInstanceReady` and will run once the scene has spawned
        .observe(play_animation_once_loaded);

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've made updates in 36f62fa

I've kept the step by step setup. Maybe just personal taste but I think it's clearer for beginners.

On the "spawn an entity" comment, I think this makes the narrative flow. Also updated it to emphasise that the observer is connected to that particular entity - I was worried the user might assume it's a global thing.

I've expanded a bit on the scene and hierarchy. I chose to explain the hierarchy in the trigger as that's where it really comes into play.

.run();
}

#[derive(Resource)]
struct Animations {
// A component that records what animation we want to play. This is created when
Copy link
Contributor

Choose a reason for hiding this comment

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

"records what animation" feels a little strange.

Maybe "stores a reference to the animation we want to play"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, although I changed "the animation" to "an animation". Which reads a little clunky, but hints that the component can be reused with different animations.

@alice-i-cecile alice-i-cecile added C-Examples An addition or correction to our examples A-Animation Make things move and change over time D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 19, 2025
Also renamed the trigger - `play_animation_when_loaded` is kinda wrong since it has to load *and* spawn, and it's technically the scene and not the animation. `play_animation_when_ready` is more correct in a somewhat vague way, and the comments cover the details.
@rparrett rparrett added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 19, 2025
Co-authored-by: Rob Parrett <[email protected]>
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jan 20, 2025
Merged via the queue into bevyengine:main with commit b2f3248 Jan 20, 2025
28 checks passed
mrchantey pushed a commit to mrchantey/bevy that referenced this pull request Feb 4, 2025
# Objective

Make the `animated_mesh` example more intuitive and easier for the user
to extend.

# Solution

The `animated_mesh` example shows how to spawn a single mesh and play a
single animation. The original code is roughly:

1. In `setup_mesh_and_animation`, spawn an entity with a SceneRoot that
will load and spawn the mesh. Also record the animation to play as a
resource.
2. Use `play_animation_once_loaded` to detect when any animation players
are spawned, then play the animation from the resource.

When I used this example as a starting point for my own app, I hit a
wall when trying to spawn multiple meshes with different animations.
`play_animation_once_loaded` tells me an animation player spawned
somewhere, but how do I get from there to the right animation? The
entity it runs on is spawned by the scene so I can't attach any data to
it?

The new code takes a different approach. Instead of a global resource,
the animation is recorded as a component on the entity with the
SceneRoot. Instead of detecting animation players spawning wherever, an
observer is attached to that specific entity.

This feels more intuitive and localised, and I think most users will
work out how to get from there to different animations and meshes. The
downside is more lines of code, and the "find the animation players"
part still feels a bit magical and inefficient.

# Side Notes

- The solution was mostly stolen from
bevyengine#14852 (comment).
- The example still feels too complicated.
    - "Why do I have to make this graph to play one animation?"
- "Why can't I choose and play the animation in one step and avoid this
temporary component?"
    - I think this requires engine changes.
- I originally started on a separate example of multiple meshes
([branch](bevyengine/bevy@main...greeble-dev:bevy:animated-mesh-multiple)).
- I decided that the user could probably work this out themselves from
the single animation example.
    - But maybe still worth following through.

# Testing

`cargo run --example animated_mesh`

---------

Co-authored-by: Rob Parrett <[email protected]>
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-Examples An addition or correction to our examples D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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