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

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

Merged
merged 1 commit into from
Oct 5, 2023
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 8 additions & 14 deletions editor/plugins/sprite_frames_editor_plugin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,13 +846,6 @@ void SpriteFramesEditor::_animation_selected() {
return;
}

if (frames->has_animation(edited_anim)) {
double value = anim_speed->get_line_edit()->get_text().to_float();
if (!Math::is_equal_approx(value, (double)frames->get_animation_speed(edited_anim))) {
_animation_speed_changed(value);
}
}

TreeItem *selected = animations->get_selected();
ERR_FAIL_COND(!selected);
edited_anim = selected->get_text(0);
Expand Down Expand Up @@ -1240,9 +1233,9 @@ void SpriteFramesEditor::_update_library(bool p_skip_selector) {
if (animated_sprite) {
String autoplay_name = animated_sprite->call("get_autoplay");
if (autoplay_name.is_empty()) {
autoplay->set_pressed(false);
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);
Comment on lines +1236 to +1238
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.

}
}

Expand Down Expand Up @@ -1298,8 +1291,8 @@ void SpriteFramesEditor::_update_library(bool p_skip_selector) {
}
}

anim_speed->set_value(frames->get_animation_speed(edited_anim));
anim_loop->set_pressed(frames->get_animation_loop(edited_anim));
anim_speed->set_value_no_signal(frames->get_animation_speed(edited_anim));
anim_loop->set_pressed_no_signal(frames->get_animation_loop(edited_anim));

updating = false;
}
Expand Down Expand Up @@ -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.

animations->connect("item_edited", callable_mp(this, &SpriteFramesEditor::_animation_name_edited));
animations->set_allow_reselect(true);

Expand Down Expand Up @@ -1834,6 +1828,7 @@ SpriteFramesEditor::SpriteFramesEditor() {
frame_duration->set_custom_arrow_step(0.1);
frame_duration->set_allow_lesser(false);
frame_duration->set_allow_greater(true);
frame_duration->connect("value_changed", callable_mp(this, &SpriteFramesEditor::_frame_duration_changed));
hbc_frame_duration->add_child(frame_duration);

// Wide empty separation control. (like BoxContainer::add_spacer())
Expand Down Expand Up @@ -1864,6 +1859,7 @@ SpriteFramesEditor::SpriteFramesEditor() {
hbc_zoom->add_child(zoom_in);

file = memnew(EditorFileDialog);
file->connect("files_selected", callable_mp(this, &SpriteFramesEditor::_file_load_request).bind(-1));
add_child(file);

frame_list = memnew(ItemList);
Expand Down Expand Up @@ -1919,8 +1915,6 @@ SpriteFramesEditor::SpriteFramesEditor() {
zoom_in->set_shortcut(ED_SHORTCUT_ARRAY("sprite_frames/zoom_in", TTR("Zoom In"),
{ int32_t(KeyModifierMask::CMD_OR_CTRL | Key::EQUAL), int32_t(KeyModifierMask::CMD_OR_CTRL | Key::KP_ADD) }));

file->connect("files_selected", callable_mp(this, &SpriteFramesEditor::_file_load_request).bind(-1));
frame_duration->connect("value_changed", callable_mp(this, &SpriteFramesEditor::_frame_duration_changed));
loading_scene = false;
sel = -1;

Expand Down