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

Fixed behavior of insert track queue in AnimationTrackEditor is unstable #51459

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Aug 10, 2021

Fixed #51415.

The queue for insert track in AnimationTrackEditor gets the drawing frame of the Engine and determines if the operation was done on the same frame. Why do we implement it this way? This implementation seems to be special compared to other queues.

There does not seem to be any code anywhere that uses the insert track queue, but when we actually try to use it, it does not work well in 4.0 (at least it was working well in 3.3) where the renderer has changed significantly.

I made it so that the queue creation and commit is done manually.

@TokageItLab TokageItLab changed the title fix AnimationTrackEditor's insert track queue doesn't work correctly Fixed AnimationTrackEditor's insert track queue doesn't work correctly Aug 10, 2021
@TokageItLab TokageItLab changed the title Fixed AnimationTrackEditor's insert track queue doesn't work correctly Fixed insert track queue in AnimationTrackEditor doesn't work correctly Aug 10, 2021
@TokageItLab TokageItLab marked this pull request as draft August 10, 2021 03:56
@TokageItLab
Copy link
Member Author

TokageItLab commented Aug 10, 2021

I found a problem with undo in BezierTrack and am working on fixing it.

@TokageItLab TokageItLab force-pushed the fix-animation-track-editor-insert-queue branch 2 times, most recently from 5ac8b76 to 60e8900 Compare August 10, 2021 04:45
@TokageItLab TokageItLab marked this pull request as ready for review August 10, 2021 04:45
@TokageItLab
Copy link
Member Author

TokageItLab commented Aug 10, 2021

The undo problem has been fixed. I assume that this queue was only for BezierTrack, and if we try to use it from outside of AnimationTrackEditor, it works fine in 3.3 and earlier, but not in 4.0. Anyway, I think this is not a good way to implement queue, and it is actually causing problems in the implementation of #45699, so I fixed it.

@TokageItLab TokageItLab force-pushed the fix-animation-track-editor-insert-queue branch from 60e8900 to 2c48c14 Compare August 10, 2021 04:53
@TokageItLab TokageItLab changed the title Fixed insert track queue in AnimationTrackEditor doesn't work correctly Fixed behavior insert track queue in AnimationTrackEditor is unstable Aug 10, 2021
@TokageItLab TokageItLab changed the title Fixed behavior insert track queue in AnimationTrackEditor is unstable Fixed behavior of insert track queue in AnimationTrackEditor is unstable Aug 10, 2021
@akien-mga akien-mga added this to the 4.0 milestone Aug 10, 2021
@TokageItLab TokageItLab force-pushed the fix-animation-track-editor-insert-queue branch 3 times, most recently from 48df4a9 to ac0573f Compare August 10, 2021 23:25
@TokageItLab
Copy link
Member Author

I fixed the part that was reviewed.

@TokageItLab TokageItLab force-pushed the fix-animation-track-editor-insert-queue branch from ac0573f to a89746a Compare August 11, 2021 21:17
@TokageItLab TokageItLab force-pushed the fix-animation-track-editor-insert-queue branch from a89746a to 50730a1 Compare August 12, 2021 22:57
@TokageItLab
Copy link
Member Author

TokageItLab commented Aug 15, 2021

@JFonS As I mentioned in #45699, if this implementation is not done, inserting bone animation will not work properly and the implementation of SkeletonGizmo is halted, so if you can, it would be helpful that you review and approve this PR.
Alternatively, it's one way to merge this PR into SkeletonGizmo without separating as this.

CC @fire

@JFonS
Copy link
Contributor

JFonS commented Aug 16, 2021

I don't know much about the animation track system, so I can't properly review this PR, sorry.

@fire fire requested a review from a team August 22, 2021 16:07
@TokageItLab
Copy link
Member Author

TokageItLab commented Aug 22, 2021

@fire Here is video demo.

This is the behavior in the current master.

2021-08-23.1.32.41-1.mov

If I do track_insert_key() from within SkeletonEditorPlugin with the number of bones, only one track is inserted. Also, you can see that the timing of the popup warning is wrong.

Thus, for example, I provided one frame of grace for the judging.

void AnimationTrackEditor::_query_insert(const InsertData &p_id) {
	if (insert_frame != Engine::get_singleton()->get_frames_drawn()) {
		//clear insert list for the frame if frame changed
		if (insert_confirm->is_visible()) {
			return; //do nothing
		}
		insert_data.clear();
		insert_query = false;
	}
	insert_frame = Engine::get_singleton()->get_frames_drawn() + 1; // <- one frame of grace
2021-08-23.1.35.53-1.mov

The result is that two tracks are now inserted at the same time. But, the timing of the popup warning is still wrong.

After this PR makes elimination of judging by drawing frame and setting the queue manually, all tracks will be inserted correctly at the same time with the same code except for setting the queue manually.

2021-08-23.1.43.22-1.mov

@TokageItLab TokageItLab force-pushed the fix-animation-track-editor-insert-queue branch from 50730a1 to 9657a46 Compare August 22, 2021 17:19
@fire fire requested a review from a team August 25, 2021 04:43
@TokageItLab TokageItLab force-pushed the fix-animation-track-editor-insert-queue branch from 9657a46 to 7a44702 Compare October 6, 2021 22:30
@TokageItLab TokageItLab requested a review from a team as a code owner October 6, 2021 22:30
@TokageItLab
Copy link
Member Author

TokageItLab commented Oct 6, 2021

#45699 has been merged, so I updated it. Currently multiple animation tracks insertion by SkeletonEditor is broken, but it will be fixed when this PR is merged.

@akien-mga akien-mga merged commit 90fd6e9 into godotengine:master Oct 7, 2021
@akien-mga
Copy link
Member

Thanks!

@TokageItLab TokageItLab deleted the fix-animation-track-editor-insert-queue branch May 23, 2022 15:19
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.

Inserting multiple tracks in AnimationTrackEditor is broken
4 participants