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

add remove_on_completed flag and new_self_removing fn to Animator and AssetAnimator #122

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gavlig
Copy link

@gavlig gavlig commented Apr 4, 2024

Helps with reducing the amount of systems that perform Animator and AssetAnimator cleanup after animations are completed.

…nd AssetAnimator

Helps with reducing the amount of systems that perform Animator and AssetAnimator cleanup after animations are completed
@djeedai
Copy link
Owner

djeedai commented Apr 27, 2024

Wouldn't this not be best accomplished with the one-shot change from #122 and registering your own system so you can clean-up things however you want?

I absolutely see the value of having something to run at the end of an animation, and having one-off animations. This is one of the main use cases of this library. However, Bevy ECS is not super encouraging adding/removing components all the time (this is costly), so I'm afraid making this trivial with a bool could encourage some bad pattern. I think it's fine to remove an Animator if you really have a one-off animation, and you really have a strong reason to not want the Animator around anymore after that; however I'm not sure I have a great example of what such a reason would be. Worried about performance of having Animators lying around on objects? If the entity is going to get deleted, then it doesn't matter, and if it stays for a long time alive, chances are you might need to animate it again, no? (and so, would need the Animator once more)

@djeedai djeedai added the enhancement New feature or request label Apr 27, 2024
@gavlig
Copy link
Author

gavlig commented Apr 29, 2024

I can provide my use case! I'm writing an IDE called Kodiki and Bevy Tweening is, as you would expect, used to add various effects to liven things up. One of the effects here directly polls for Animator for its own state management and there are a few more examples like this (I can dig them up on request). Apart from state management I also poll engine for animators to maintain certain fps, basically if there is any animation going on I'm keeping fps high, otherwise throttle into oblivion to preserve precious cpu time and not consume more battery than needed.

Now to anti-patterns and removing component being costly, I didn't know that! Do you know of a benchmark or some overview that can shine more light on this for me? (maybe I need to dig through Bevy's ECS code too eventually, but any hints would help greatly).

As for one-shot systems, maybe that will work too, though it does seem a bit more complex than one bool as you mentioned already. I think I can even rewrite the logic that looks at Animator component instead and analyzes it's state, but for now that seems like a worse tradeoff for my use-case since I don't expect to have hundreds of simultaneous animations running at the same time, but the list of entities with Animator will grow for sure and if I don't clean them up the systems that iterate over them will become slower and slower eventually especially the ones that manage fps.

Cheers and thanks for looking into this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants