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

[3.x] AnimatedSprite Fix updating inspector when SpriteFrames is modified #49495

Merged

Conversation

kleonc
Copy link
Member

@kleonc kleonc commented Jun 10, 2021

(needs a separate master version due to #45879; for the same reason some issues fixed here might already be non-existent in the master; haven't checked none of these)

Makes creating / renaming / deleting animations in the SpriteFramesEditorPlugin properly update properties of the AnimatedSprite in the inspector (mainly options in the animation dropdown). Removing current animation of the AnimatedSprite will make the first (after removal) animation left be selected as the new current animation (if no animation would be left then nothing will be done from the inspector point of view).

Reverts #27254 but keeps #26381 being fixed.
Fixes #44438.
Fixes #46741.
Fixes #28658, fixes #40263 (these look like duplicates).

Removes unused SpriteFrames::_get_animation_list() method.

After reverting #27254 newly created animation is no longer automatically set as the current animation of the AnimatedSprite/AnimatedSprite3D being edited. Should the current behavior be preserved (assigning it automatically)?
I've changed it so currently the newly created animation is automatically set as the current animation of the anim sprite being edited (same as before), and the same for all other anim sprites having currently invalid animation set (same as before). But valid animations of other anim sprites will no longer be automatically changed (as they used to be).

@kleonc kleonc requested a review from a team as a code owner June 10, 2021 17:51
@Calinou Calinou added this to the 3.4 milestone Jun 10, 2021
@KoBeWi
Copy link
Member

KoBeWi commented Jul 7, 2021

I checked the issues. One is valid on both branches and one only in 3.x

newly created animation is no longer automatically set as the current animation of the AnimatedSprite/AnimatedSprite3D being edited.

I think the reason for this was that when you create new animation and put some sprites, you can preview it immediately.

@kleonc
Copy link
Member Author

kleonc commented Jul 7, 2021

I checked the issues. One is valid on both branches and one only in 3.x

Hmm, I think I've listed 3 issues (4 if counting the duplicated one):

Reverts #27254 but keeps #26381 being fixed.
Fixes #44438.
Fixes #46741.
Fixes #28658, fixes #40263 (these look like duplicates).

so it's kinda hard to tell what have you found out. 😄 So please try being more specific. Also thanks for checking it out!
Oh, and if by any chance you'd like to do master version feel free to do so (or anyone else).


newly created animation is no longer automatically set as the current animation of the AnimatedSprite/AnimatedSprite3D being edited.

I think the reason for this was that when you create new animation and put some sprites, you can preview it immediately.

For me it looks like it was just an unintended side effect of #27254. But yeah, I can see how it might be a desired behavior for some people and that's why I've asked which behavior is the desired one / should current behavior be preserved.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 7, 2021

so it's kinda hard to tell what have you found out.

I meant #28658 and #46741 (the other ones are closed now).

@akien-mga akien-mga added the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Jul 4, 2022
@kleonc kleonc force-pushed the sprite_frames_editor-updating-fixes-3x branch from 298bc38 to 8ae246f Compare July 13, 2022 13:51
@kleonc
Copy link
Member Author

kleonc commented Jul 13, 2022

@akien-mga Rebased, applied suggestion. Added some more comments and changed this (part of the PR description):

After reverting #27254 newly created animation is no longer automatically set as the current animation of the AnimatedSprite/AnimatedSprite3D being edited. Should the current behavior be preserved (assigning it automatically)?
I've changed it so currently the newly created animation is automatically set as the current animation of the anim sprite being edited (same as before), and the same for all other anim sprites having currently invalid animation set (same as before). But valid animations of other anim sprites will no longer be automatically changed (as they used to be).

@akien-mga akien-mga modified the milestones: 3.x, 3.6 Jul 13, 2022
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Should be good to merge after 3.5 is released.

@akien-mga akien-mga merged commit 173a803 into godotengine:3.x Aug 5, 2022
@akien-mga
Copy link
Member

Thanks!

@kleonc kleonc deleted the sprite_frames_editor-updating-fixes-3x branch August 6, 2022 08:24
@akien-mga
Copy link
Member

Cherry-picked for 3.5.1.

@akien-mga akien-mga removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Aug 8, 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

4 participants