Repository navigation
Conversation
Leinnan
changed the title
alice-i-cecile
added
C-Feature
|
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. |
There was a problem hiding this comment.
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
added
S-Waiting-on-Author
|
@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 😃 |
consistent and reduce chance of diff with the same content but in different order
…sponse more" This reverts commit 9523c69.
alice-i-cecile
added
S-Needs-Review
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
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 :)
| /// For more details, see the [`specify_animation_system`] documentation. | ||
| #[derive(Debug, Component, Reflect, Clone, Copy, Default)] | ||
| #[reflect(Component)] | ||
| pub struct DontUseDefaultAnimationTime; |
There was a problem hiding this comment.
Bikeshedding help please: I don't like this name very much. It's extremely long, and the missed apostrophe bothers me.
There was a problem hiding this comment.
- NoAutomaticAnimationClock
- CustomAnimationTimeSource
- ExplicitAnimationTime
| /// 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>( |
There was a problem hiding this comment.
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
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
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.
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
|
@alice-i-cecile Any updates on this? 😅 |
This branch has not been deployed
4 participants
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