godot
godot copied to clipboard
godotengine
`AnimatedSprite{2D,3D}` improvements
- Add support for individual frame duration to
SpriteFrames. - Various minor improvements.
Closes godotengine/godot-proposals#2214.
#65188 recently added similar changes to AnimatedTexture.
You can specify the duration of individual frames (default 1.0 s).
This PR introduces backward incompatible changes to the public API ~and the SpriteFrames serialization format~.
This PR does not supersede #64155. In that PR the code was harmonised and polished. One thing at the time. It doesn't quite supersede #65148 either because the implementation is different. But that's fine.
For the AnimatedTexture, it would make sense to include its own timeline within it as a Resource referenced by other Nodes.
But since AnimatedSprite is a Node, so its frames can be controlled by AnimationPlayer. Therefore, I am a little skeptical that this feature (individual frame duration in AnimatedSprite) is really necessary and I wonder whether there is a need for such comprehensive support.
AnimationPlayer is for advanced cases, AnimatedSprite is for simple ones, but I still want to be able to adjust the duration of individual frames, and not duplicate them.
With this feature it will be much faster to set up the animation. AnimationPlayer is too versatile and inconvenient.
Frame duration/delay is also available in other similar animation editors (for example, in Construct).
I would like this but I am mostly concerned about implementation. The PR introduced many things at once and it's hard to grasp a focus on how the individual frame duration is accomplished. Ideally, it should be completely optional and preferably not noticeable until the user decides to make use of it. Because, as useful as frame duration may be, it's somewhat situational and I would not encourage beginning users to experiment, and see way too many overwhelming numbers.
The changes done in this PR are nice, but I wonder if it would be better to merge SpriteFrames and AnimatedTexture since their feature sets are so similar.
The changes done in this PR are nice, but I wonder if it would be better to merge SpriteFrames and AnimatedTexture since their feature sets are so similar.
An AnimatedTexture can be used as a property value without being added to the tree as a node. For example, in 3.x AnimatedTexture can be used for animated tiles (however, in 4.0-dev, animation in TileMap is done separately). It seems to me that in 4.0 AnimatedTexture is mostly not needed. Only as a workaround for possible corner cases (like animated tiles in 3.x). Or as a replacement for GIF, which is not supported by Godot.
AnimatedSprite is mostly used for characters, NPCs, interactive objects I think.
Perhaps we could add frames property (of type SpriteFrames) to the AnimatedTexture? In this case, we will have the same interface for editing AnimatedSprite2D, AnimatedSprite3D and AnimatedTexture frames. However, SpriteFrames is an animation library (contains multiple animations), and AnimatedTexture only needs one animation.
AnimatedTexture can't really get new features, because it does the animation on the GPU and also it doesn't support texture atlases, unlike AnimatedSprite. They are 2 completely different things.
#65148 has been merged, so this needs to be rebased properly. From what I can see, few changes to AnimatedSprite are needed; You just need to change the timing of getting the duration and updating the frames if you need.
Also, adding the duration property to SpriteFrames would be okay, but get_animation_total_duration() doesn't seem necessary.
Also, adding the duration property to
SpriteFrameswould be okay, butget_animation_total_duration()doesn't seem necessary.
Deleted because most often the total duration of frames is greater than zero. And if not, then there is protection for this, which I mentioned above.
It's very hard to make out whether or not all changes are good. Not saying there aren't, but there's a... lot to unpack in this PR. There's even API changes that are not documented, not even on the commit's description
There's even API changes that are not documented, not even on the commit's description
I tried to document all the API changes and clarify the behavior. If I missed something, please point it out to me. The commit description doesn't really list everything, but I think we can put that question aside for now. Will it help the review if I briefly list the changes and for what purposes they were made?
The implementation of pause() needs to be careful, because similar discussions already exist for AnimationPlayer in #56645.
It may be one way to allow stop() to store the last position of the timeline, as in the #65986, but we cannot decide about it yet without consensus.
For now, I suggest for pause() to be sent in a separate PR so that there is no need for extra discussion in this PR.
The implementation of
pause()needs to be careful, because similar discussions already exist forAnimationPlayerin #56645.
Done. I also believe that play/pause/stop behavior should be consistent across classes.
There is a weird behavior when dragging a frame with a duration set that resets the duration of that frame. Other things seem to be good.
https://user-images.githubusercontent.com/61938263/190900939-b959ede0-07ea-408f-91e2-b359885c1836.mp4
There is a weird behavior when dragging a frame with a duration set that resets the duration of that frame.
Fixed.
I noticed during testing that the fetching of images displayed on AnimationPlayer keyframes is broken, but that happened before this PR so this does not seem to be the cause. I will send an issue later.
Apologies, but I don't think this is a good PR as-is. The default workflow is fine for most users and this makes things more complicated. IMO whatever changes are made should be optional workflow, not changes.
A suggestion from my side is to make default duration 0 (which means, use fps), if you put something else, then this overrides the fps.
@reduz
- This is consistent with recent
AnimatedTexturechanges (PR #65188). - It was mentioned above that in the next PR we can add a tool for multi-editing frame durations (comment).
- In the previous version of this PR, animation's FPS was replaced with
speed_scale(not to be confused withAnimationPlayer*'sspeed_scale). I originally left it out for pretty much the same reasons you're worried about, but then we came to the conclusion that the twospeed_scales are too confusing.
@dalexeev It does not matter, this just makes the usability worse. It is also very likely that AnimatedTexture will be removed before RC because the way it works is no longer efficient on the Vulkan side.
FPS with delay seconds has caused confusion in AnimatedTexture for some time like #64657, and has been resolved by #65188. Therefore, it is recommended to remove the FPS in this PR and replace it with a animation based on duration and speed_scale.
@TokageItLab as I said, I am not very concerned with AnimatedTexture. The main problem here is user friction of changing this and the added complexity. I am not sure that an animation duration is better than an FPS, because animators generally just animate at fixed FPS and then export their animations and open them in Godot. This absolutely breaks workflow. Editing length could, at much, be an optional toggle so you can choose to edit length or FPS.
The main problem here is that the suggested approach in this PR is a hassle by default, and adding tools to set all frames is hacky.
IMO, course of action should be:
- If you want to put a delay per frame, feel free, but it should be optional (0 means ignore).
- If you want to edit length instead of FPS, also feel free but it should also be optional (toggle what you edit with a button).
But changing default behavior should be out of the picture.
@reduz It is simply confusing to put two units, FPS and second, in the same function. It would be easier to combine one unit second, with a unitless value speed_scale.
As a compromise, would it be acceptable if it reverts to FPS and implement a parameter called delay(frame) each frames?
@TokageItLab If we are still uncertain about these things, then I think this PR should be put on the fridge until better feedback is collected. IMO the only thing that should be certain to me is that default behavior should not change, how you add optional delay or customization per frame should be re-evaluated.
@reduz
A suggestion from my side is to make default duration 0 (which means, use fps), if you put something else, then this overrides the fps.
It seems to me that this is more like a magic value. I would expect a frame of duration 0 to simply be skipped (instantly changed to the next), as it is now.
Animation FPS is the inverse of the animation's speed_scale, which is just an additional multiplier to AnimatedSprite*.speed_scale. I don't think this makes much sense, given the possibility of multi-editing.
Although I originally suggested leaving the FPS and not using the s suffix (and think of 1.0 as something like Control.size_flags_stretch_ratio, relative frame duration). If in this sense, then I agree, but it will be inconsistent with AnimatedTexture. Are you sure it will be removed?
Question to all: what is better perceived, relative or absolute value? Which is better: 0.2 s and 0.1 s or 1.0 and 0.5?
@dalexeev Yes, as I mentioned, the way AnimatedTexture works is very inefficient in 4.0 and the main use case for it was animating tilemaps (which now has proper support for it), so will most likely be removed.
leaving the FPS and not using the s suffix (and think of 1.0 as something like Control.size_flags_stretch_ratio, relative frame duration)
For now, I think this is close to the current proposal, so +1 to this. Probably does it need to add int delay? And then I think that setting the default delay value to 0 would be compatible with the old behavior.
Visualization
5 FPS: 1 s / 5 = 0.2 s
If there are no objections, tomorrow morning I will implement these changes.