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

Update AnimationPlayer in real-time when bezier curve properties or bezier editor changes #96753

Conversation

Dowsley
Copy link
Contributor

@Dowsley Dowsley commented Sep 9, 2024

I am unsure whether I should add the update to AnimationBezierTrackEdit::duplicate_selected_key as well.

Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

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

I would consider adding an update in _change_selected_keys_handle_mode too since right-clicking and changing the handle mode currently does not trigger an update of the property.

@SaracenOne
Copy link
Member

SaracenOne commented Sep 10, 2024

I would also say that it is probably worth adding the update to the duplicate operation for good measure.

@Dowsley Dowsley force-pushed the realtime-animation-player-bezier-property-changes branch from f8cc757 to 07bcede Compare September 10, 2024 15:57
@Dowsley
Copy link
Contributor Author

Dowsley commented Sep 10, 2024

I would consider adding an update in _change_selected_keys_handle_mode too since right-clicking and changing the handle mode currently does not trigger an update of the property.

I would also say that it is probably worth adding the update to the duplicate operation for good measure.

Updated with the required changes. I suppose the EditorUndoRedoManager is a queue, so the order of add calls matter. So I put the APE's update key frame adds above the queue_redraw adds.

@Dowsley Dowsley requested a review from SaracenOne September 10, 2024 15:59
Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

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

Actually, found two more instances where we would also want to trigger the update: cutting and pasting.

To fix pasting, I think you should change line 1923 from:

		undo_redo->commit_action();

		queue_redraw();

to:

		AnimationPlayerEditor *ape = AnimationPlayerEditor::get_singleton();
		if (ape) {
			undo_redo->add_do_method(ape, "_animation_update_key_frame");
			undo_redo->add_undo_method(ape, "_animation_update_key_frame");
		}
		undo_redo->add_do_method(this, "queue_redraw");
		undo_redo->add_undo_method(this, "queue_redraw");

		undo_redo->commit_action();

For cutting, add this below line 1845:

		AnimationPlayerEditor *ape = AnimationPlayerEditor::get_singleton();
		if (ape) {
			undo_redo->add_do_method(ape, "_animation_update_key_frame");
			undo_redo->add_undo_method(ape, "_animation_update_key_frame");
		}
		undo_redo->add_do_method(this, "queue_redraw");
		undo_redo->add_undo_method(this, "queue_redraw");

This should solve the remaining edge cases.

@Dowsley Dowsley force-pushed the realtime-animation-player-bezier-property-changes branch from 07bcede to ca25b7a Compare September 11, 2024 20:58
@Dowsley
Copy link
Contributor Author

Dowsley commented Sep 11, 2024

@SaracenOne good catch. Force pushed the changes.

@Dowsley Dowsley requested a review from SaracenOne September 11, 2024 20:58
Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Sep 12, 2024
@akien-mga akien-mga force-pushed the realtime-animation-player-bezier-property-changes branch from ca25b7a to 9d0944b Compare September 12, 2024 07:34
@akien-mga akien-mga merged commit a8904b9 into godotengine:master Sep 12, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@akien-mga akien-mga added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 12, 2024
@Zireael07
Copy link
Contributor

This was someone's first contribution? O_o

@Dowsley
Copy link
Contributor Author

Dowsley commented Sep 12, 2024

It was mostly copying and pasting 😆 thanks for the help folks

@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

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.

AnimationPlayer doesn't update in real-time when bezier curves change
5 participants