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

AnimatedSprite{2D,3D} improvements #65609

Merged
merged 1 commit into from
Jan 5, 2023

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Sep 10, 2022

  • 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.

@dalexeev

This comment was marked as resolved.

@Mickeon
Copy link
Contributor

Mickeon commented Sep 10, 2022

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.

@TokageItLab
Copy link
Member

TokageItLab commented Sep 10, 2022

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.

@dalexeev
Copy link
Member Author

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).

@Mickeon
Copy link
Contributor

Mickeon commented Sep 10, 2022

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.

@fire-forge
Copy link
Contributor

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.

@dalexeev
Copy link
Member Author

dalexeev commented Sep 10, 2022

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.

@TokageItLab TokageItLab requested a review from a team September 10, 2022 16:11
@KoBeWi
Copy link
Member

KoBeWi commented Sep 10, 2022

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.

@dalexeev dalexeev force-pushed the animated-sprite branch 2 times, most recently from 2ab118e to 71bd52e Compare September 12, 2022 13:47
scene/2d/animated_sprite_2d.cpp Outdated Show resolved Hide resolved
scene/3d/sprite_3d.cpp Show resolved Hide resolved
@dalexeev dalexeev force-pushed the animated-sprite branch 3 times, most recently from 4a5f61f to 7bfe50f Compare November 26, 2022 14:32
@TokageItLab
Copy link
Member

TokageItLab commented Dec 6, 2022

Regardless of the naming for props, I think this is in a mergeable state for now, and as I mentioned in #69621, it would be better to merge this as long as we don't know where AnimationTexture is going to go in the future.

@TokageItLab TokageItLab requested a review from akien-mga December 6, 2022 11:21
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

Approved in the previous animation meeting. Breaks compat, should probably be mergeable, reduz' feedback doesn't seem like a blocker.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Some suggestions and corrections for the documentation, as requested.

doc/classes/AnimatedSprite2D.xml Outdated Show resolved Hide resolved
doc/classes/AnimatedSprite2D.xml Outdated Show resolved Hide resolved
doc/classes/AnimatedSprite2D.xml Outdated Show resolved Hide resolved
doc/classes/AnimatedSprite2D.xml Outdated Show resolved Hide resolved
doc/classes/AnimatedSprite2D.xml Outdated Show resolved Hide resolved
doc/classes/SpriteFrames.xml Outdated Show resolved Hide resolved
doc/classes/SpriteFrames.xml Outdated Show resolved Hide resolved
doc/classes/SpriteFrames.xml Outdated Show resolved Hide resolved
doc/classes/SpriteFrames.xml Outdated Show resolved Hide resolved
doc/classes/SpriteFrames.xml Outdated Show resolved Hide resolved
@dalexeev dalexeev requested review from YuriSizov and removed request for akien-mga and reduz January 4, 2023 18:11
@dalexeev
Copy link
Member Author

dalexeev commented Jan 4, 2023

Note: The docs of other SpriteFrames methods should also be improved in the future.

* Add support for individual frame duration to `SpriteFrames`.
* Various minor improvements.
Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Docs are good now.

@akien-mga akien-mga merged commit 798582a into godotengine:master Jan 5, 2023
@akien-mga
Copy link
Member

Thanks!

@dalexeev dalexeev deleted the animated-sprite branch January 5, 2023 14:27
@dalexeev
Copy link
Member Author

dalexeev commented Jan 5, 2023

@TokageItLab @YuriSizov Thanks for your reviews!

@FeralBytes
Copy link
Contributor

FeralBytes commented Jan 11, 2023

Latest documentation is not good, https://docs.godotengine.org/en/latest/classes/class_spriteframes.html still has a method of get_frame() but should be get_frame_texture(). This was merged for beta11.

@dalexeev
Copy link
Member Author

Latest documentation is not good, https://docs.godotengine.org/en/latest/classes/class_spriteframes.html still has a method of get_frame() but should be get_frame_texture().

As far as I know, the docs on the website is updated with some delay.

@dalexeev
Copy link
Member Author

Doesn't break compatibility (see godotengine/godot-proposals#2214 (comment)):

#ifndef DISABLE_DEPRECATED
// For compatibility.
Ref<Resource> res = frames[j];
if (res.is_valid()) {
Frame frame = { res, 1.0 };
anim.frames.push_back(frame);
continue;
}
#endif

@YuriSizov
Copy link
Contributor

I would say that presence of compatibility code doesn't mean it doesn't break it. In this case it's tricky a bit, because DISABLE_DEPRECATED was added in #73741, not in this PR. So technically this PR just supports 2 formats, and #73741 breaks compatibility.

The point is that such compatibility code is temporary and optional.

@dalexeev
Copy link
Member Author

The frames property was renamed to sprite_frames not in this PR, but in #71907 (diff). The data loss on conversion fixed in #73741 was a consequence of how the converter works (regexp based approach), not this PR or #71907 (it has a compatibility code, diff).

The point is that such compatibility code is temporary and optional.

If we consider the code inside #ifndef DISABLE_DEPRECATED as temporary and "non-existent", then I agree with you.

@IsotoxalDev
Copy link

Please mention the unit of duration for the parameter in add_frame. get_frame_duration gives relative duration. Is the duration in add_frame also relative duration? Is it in seconds?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the ability to set the duration of individual AnimatedSprite frames