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] Implement duplicate animations #57949

Closed
wants to merge 3 commits into from

Conversation

nonunknown
Copy link
Contributor

@nonunknown nonunknown commented Feb 11, 2022

Implements #3942

PS:

  • As I'm a new contributor, my code can be messed or not following rules, any review to the codebase is welcome!!!
  • Cherrypick for 3.x

Edit:

Just added @Mickeon's idea, to use the duplicated animation's name:

duplicate_2

@Mickeon
Copy link
Contributor

Mickeon commented Feb 11, 2022

That's great. One thing I believe could be done on top of it is duplicating the animation name, as well, but adding an incrementing number to it. (e.g. "anim" duplicates into "anim2", then "anim3", and so on). It would keep it in line with the SceneTree duplication behaviour.

d
@@ -0,0 +1 @@
* 3942
Copy link
Member

Choose a reason for hiding this comment

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

This file has to be removed :)

@@ -662,13 +663,43 @@ void SpriteFramesEditor::_animation_add() {
undo_redo->add_do_method(E, "set_animation", name);
undo_redo->add_undo_method(E, "set_animation", current);
}

Copy link
Member

Choose a reason for hiding this comment

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

These trailing spaces have to be removed.

edited_anim = name;

undo_redo->commit_action();
animations->grab_focus();
}

void SpriteFramesEditor::_animation_duplicate() {
if ( !frames->has_animation(edited_anim) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ( !frames->has_animation(edited_anim) ) {
if (!frames->has_animation(edited_anim)) {

I wonder why CI did not detect this :(


StringName anim_to_duplicate = edited_anim;
int duplicate_frame_count = frames->get_frame_count(anim_to_duplicate);
String new_name = String(edited_anim); //Get the animation name to duplicate, cant use the anim_to_duplicate since its a StringName and cannot be modified
Copy link
Member

Choose a reason for hiding this comment

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

For comments, it's prefered to start with // (with an empty space following //) and end with a period. Serveral other comments need to be fixed too.

Comment on lines +690 to +695
int counter = 1;
while(frames->has_animation(new_name + "_" + itos(counter))) {
counter++;
}
new_name = new_name + "_" + itos(counter);
frames->rename_animation(edited_anim,new_name);
Copy link
Member

Choose a reason for hiding this comment

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

This is inconsistent with how _animation_add() handles name conflicts. It uses "Existing Name N" while this uses "Existing Name_N". I think they should be unified.

Also, there should be a space between while and (. More style issues can be found by running clang-format, see Code Style Guide.

@nonunknown
Copy link
Contributor Author

@Mickeon made the changes you requested,
also superseded by: #60158

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.

None yet

6 participants