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

SpriteFrames Editor: Fix FPS applied to two animations when switching animation #79692

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Jul 20, 2023

Fix Animation FPS change applied to two animations when switching animation while editing spinbox FPS value.

Minor changes:

  1. Use *_no_signal() methods in _update_library() for FPS and Loop controls.
  2. Reorder some lines in the constructor.

@adamscott
Copy link
Member

Animation/assets-pipeline meeting: can you split up your PR, each PR fixing one bug (if possible?)? Thanks!

@dalexeev dalexeev force-pushed the sprite-frames-editor branch from 0118889 to 2f1e135 Compare July 25, 2023 07:56
@dalexeev dalexeev changed the title Editor: Fix some bugs with SpriteFrames editor SpriteFrames Editor: Fix FPS applied to two animations when switching animation Jul 25, 2023
@dalexeev
Copy link
Member Author

can you split up your PR, each PR fixing one bug (if possible?)?

@adamscott Done!

Comment on lines +1236 to +1238
autoplay->set_pressed_no_signal(false);
} else {
autoplay->set_pressed(String(edited_anim) == autoplay_name);
autoplay->set_pressed_no_signal(String(edited_anim) == autoplay_name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just leaving a note that this doesn't seem to result in any change in behavior, because the connected method _autoplay_pressed doesn't run if updating is true. So this should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

And the same applies to anim_speed and anim_loop.

@@ -1705,7 +1698,8 @@ SpriteFramesEditor::SpriteFramesEditor() {
sub_vb->add_child(animations);
animations->set_v_size_flags(SIZE_EXPAND_FILL);
animations->set_hide_root(true);
animations->connect("cell_selected", callable_mp(this, &SpriteFramesEditor::_animation_selected));
// HACK: The cell_selected signal is emitted before the FPS spinbox loses focus and applies the change.
animations->connect("cell_selected", callable_mp(this, &SpriteFramesEditor::_animation_selected), CONNECT_DEFERRED);
Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding the hack itself (and the same applies to #79872 really): we could probably avoid this if we store which item was selected when focus was gained, and on focus loss we update that item, and not the current item.

What do you think about that?

Copy link
Member Author

Choose a reason for hiding this comment

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

This will work, but I think it's about the same hack as this one, but also requires adding an unnecessary variable with duplicate content, a callback, and a connection to the LineEdit signal. I would prefer to keep it as is until #79695 is fixed or if we find any problems with this solution.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

I left a comment suggesting how to avoid relying on the order of operations, but I'm also approving this in case we are okay as is, because the fix works from my testing (and other changes seem good).

@akien-mga akien-mga merged commit 3cf1bc0 into godotengine:master Oct 5, 2023
@akien-mga
Copy link
Member

Thanks!

@dalexeev dalexeev deleted the sprite-frames-editor branch October 5, 2023 22:47
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.

Unexpected behavior on SpriteFrames animation FPS config
4 participants