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 inconsistent behavior of Bezier editor undo operations upon selection of different animation #93860

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CookieBadger
Copy link
Contributor

@CookieBadger CookieBadger commented Jul 2, 2024

Fixes #93859.

This PR removes the method _bezier_track_insert_key in the AnimationBezierEditor and adds a method bezier_track_insert_key_with_handle_mode in Animation. This new method is then called in the undo/redo operations, such that the correct animation is always tracked.

This PR adds the animation as a parameter to a method _bezier_track_insert_key_at_anim (before _bezier_track_insert_key) in the AnimationBezierEditor, such that it is tracked when used in UndoRedo operations.

@CookieBadger CookieBadger force-pushed the animation-bezier-undo-on-different-animation-fix branch from 8c473ac to 163d218 Compare July 2, 2024 11:34
@@ -455,6 +455,7 @@ class Animation : public Resource {
void track_set_interpolation_type(int p_track, InterpolationType p_interp);
InterpolationType track_get_interpolation_type(int p_track) const;

void bezier_track_insert_key_with_handle_mode(int p_track, double p_time, real_t p_value, const Vector2 &p_in_handle, const Vector2 &p_out_handle, HandleMode p_handle_mode);
Copy link
Member

Choose a reason for hiding this comment

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

HandleMode is TOOLS_ENABLED (editor) only, so this doesn't compile in export templates.

If this is only needed for the editor, it should also be enclosed in #ifdef TOOLS_ENABLED, but then it can't be exposed in _bind_methods as the scripting API shouldn't differ between tools and non-tools.

Copy link
Contributor Author

@CookieBadger CookieBadger Jul 2, 2024

Choose a reason for hiding this comment

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

oh I see. Well, it should not be exposed in the scripting API, but I figured it would have to be bound, such that it can be called from a signal, right?

Copy link
Member

Choose a reason for hiding this comment

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

To use as a signal callback it doesn't need to be bound, you can use callable_mp.
But in this PR you're using it for UndoRedo and that one currently still requires method bindings, it doesn't support callable_mp.

So ideally if there's a way to achieve what you're doing while keeping the method within the AnimationBezierEditor, that might be best. Otherwise it might need some creative API rethinking (possibly exposing HandleMode to non-tools if there's no good alternative).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, makes sense. The easiest is to move it back to the bezier editor then. Thank you, akien! :)

@CookieBadger CookieBadger force-pushed the animation-bezier-undo-on-different-animation-fix branch from 163d218 to af26e7b Compare July 2, 2024 12:25
@fire fire requested a review from a team July 2, 2024 23:01
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.

Inconsistent behavior when undoing animation bezier movements upon selection of different animations
3 participants