Load animations in assets loader all at once instead of defining individual ones

Basically, can I do this

#[asset(path = "player/player.trickfilm.ron", collection(typed))]
pub player_animations: Vec<Handle<AnimationClip2D>>,

instead of having to do this?

#[asset(
    paths(
        "player/player.trickfilm.ron#idle",
        "player/player.trickfilm.ron#run",
        "player/player.trickfilm.ron#punch1",
        "player/player.trickfilm.ron#punch1recover",
        "player/player.trickfilm.ron#punch2",
        "player/player.trickfilm.ron#punch2recover",
        "player/player.trickfilm.ron#stagger-normal",
        "player/player.trickfilm.ron#kick1",
        "player/player.trickfilm.ron#kick1recover",
        "player/player.trickfilm.ron#kick2",
        "player/player.trickfilm.ron#kick2recover",
        "player/player.trickfilm.ron#punch3",
        "player/player.trickfilm.ron#punch3recover",
        "player/player.trickfilm.ron#stagger-flying",
    ),
    collection(typed)
)]
pub player_animations: Vec<Handle<AnimationClip2D>>,

My rust macro knowledge is way to limited to understand what happens here under the hood, though I reckon one would need to adjust the .trickfilm.ron file to not be a dict but an array? That would be a breaking change and not so cool, so I am wondering if there is a way to have both?

Because for bigger ron files it makes much more sense to just collect them all into a vec and define all the indices in consts or enums to handle them properly. I guess another possibility to handle bigger files is to break them off into multiple animations: Vec<Handle<AnimationClip2D>>, but that seems a little cumbersome and I honestly don't see a good reason to do that (except if maybe enemies share same animation data as the player or something similar).

Oh I just remembered that I think this was supported in the past? Oh yeah I guess you can still do it somewhat using the AnimationClip2DSet, although this seems even more cumbersome (cbe2f81 has an example). Hm I guess it's not quite easy then to just collect them into a Vec<Handle<AnimationClip2D>> without specifying each animation path?

Basically, can I do this

-snip-

instead of having to do this?

-snip-

My rust macro knowledge is way to limited to understand what happens here under the hood, though I reckon one would need to adjust the .trickfilm.ron file to not be a dict but an array? That would be a breaking change and not so cool, so I am wondering if there is a way to have both?

For what you want to do it has less to do with bevy_trickfilm and more to do with bevy_asset_loader. An AssetLoader will always return a Handle. For trickfilm it will return a Handle\. Additionally it provides labeled assets, that are Handle\. Now what bevy_asset_loader does is loading multiple assets and collecting the handles in a collection. In short there is no way to just grab every animation clip in its current form. I do not think you can query for all labeled assets and even if you could they do not have to be of the same type, so collecting to a vec would not always work (for trickfilm this is actually the case, but again I am not aware of a way to grab all labeled assets and that would also need to be supported by bevy_asset_loader).

Because for bigger ron files it makes much more sense to just collect them all into a vec and define all the indices in consts or enums to handle them properly. I guess another possibility to handle bigger files is to break them off into multiple animations: Vec<Handle<AnimationClip2D>>, but that seems a little cumbersome and I honestly don't see a good reason to do that (except if maybe enemies share same animation data as the player or something similar).

Could you explain why you would prefer a Vec over the HashMap in AnimationClip2DSet? Do you just not want to go through Res\<Assets\>?

Oh I just remembered that I think this was supported in the past? Oh yeah I guess you can still do it somewhat using the AnimationClip2DSet, although this seems even more cumbersome (cbe2f81 has an example). Hm I guess it's not quite easy then to just collect them into a Vec<Handle<AnimationClip2D>> without specifying each animation path?

It was a Vec in the past, but I switched to a HashMap because the animation clips had unique names anyways and "indexing" by that name seemed better than relying on the order of animation in the file.

Some thoughts to support both Vec and HashMap (at least user facing):

Use something like indexmap inside AnimationClip2DSet that allows you to get clips also by insertion order index (Same thing regarding Res\<Assets\> still applies).
Rework trickfilm files so they only define a single AnimationClip2D. You would order your sets by folder. It is more files, but no more content than we currently have. The problem: bevy_asset_loader loading by path does not work on the web and the solution using "Files" means you have to write down your list of files in another file (essentially the same thing you currently do in the macro). Furthermore doing the same thing without bevy_asset_loader got a lot more complicated now.

For what you want to do it has less to do with bevy_trickfilm and more to do with bevy_asset_loader. An AssetLoader will always return a Handle. For trickfilm it will return a Handle. Additionally it provides labeled assets, that are Handle. Now what bevy_asset_loader does is loading multiple assets and collecting the handles in a collection. In short there is no way to just grab every animation clip in its current form. I do not think you can query for all labeled assets and even if you could they do not have to be of the same type, so collecting to a vec would not always work (for trickfilm this is actually the case, but again I am not aware of a way to grab all labeled assets and that would also need to be supported by bevy_asset_loader).

Yeah okay I kinda expected something like this.

Could you explain why you would prefer a Vec over the HashMap in AnimationClip2DSet? Do you just not want to go through Res<Assets>?

Yep it's just that. I also don't see a huge benifit in a HashMap to be honest. The reason is that you always have two points of failure because you are referncing with a string, so if you ever change your animation name in the trickfilm file you will also have to do so in your rust code. If you had a Vec you would instead need to care about your indexing, for which I would implement some kind of enum or consts which you also need to keep in sync with the trickfilm file, so you don't really gain too much from a HashMap (at least the way I would use them).

