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 Frame Duration applied to wrong frame when switching frame #79872

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

dalexeev
Copy link
Member

Fix frame duration change applied to last frame when switching frame while editing value of Frame Duration spinbox.

Other changes:

  1. Fix the UndoRedo merging mode of the Frame Duration field to be consistent with the FPS field (I think it was my copy-paste mistake).
  2. Use *_no_signal() methods in _update_library() for Frame Duration control.

@@ -1876,8 +1874,8 @@ SpriteFramesEditor::SpriteFramesEditor() {
frame_list->set_max_text_lines(2);
SET_DRAG_FORWARDING_GCD(frame_list, SpriteFramesEditor);
frame_list->connect("gui_input", callable_mp(this, &SpriteFramesEditor::_frame_list_gui_input));
frame_list->connect("item_selected", callable_mp(this, &SpriteFramesEditor::_frame_list_item_selected));

// HACK: The item_selected signal is emitted before the Frame Duration spinbox loses focus and applies the change.
Copy link
Member

Choose a reason for hiding this comment

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

Oh no a hack.

Copy link
Member Author

Choose a reason for hiding this comment

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

To me, the deferred connection looks like a hack that could cause other issues. See #79695.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can avoid this if we track which frame was selected when focus was gain and update that frame on focus loss, not the one currently selected. See #79692 (comment).

@@ -1192,7 +1190,7 @@ void SpriteFramesEditor::_update_library(bool p_skip_selector) {

updating = true;

frame_duration->set_value(1.0); // Default.
frame_duration->set_value_no_signal(1.0); // Default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the same be done to SpriteFramesEditor::_frame_list_item_selected? Then you can probably remove the updating guards in it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then you can probably remove the updating guards in it.

Probably yes, but I wouldn't want to add any regressions. We'll probably refactor this plugin at some point and remove the obscurity in the code execution order.

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.

Same suggestion as in #79692, but pre-approving because this should still work as a fix (haven't tested, but it's the same as #79692).

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

Thanks!

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

4 participants