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.

Sign up for GitHub

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

Jump to bottom

Allow duplicating entire animations #80380

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

Allow duplicating entire animations #80380

wants to merge 1 commit into from
+55 −0

Conversation

nonunknown
Copy link
Contributor

@nonunknown nonunknown commented Aug 7, 2023 •
edited by AThousandShips
Loading

This is the duplicate animation ability for spriteframes. Sometimes the user want to duplicate entire animations instead of a frame only!

this is a better version of: #67533

With the suggestions of: @KoBeWi , @groud and @volokh0x

demonstration:

PR-duplicate-anim.mp4

thanks guys!!!

@AThousandShips
Copy link
Member

Please rebase instead of merging into the branch

@KoBeWi KoBeWi added this to the 4.x milestone Aug 7, 2023
@AThousandShips
Copy link
Member

Please use git commit --amend to update minor changes

@AThousandShips
Copy link
Member

You need to add documentation, see the CI

@YuriSizov YuriSizov changed the title Allow Duplicate Entire Animations Allow to duplicate entire animations Aug 7, 2023
@nonunknown nonunknown requested a review from a team as a code owner August 7, 2023 16:50
@AThousandShips
Copy link
Member

Documentation has to be in alphabetical order

Please take this opportunity to also squash your commits into one, and rebase onto master to make sure CI doesnt fail

@AThousandShips
Copy link
Member

Your commit message should be descriptive, the PR title for example

AThousandShips
AThousandShips reviewed Aug 7, 2023
View reviewed changes
doc/classes/SpriteFrames.xml
<param index="0" name="anim_from" type="StringName" />
<param index="1" name="anim_to" type="StringName" />
<description>
Duplicate all frames [param anim_from] to the [param anim_to].
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
Duplicate all frames [param anim_from] to the [param anim_to].
Duplicate all frames from [param anim_from] to [param anim_to].

Copy link
Member

Choose a reason for hiding this comment

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

I think the current description is misleading. It sounds like the animation has to already exist, while it doesn't need to.

@AThousandShips AThousandShips changed the title Allow to duplicate entire animations Allow duplicating entire animations Aug 10, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Aug 17, 2023 •
edited
Loading

Found a bug. If you undo duplicating the animation and redo, the frame list is empty until you refresh it manually:

godot.windows.editor.dev.x86_64_XySY1C6LwR.mp4

EDIT:
The naming scheme is wrong:
image
Either the number should be incremented or there shouldn't be number. You could use e.g. _(copy) suffix.
EDIT2: Seems like you do increment, but only if the name doesn't have a suffix already. Then it's a bug.

KoBeWi
KoBeWi reviewed Aug 17, 2023
View reviewed changes
editor/plugins/sprite_frames_editor_plugin.cpp
Comment on lines +1044 to +1045
undo_redo->add_do_method(this, "_update_library");
undo_redo->add_undo_method(this, "_update_library");
Copy link
Member

Choose a reason for hiding this comment

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

I think this should run after other methods.

KoBeWi
KoBeWi reviewed Aug 17, 2023
View reviewed changes
editor/plugins/sprite_frames_editor_plugin.cpp
undo_redo->add_do_method(this, "_update_library");
undo_redo->add_undo_method(this, "_update_library");
undo_redo->add_do_method(frames.ptr(), "duplicate_animation", edited_anim, name);
undo_redo->add_undo_method(frames.ptr(), "remove_animation", 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 called twice.

KoBeWi
KoBeWi reviewed Aug 17, 2023
View reviewed changes
editor/plugins/sprite_frames_editor_plugin.cpp

EditorUndoRedoManager *undo_redo = EditorUndoRedoManager::get_singleton();
undo_redo->create_action(TTR("Duplicate Animation"), UndoRedo::MERGE_DISABLE, EditorNode::get_singleton()->get_edited_scene());
undo_redo->add_do_method(frames.ptr(), "add_animation", name);
Copy link
Member

Choose a reason for hiding this comment

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

duplicate_animation() works even if the new animation does not exist, so this step is redundant.

@Kosyne
Copy link

Kosyne commented Feb 9, 2024

Was this still being worked on? Recently did some tutorials, and this was a real pain point for myself and even the person giving the tutorial.

@pepsipwns
Copy link

Is this still likely to be added? Would love some way to duplicate animations in a AnimatedSprite

@KoBeWi
Copy link
Member

KoBeWi commented Mar 17, 2024

Looks like abandoned PR, someone would have to take over and finish it.

@AThousandShips
Copy link
Member

AFAIK the OP has stopped contributing or participating entirely

@EAinsley
Copy link
Contributor

EAinsley commented Jun 26, 2024 •
edited
Loading

I will continue to work on this.

@EAinsley EAinsley mentioned this pull request Jun 26, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KoBeWi KoBeWi KoBeWi left review comments

@AThousandShips AThousandShips AThousandShips left review comments

@TokageItLab TokageItLab Awaiting requested review from TokageItLab

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
enhancement salvageable topic:animation topic:editor
Projects
None yet
Milestone
4.x
Development

Successfully merging this pull request may close these issues.

Add a Duplicate button and context menu to the SpriteFrames editor
7 participants
@nonunknown @AThousandShips @KoBeWi @Kosyne @pepsipwns @EAinsley @nonunknown777