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

Make AnimatedSprite2D update animation property when SpriteFrames changes #58966

Closed

Conversation

L4Vo5
Copy link
Contributor

@L4Vo5 L4Vo5 commented Mar 10, 2022

AnimatedSprite2D will ensure it has a valid animation when the SpriteFrames is set, and on its "changed" signal.
To that end, makes SpriteFrames emit the "changed" signal when one or all of its animations are deleted.
Note that the animation property will remain as-is if the SpriteFrames has no animations at all.

Removes error message on set_animation when SpriteFrames is null, which caused "no animation with name X" errors even with the "default" value of freshly created instances.
So, this allows the animation property to be set even without a valid SpriteFrames. And once one is added, the animation will be changed if necessary to make it valid, making the error pointless.

Fixes #45459 and #28658.
Related to #49495 on the 3.x branch.

@L4Vo5 L4Vo5 requested a review from a team as a code owner March 10, 2022 05:16
@L4Vo5 L4Vo5 force-pushed the animatedsprite2d-valid-animation branch from 2f17966 to 5591575 Compare March 10, 2022 05:22
@Chaosus Chaosus added this to the 4.0 milestone Mar 10, 2022
@akien-mga akien-mga requested a review from kleonc July 4, 2022 09:46
@L4Vo5 L4Vo5 force-pushed the animatedsprite2d-valid-animation branch from 5591575 to b1bbe52 Compare July 4, 2022 20:24
@L4Vo5
Copy link
Contributor Author

L4Vo5 commented Jul 4, 2022

Changed the files according to the suggestions
on _res_changed(), I put the second set_frame on an else block. After all, set_animation shouldn't quit early, and it'll call set_frame(0); itself

Copy link
Member

@kleonc kleonc left a comment

Choose a reason for hiding this comment

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

@L4Vo5 Changes made in AnimatedSprite2D would need to be done in AnimatedSprite3D too.

Also as you can see static checks fail (formatting issues), you'd need to apply clang-format to fix these. I recommend adding relevant pre-commit hooks to run it automatically when you commit (so you can fix formatting issues immediately).

Comment on lines 88 to 89
animations.erase(p_anim);
emit_changed();
Copy link
Member

Choose a reason for hiding this comment

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

HashMap::erase returns a bool telling whether the erasing happened.

Suggested change
animations.erase(p_anim);
emit_changed();
if (animations.erase(p_anim)) {
emit_changed();
}

@@ -261,6 +261,10 @@ void AnimatedSprite2D::set_sprite_frames(const Ref<SpriteFrames> &p_frames) {
frames = p_frames;
if (frames.is_valid()) {
frames->connect("changed", callable_mp(this, &AnimatedSprite2D::_res_changed));
Vector<String> animation_names = frames->get_animation_names();
Copy link
Member

Choose a reason for hiding this comment

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

@akien-mga This returns sorted animation names, as desired (so its first element is indeed what we want):

Vector<String> SpriteFrames::get_animation_names() const {
Vector<String> names;
for (const KeyValue<StringName, Anim> &E : animations) {
names.push_back(E.key);
}
names.sort();
return names;
}

But in other places in the source code SpriteFrames::get_animation_list() is being used instead and the obtained list is being sorted separately:

void SpriteFrames::get_animation_list(List<StringName> *r_animations) const {
for (const KeyValue<StringName, Anim> &E : animations) {
r_animations->push_back(E.key);
}
}

List<StringName> anim_names;
frames->get_animation_list(&anim_names);
anim_names.sort_custom<StringName::AlphCompare>();

Both approaches seem to result in the same ordering (both end up comparing with is_str_less). But I think we should be consistent and use one over the other. I'd guess sorting a list is probably slower (it probably doesn't matter though) but in the list version there's no StringName -> String conversion (which is also cheap I guess). So is any of these approaches preferred?

@L4Vo5 L4Vo5 force-pushed the animatedsprite2d-valid-animation branch from b1bbe52 to 2adf55c Compare July 28, 2022 22:58
@L4Vo5
Copy link
Contributor Author

L4Vo5 commented Jul 28, 2022

Thanks for the clang help!

Changes made in AnimatedSprite2D would need to be done in AnimatedSprite3D too.

Should that be a separate PR?
I've been looking at the code, and there's a few other differences AnimatedSprite3D has compared with 2D. For example: it can't play backwards, it doesn't support speed scale, and it produces no errors on invalid animations.
That last one would make sense to change if I add AnimatedSprite3D to this PR, but since there's more important differences anyways, I think a separate PR to bring the missing features to AnimatedSprite3D would be better. I can get started on that, and if anything just bring the code to this one.

Makes AnimatedSprite2D and AnimatedSprite3D have a valid animation
when setting or changing the SpriteFrames.
Removes error on set_animation when SpriteFrames is null,
which caused errors even with its own default value.
@L4Vo5 L4Vo5 force-pushed the animatedsprite2d-valid-animation branch from 2adf55c to 74a905b Compare September 17, 2022 21:51
@L4Vo5 L4Vo5 requested a review from a team as a code owner September 17, 2022 21:51
@L4Vo5
Copy link
Contributor Author

L4Vo5 commented Sep 17, 2022

Applied changes to AnimatedSprite3D following #64155, which brought the same errors to it (but also made it infinitely easier to make the same changes)

@KoBeWi
Copy link
Member

KoBeWi commented Feb 8, 2023

AnimatedSprite underwent many changes recently and both issues are now closed. Is this PR still relevant?

@L4Vo5
Copy link
Contributor Author

L4Vo5 commented Feb 8, 2023

Hmm, the current fix for 28658 is only in the inspector: it does nothing when deleting animations in code. Still, that makes the issue even more minor than it was before, so this PR is probably not worth keeping as it'd require a full revision to update it just for that.

@L4Vo5 L4Vo5 closed this Feb 8, 2023
@KoBeWi KoBeWi added the archived label Feb 8, 2023
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.

Duplicating AnimatedSprite2D shows error: There is no animation with name 'default'
5 participants