Skip to content

Animations- support for specifying Time type with QueryFilter #20717

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Animations- support for specifying Time type with QueryFilter #20717

wants to merge 19 commits into from
+359 −12

Conversation

Leinnan
Copy link
Contributor

@Leinnan Leinnan commented Aug 22, 2025

Objective

Fixes #20716

Solution

Adds API that will make it possible to use different Time types for various animation players based on the FilterQuery.

Testing

  • I updated the animated_transform sample to use the new api and the virtual time instead of regular Time resource.

@Leinnan Leinnan changed the title Naive implementation Animations- support for specifying Time type with QueryFilter Aug 22, 2025
@james7132 james7132 self-requested a review August 22, 2025 18:58
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-Animation Make things move and change over time labels Aug 24, 2025
@github-actions GitHub Actions
Copy link
Contributor

It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note.

Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes.

alice-i-cecile
alice-i-cecile reviewed Aug 24, 2025
View reviewed changes
alice-i-cecile
alice-i-cecile reviewed Aug 24, 2025
View reviewed changes
alice-i-cecile
alice-i-cecile reviewed Aug 24, 2025
View reviewed changes
alice-i-cecile
alice-i-cecile requested changes Aug 24, 2025
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Needs a lot more docs to clearly explain why you might want to do this and how it can be used. But I really like the core idea, and don't hate the implementation.

IMO a usage example for this is more warranted, rather than bloating the existing example. We have a few clear use cases in mind: fixed updates for fighting game stability, or pausing animations when the game pauses. We should at least hint at that directly in the learning material.

I also think this warrants a release note: the usage in fighting games via fixed update is extremely interesting.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 24, 2025
@Leinnan Leinnan requested a review from alice-i-cecile August 25, 2025 11:06
@Leinnan
Copy link
Contributor Author

Leinnan commented Aug 25, 2025

@alice-i-cecile Feedback applied 🫡 . One thing- I will be out for the next week, so if there some further changes required to make it into repo anyone else can feel free to create PR out of it instead if they can make it before I come back from vacation. The main thing for me is having that functionality built into the engine, not getting credit for it 😃

Leinnan added 3 commits August 25, 2025 22:19
@Leinnan
Update lib.rs
4a1388d
@Leinnan
Sort get_registered_reflect_types response. It makes the response more
9523c69 
consistent and reduce chance of diff with the same content but in
different order
@Leinnan
Revert "Sort get_registered_reflect_types response. It makes the re…
3a2b75e 
…sponse more"

This reverts commit 9523c69.
@alice-i-cecile alice-i-cecile 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 25, 2025
alice-i-cecile
alice-i-cecile reviewed Aug 25, 2025
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Much improved! The docs need a copyediting pass, but I'll wait to see what our Animation experts think about the architecture before fixing that up for you :)

viridia
viridia reviewed Aug 25, 2025
View reviewed changes
alice-i-cecile
alice-i-cecile reviewed Sep 4, 2025
View reviewed changes
alice-i-cecile
alice-i-cecile reviewed Sep 4, 2025
View reviewed changes
alice-i-cecile
alice-i-cecile reviewed Sep 4, 2025
View reviewed changes
alice-i-cecile
alice-i-cecile reviewed Sep 4, 2025
View reviewed changes
crates/bevy_animation/src/lib.rs Outdated
/// For more details, see the [`specify_animation_system`] documentation.
#[derive(Debug, Component, Reflect, Clone, Copy, Default)]
#[reflect(Component)]
pub struct DontUseDefaultAnimationTime;
Copy link
Member

Choose a reason for hiding this comment

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

Bikeshedding help please: I don't like this name very much. It's extremely long, and the missed apostrophe bothers me.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • NoAutomaticAnimationClock
  • CustomAnimationTimeSource
  • ExplicitAnimationTime

alice-i-cecile
alice-i-cecile reviewed Sep 4, 2025
View reviewed changes
crates/bevy_animation/src/lib.rs Outdated
/// It allows to specify which Time resource to use based on the [`bevy_ecs::query::QueryFilter`].
/// That can be especially useful when you want to use a custom time resource for subset of animations.
/// By default, there is added a one version with Time<()> and Without [`DontUseDefaultAnimationTime`] component.
pub fn specify_animation_system<T: Default, F: bevy_ecs::query::QueryFilter + 'static>(
Copy link
Member

Choose a reason for hiding this comment

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

I don't think "specify" is the right word here. I think this would be a lot clearer if this was an ordinary struct-based plugin called TimeDependentAnimationPlugin or something

alice-i-cecile
alice-i-cecile requested changes Sep 4, 2025
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Okay, so I'm pretty sold on the utility and the core implementation of this, but the clarity still needs a lot of work. I've left some suggestions that I think would help fix that.

@Leinnan
Copy link
Contributor Author

Leinnan commented Oct 1, 2025

@alice-i-cecile Any updates on this? 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@james7132 james7132 Awaiting requested review from james7132

@maniwani maniwani Awaiting requested review from maniwani

@viridia viridia Awaiting requested review from viridia

@alice-i-cecile alice-i-cecile Awaiting requested review from alice-i-cecile

@mockersf mockersf Awaiting requested review from mockersf

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
A-Animation Make things move and change over time C-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for specifying Time type for animations
3 participants
@Leinnan @viridia @alice-i-cecile