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

Improved way of getting MethodTrack keys #61885

Merged
merged 1 commit into from
Jun 23, 2022

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Jun 10, 2022

Fixed #61766. Fixed partially #61853.
Supersede #61836.

MethodTrack kept the old checking algorithm. In the past, delta == 0 was used as a flag for having done a seek, but this is not necessary since seeked can be used. So remove it and this will be fixed #61766.

Instead, MethodTrack should get a key to not consider delta when seeking, as the same way with AudioTrack.

For example in ValueTrack:

if (seeked) {
	int idx = a->track_find_key(i, time); // Not considering delta.
	Variant value = a->track_get_key_value(i, idx);
	t->object->set_indexed(t->subpath, value);
} else {
	List<int> indices;
	a->value_track_get_key_indices(i, time, delta, &indices, pingponged); // Considering delta.
	for (int &F : indices) {
		Variant value = a->track_get_key_value(i, F);
		t->object->set_indexed(t->subpath, value);
	}
}

However, this fix prevents the behavior of "firing the end key at restart" in #61853, but it does not prevent the behavior of "firing the first key twice".

To be exact, it is fired in two frames, "the seek frame" and "the playbacked next frame"; if nothing is done in the seek frame, there could be a delay if there is user input. In the past, that delay was a problem with ValueTrack's Discrete mode, so I fixed it in #52518.

This double firing is not as much of a problem for AudioTrack and AnimationTrack, since they are checked for double insertion into the queue by HashSet::insert(). For ValueTrack, even if a value is set twice, it is not a problem unless there is a signal to subscribe to it. However with MethodTrack, it is enough of a problem.

Since MethodTrack calls are only fire in one frame, and it is impossible to determine how many frames will be processed by a call, so it is not possible to create a queue. At least at both ends of pingpong, it is possible to check for duplicates in one frame so it can be avoided, but it does not solve the problem in Seek.

At the moment, a compromise is to add a check such as if (is_processing) to the GDScript side of the MethodTrack call to prevent multiple processing.

@Rindbee
Copy link
Contributor

Rindbee commented Jun 10, 2022

In this method, it seems that the animation status check is placed in each track.

Are there inspection conditions (stop/end) that could be applied to each track? Could we avoid entering the loop that iterates over each track?

@TokageItLab
Copy link
Member Author

TokageItLab commented Jun 10, 2022

Since this is not the place to decide whether or not to stop the animation, the stopping condition is not relevant here, nor is it necessary to change that. What is needed now is to correct the incorrect seek method and remove unnecessary checks.

The animation itself is stopped by clamping at the end. At this time, it depends on track_get_key_indices() to get the end key. To allow track_get_key_indices() to get the end key, the animation is internally lengthened by CMP_EPSILON, but because of unnecessary checks, that added length was not working effectively.

So, what was actually needed was neither Math::is_zero_approx(delta) nor delta==0, but to remove that checks.

However, there is another problem when seeking, so it fixed partially that by matching the behavior with AudioTrack and AnimationTrack.

@Rindbee

This comment was marked as off-topic.

@TokageItLab

This comment was marked as off-topic.

@Rindbee

This comment was marked as off-topic.

@TokageItLab

This comment was marked as off-topic.

@reduz
Copy link
Member

reduz commented Jun 23, 2022

Seems a bit hacky but I guess its ok

@akien-mga akien-mga merged commit 1a4a485 into godotengine:master Jun 23, 2022
@akien-mga
Copy link
Member

Thanks!

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.

Methods at the last frame of an animation does not gets called when executed by AnimationTree
4 participants