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] Add ability to duplicate animations #60158

Closed
wants to merge 1 commit into from

Conversation

nonunknown
Copy link
Contributor

This PR supersedes #57949 with the additions that @Mickeon stated!

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

@SaracenOne SaracenOne added topic:editor usability topic:2d cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Apr 11, 2022
@Calinou Calinou added this to the 4.0 milestone Apr 11, 2022
editor/plugins/sprite_frames_editor_plugin.h Outdated Show resolved Hide resolved
editor/plugins/sprite_frames_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/sprite_frames_editor_plugin.cpp Outdated Show resolved Hide resolved
editor/plugins/sprite_frames_editor_plugin.cpp Outdated Show resolved Hide resolved
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.

With that suggested change the code would be ok.
Formatting needs to be fixed though, commits squashed.


This still holds but it's not crucial / can be addressed separately later:

Also it might be a good idea to detect if the name of the animation being duplicated ends with a number. Currently duplicating "New Anim 1" would create "New Anim 1 1", but "New Anim 2" would probably be more sensible.


Since it's marked for PR review and a search box was added in there in the meantime (#49488), can you update the GIF of how it looks now? So others could easily evaluate how this new duplicate button fits in visually.

editor/plugins/sprite_frames_editor_plugin.cpp Outdated Show resolved Hide resolved
@nonunknown
Copy link
Contributor Author

@kleonc fixed everything now!

@kleonc
Copy link
Member

kleonc commented Aug 3, 2022

@kleonc fixed everything now!

Seems like you ignored/skipped this suggestion (animation's loop/speed is still not copied from the animation being duplicated).

@nonunknown
Copy link
Contributor Author

oh my bad, I'm trying my best here, since I have no much knowledge with PRs, sorry for that, can you suggest again, since outdated suggestions doesnt work.

@kleonc
Copy link
Member

kleonc commented Aug 4, 2022

oh my bad, I'm trying my best here, since I have no much knowledge with PRs, sorry for that

No worries. 🙂

can you suggest again, since outdated suggestions doesnt work.

You don't need to do it through the "Apply suggestion" (or whatever that option is called) on GH. Actually, I'd even recommend not using it as it creates a separate commit for each such suggestion application. And what's preferred in the end is a single commit (not 10 like currently in this PR) so you're gonna need to clean this anyway. Meaning the log is now kinda polluted with some stuff which won't be relevant after the cleanup. But more importantly everytime you're doing such small changes right here you're notifying subscribers to this PR (so I can get a notification, go check things out, and then I'd realize that you're in the middle of resolving issues so it's not worth looking at the current state of the PR yet). Also you're retrigerring CI checks with each such little change. So because of all of these I'd say it's better to do all the changes locally on your end, and then push it all at once in here.
Anyway, what to do? You can make the suggested changes locally (like copy paste), commit it, rebase to fixup all subsequent commits into the first commit of yours, and finally force push you local branch with a single commit to the origin (so it will update in here). More detailed how-to: The interactive rebase (I recommend reading the whole page, not just the linked section). In case of some doubts you can ask for help on the Contributors Chat.

@nonunknown nonunknown closed this Oct 17, 2022
@akien-mga akien-mga added archived and removed cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Nov 30, 2022
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