Though I am not quite sure I understand correctly, in the past the Handle<AnimationClip2DSet> was a Vec and now it is a HashMap? If that is the case then I think it doesn't matter too much either way because you need Res<Assets<AnimationClip2DSet>> regardless of the underlying implementation.

It would also be a huge pain to always query for two resources and even more so when you pass it to other functions, for example I use this pattern a lot

pub fn state_animation(&self, assets: &Res<GameAssets>) -> (Handle<AnimationClip2D>, bool) {

where in this case for example I am just matching some states and get the corresponding animation. While you could to this with the AnimationSet too it makes things more tedious (I would rather have the tediousness in the asset macro).

I guess I will just accept that this is how it is. It isn't that big of a deal to begin with, I was just wondering if there is an easy way to make it more streamlined but if it's such a pain in the ass then it's absolutely not worth it, I can very much live with how things are now.

Could you explain why you would prefer a Vec over the HashMap in AnimationClip2DSet? Do you just not want to go through Res?

Yep it's just that. I also don't see a huge benifit in a HashMap to be honest. The reason is that you always have two points of failure because you are referncing with a string, so if you ever change your animation name in the trickfilm file you will also have to do so in your rust code. If you had a Vec you would instead need to care about your indexing, for which I would implement some kind of enum or consts which you also need to keep in sync with the trickfilm file, so you don't really gain too much from a HashMap (at least the way I would use them).

The benefit I saw in it is that you are independant from order. That is if you restructure your trickfilm file (reorder or add new clip somewhere other than the end), no changes in code are necessary. I don't think there is any additional sync necessary (though maybe I misunderstood you), but what I like about the Hashmap is that you either get that specific animation (identified by name) or you get nothing. With indexing, if you do mess up/do not sync, then you may get some random animation.

Though I am not quite sure I understand correctly, in the past the Handle<AnimationClip2DSet> was a Vec and now it is a HashMap? If that is the case then I think it doesn't matter too much either way because you need Res<Assets<AnimationClip2DSet>> regardless of the underlying implementation.

Yeah, exactly. Same thing.

It would also be a huge pain to always query for two resources and even more so when you pass it to other functions, for example I use this pattern a lot
pub fn state_animation(&self, assets: &Res<GameAssets>) -> (Handle<AnimationClip2D>, bool) {
where in this case for example I am just matching some states and get the corresponding animation. While you could to this with the AnimationSet too it makes things more tedious (I would rather have the tediousness in the asset macro).

Is it really such a huge pain to through Res<Assets<AnimationClip2DSet>> in there :D? Would just be just one more argument and one more "get" (which you could even do in update_player_animation if that is the code you are refering to; state_animation could just get a AnimationClip2DSet)

The benefit I saw in it is that you are independant from order. That is if you restructure your trickfilm file (reorder or add new clip somewhere other than the end), no changes in code are necessary. I don't think there is any additional sync necessary (though maybe I misunderstood you), but what I like about the Hashmap is that you either get that specific animation (identified by name) or you get nothing. With indexing, if you do mess up/do not sync, then you may get some random animation.

Okay true, I agree. Though you do need to make sure your keys (animation names) are synced. Granted, you are much less likely to rename your animations in your trickfilm then you are to reorder them, but it's still something that you wouldn't want to hardcode in the code. Instead you would want to create either a bunch of const keys or just create an enum or something like that. This way you always need to only change one place when you rename you animations in your trickfilm file.

And that is exactly the same if you had indices, you would create one enum or something similar where you reference the index of the animation through that enum, if you reorder your trickfilm file you only need to touch that enum.

So in that regard, if we wanted to scale either implementation, they end up being the same (having hardcoded indices is definitely worse then key strings but neither is going to scale very well).

Is it really such a huge pain to through Res<Assets> in there :D? Would just be just one more argument and one more "get" (which you could even do in update_player_animation if that is the code you are refering to; state_animation could just get a AnimationClip2DSet)

Yes. Yes it is haha. I love having one single giant struct with all my asset handles inside that I can just pass to any function + &mut commands and there we go. Again, I would rather add each animation to my macro definition then to deal with a second resource in my rust code. I have less of a problem refactoring something like my assets.rs, but refactoring function signatures isn't very fun.

To begin with this was a minor nitpick, I was just wondering if there is an easy way to go about adding all animations at once, but there isn't. And honestly, as long as I don't have to make the actual pixel art of the animations I really have nothing to complain about :D

Fair enough. Sorry I could not come up with a satisfactory solution. Thanks for raising the issue anyways - for sure worth to keep in mind for future development. I have been meaning to look how Unity/Godot/whatever handles 2d animations and I still did not get it. Maybe there is something there to learn from.

Fair enough. Sorry I could not come up with a satisfactory solution.

Oh no bro, not at all. I am glad this crate exists at all, if I had to implement it would be a lot more hacky and definitely a lot worse.

Thanks for raising the issue anyways - for sure worth to keep in mind for future development. I have been meaning to look how Unity/Godot/whatever handles 2d animations and I still did not get it. Maybe there is something there to learn from.

Oh yeah I agree. The godot animation signal stuff was the whole reason I even thought about having access to frames in the first place.

KirmesBude / bevy_trickfilm

Load animations in assets loader all at once instead of defining individual ones #30