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

Fix rename animation in SpriteFramesEditor #79600

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented Jul 18, 2023

When the name suffix grows, the old name is used if it is obtained first.

Fix the case where the following error message would appear when renaming an animation.

ERROR: Animation '' doesn't exist.
   at: get_frame_count (scene/resources/sprite_frames.cpp:71)

Fix #73883.

counter++;
name = new_name + "_" + itos(counter);
}

EditorUndoRedoManager *undo_redo = EditorUndoRedoManager::get_singleton();
undo_redo->create_action(TTR("Rename Animation"), UndoRedo::MERGE_DISABLE, EditorNode::get_singleton()->get_edited_scene());
_rename_node_animation(undo_redo, false, edited_anim, "", "");
Copy link
Member

Choose a reason for hiding this comment

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

To be honest, I don't really remember what this initialization was implemented for. The _rename_node_animation() is a method to change the animation name kept in other properties of AnimatedSprite, but without it, it is possible to have some inconsistencies in some properties, so it should be tested carefully.

Copy link
Contributor Author

@Rindbee Rindbee Jul 18, 2023

Choose a reason for hiding this comment

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

The previous code will rename name to a non-empty value. Even though it would have side effects, I feel like it could be done in the call on line 961.

_rename_node_animation(undo_redo, false, edited_anim, "", "");
undo_redo->add_do_method(frames.ptr(), "rename_animation", edited_anim, name);
undo_redo->add_undo_method(frames.ptr(), "rename_animation", name, edited_anim);
_rename_node_animation(undo_redo, false, edited_anim, name, name);
_rename_node_animation(undo_redo, true, edited_anim, edited_anim, edited_anim);

void SpriteFramesEditor::_rename_node_animation(EditorUndoRedoManager *undo_redo, bool is_undo, const String &p_filter, const String &p_new_animation, const String &p_new_autoplay) {
List<Node *> nodes;
_find_anim_sprites(EditorNode::get_singleton()->get_edited_scene(), &nodes, Ref<SpriteFrames>(frames));

@Calinou Calinou added this to the 4.x milestone Jul 18, 2023
@Calinou Calinou added cherrypick:4.0 cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Jul 18, 2023
…ditor

When the name suffix grows, the old name is used if it is obtained first.

Fix the case where the following error message would appear when renaming
an animation.

```
ERROR: Animation '' doesn't exist.
   at: get_frame_count (scene/resources/sprite_frames.cpp:71)
```
@Rindbee Rindbee force-pushed the fix-rename-animation-in-SpriteFramesEditor branch from 2f7fe6c to e9cd29c Compare July 18, 2023 07:40
@Rindbee Rindbee requested a review from TokageItLab July 18, 2023 07:48
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

I don't know what affect deleting the _rename_node_animation(undo_redo, false, edited_anim, "", ""); would have. Perhaps it was there to prevent some error when trying to use a space in the name, but rather it seems to be triggering an error and should be removed. If this removing causes other errors, that should be resolved in another way.

Well, I tested this and it seems to work fine, so I believe this can be merged.

@TokageItLab TokageItLab modified the milestones: 4.x, 4.2 Jul 18, 2023
@YuriSizov YuriSizov merged commit 7573a45 into godotengine:master Jul 21, 2023
@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 21, 2023

Thanks! Don't think we should cherry-pick it into patch releases if we aren't sure what the removal may affect. But we can consider it for 4.1 after some time, if there are no issues reported.

@Rindbee Rindbee deleted the fix-rename-animation-in-SpriteFramesEditor branch July 21, 2023 22:19
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 23, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants