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

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

Closed
eh-jogos opened this issue Jun 6, 2022 · 23 comments · Fixed by #61885
Closed
Assignees
Milestone

Comments

@eh-jogos
Copy link

eh-jogos commented Jun 6, 2022

Godot version

4.0.alpha9

System information

Manjaro Linux, Nvidia proprietary drivers, Vulkan Clustered

Issue description

When controlling animations through an AnimationTree, and there is a method call track with a keyframe on the last frame of the animation, that method call becomes unreliable, working only rarely, and not being called most of the time. The same animation when called through AnimationPlayer, always calls the method correctly.

On further inspection, the error might be because of floating point imprecision, as when I hover the keyframe it shows it's not exactly at 0.4 even though snap is turned on:
image

This seems to be happening with other frames except multiples of 5, so 0.0 is fine, 0.5 as well, 1.0 too and so on, but any frame values in between are imprecise.

To solve my issue for now I'm just going into the details and manually puting it at 0.39
image
image

Not super ideal, as I was already using this as a fix for AnimationPlayer signals being ignored when using AnimationTree (#28311)

--- EDIT ---
Floating point inaccuracy might not be related to the problem: #61766 (comment)

Steps to reproduce

Create any animation that ends on anything other than a multiple of 5 and add a method call to the last frame
Then call this animation through an AnimationTree StateMachine.
Calling the function directly through Animation Player seems to work fine

Minimal reproduction project

floating_point_last_frame.zip
In this project there is a simple scene with three simple animations and one of them calls a function on the last frame that prints a message and emits a signal.

image

By executing the project and pressing "ui_up" you can play the animation though the AnimationPlayer, "ui_right" through and AnimationTree using BlendTree as root, and "ui_down" through an AnimationTree using AnimationStateMachine as root. Only when playing through AnimationPlayer the method at the last frame is called consistently.

Changing the method call keyframe position from 0.4 to 0.39 fix it for both AnimationTrees.

@Rindbee
Copy link
Contributor

Rindbee commented Jun 8, 2022

Maybe we can mark the callback function called_at_the_end according to the time, or record it in a list when editing, to avoid problems caused by inaccurate floating point numbers.

@eh-jogos
Copy link
Author

eh-jogos commented Jun 8, 2022

Hum, problem might not be related floating point inaccuracy after all, as one thing I had not though about testing when I was doing the MRP was to try it with an animation that actually ends on 0.5.

In this case hovering the keyframe on the editor shows no floating point inaccuracy, but if I try using the AnimationTree the method on the last keyframe still doesn't get called.
image

@eh-jogos eh-jogos changed the title Floating point innacuracy prevents method track to executing method at the last frame of animation Methods at the last frame of an animation does not gets called when executed by AnimationTree Jun 8, 2022
@TokageItLab
Copy link
Member

AFAIK, AnimationPlayer, AnimationTree and AnimationStateMachine each have a different playback time.

Is the problem only with the AnimationStateMachine? Does the problem occur if I playback it in the AnimationTree without using the AnimationStateMachine?

@eh-jogos
Copy link
Author

eh-jogos commented Jun 8, 2022

AFAIK, AnimationPlayer, AnimationTree and AnimationStateMachine each have a different playback time.

Is the problem only with the AnimationStateMachine? Does the problem occur if I playback it in the AnimationTree without using the AnimationStateMachine?

@TokageItLab I don't know, I've only tested it with AnimationPlayer and AnimationStateMachine. I'll try to test it with AnimationTree and update the MRP, brb.

@eh-jogos
Copy link
Author

eh-jogos commented Jun 8, 2022

AFAIK, AnimationPlayer, AnimationTree and AnimationStateMachine each have a different playback time.

Is the problem only with the AnimationStateMachine? Does the problem occur if I playback it in the AnimationTree without using the AnimationStateMachine?

@TokageItLab Just confirmed it happens as well for AnimationTree using AnimtionBlendTree, or using AnimationNodeAnimation as root. Didn't test with BlendSpaces though as the other two were easier, would that be needed?

Also updated the issue description and the mrp zip.

@TokageItLab
Copy link
Member

TokageItLab commented Jun 8, 2022

Thanks! I remember there was a code somewhere in the core that added the minimum value of double (=CMP_EPSILON) when it reaches the end of the timeline to get the key for the last frame. Perhaps I think the problem is in that area, but I do not know without looking into it in the depth.

@Rindbee
Copy link
Contributor

Rindbee commented Jun 8, 2022

case Animation::TYPE_METHOD: {
if (blend < CMP_EPSILON) {
continue; //nothing to blend
}
if (!seeked && Math::is_zero_approx(delta)) {
continue;
}
TrackCacheMethod *t = static_cast<TrackCacheMethod *>(track);
List<int> indices;
a->method_track_get_key_indices(i, time, delta, &indices, pingponged);

If the delta is small enough, keyframes may be lost here.

I added some print code around this code.
captrue

Using delta<=0 instead of Math::is_zero_approx(delta), I think it will solve this problem. I'm not sure if other problems will be introduced.

captrue

Playback mode may be backward, pingpong, so, simply replacing with delta<=0 does not solve the problem very well.

@TokageItLab
Copy link
Member

Repost from #61836:
If delta is small enough, the animation should be stopped. The problem that needs to be fixed is not the comparison, but the way keys are gotten at the end of the animation.

@Rindbee
Copy link
Contributor

Rindbee commented Jun 9, 2022

Repost from #61836: If delta is small enough, the animation should be stopped. The problem that needs to be fixed is not the comparison, but the way keys are gotten at the end of the animation.

Another way of thinking, if delta is 0, it means the end, then we can get the exact length, so that there will be no loss.
The rest is to determine, when to decide that a small enough delta is 0.

Longer is acceptable, shorter may lose keyframes.

@akien-mga
Copy link
Member

I guess when the delay is near zero, it could snap to the end and play the last frame?

@Rindbee
Copy link
Contributor

Rindbee commented Jun 9, 2022

I guess when the delay is near zero, it could snap to the end and play the last frame?

When the delta is small enough for the first time, it is only marked and not processed. Let it be 0 directly the second time. One more step will be ok.

@TokageItLab
Copy link
Member

TokageItLab commented Jun 9, 2022

I looked into AnimationTree. Originally, #61836 state seems to address the fact that it fires continuously when the TimeScale is 0 (Conversely, AudioTrack and AnimationTrack, where that process does not exist, may have a continuous firing problem..). And I think that we need to think of a special case for the end of animation.

@Rindbee
Copy link
Contributor

Rindbee commented Jun 9, 2022

I think delta==0 as the animation-finished flag will make things clearer.

For begin-to-end, and end-to-begin, it's linear, so it's easy to compute.

For situations where jitter may occur, like pingpong or bouncy balls (The animation may end in the middle, nor is it an unimaginable situation, time is relative, like a number axis of position). Count the number of times if delta is small enough and continue, when the number is big enough, end it and force the delta to 0.

@TokageItLab
Copy link
Member

TokageItLab commented Jun 9, 2022

It is a rare case, but it cannot deal with the case where delta is 0 instead of a very small value at the end of the animation.

Rather than using delta==0 as a substitute for the animation end flag, I recommend managing the flag as end=true in end of timeline and process keyframe only once after ending animation.

@Rindbee
Copy link
Contributor

Rindbee commented Jun 9, 2022

delta should be calculated on a case-by-case basis, and this is not a good place to calculate it, the delta is related to AnimationState. The place where AnimationState is calculated should be appropriate, and the processing of the delta should be performed there.

The following is the processing logic for delta when delta is small enough.

...
// outside the loop or the integral function
int times = 0;
bool end = false;
...
// inside the loop or the integral function
if(end and other_condition){
	delta = 0;
	break;
}

if(Math::is_zero_approx(delta)){
	times++;
	end = times > MAX_ALLOW;
}else{
	times = 0; // reset as need.
}
...

@TokageItLab
Copy link
Member

TokageItLab commented Jun 9, 2022

The end of animation should be determined by current_time, loop_mode, backward, and animation->length. So setting delta = 0 in that case is fine, but the end flag should be managed at the same time, and using delta == 0 to determine the end of the animation is a bad implementation.

@Rindbee
Copy link
Contributor

Rindbee commented Jun 9, 2022

The end of animation should be determined by current_time, loop_mode, backward, and animation->length.

Couldn’t agree more. So wherever you can get this information, that's a good place for computation.

My idea is just to do further processing on the result after the calculation. According to the 4 parameters you mentioned.

In the existing implementations, delta==0 is also an end flag, but not the only one.

The essence of this issue is that the value field (the delta as value) and the flag field (the delta as end flag) intersect.

@TokageItLab
Copy link
Member

TokageItLab commented Jun 9, 2022

I found a related bug #61853 during testing this. I feel that we need a special case for the processing of the frames at both start and end of animation by any means. I will try to find a consistent solution.

@TokageItLab
Copy link
Member

TokageItLab commented Jun 10, 2022

Will be fixed by #61885. In the past, delta == 0 was used as a seeked flag. Sorry but I had forgotten about that.

@eh-jogos
Copy link
Author

eh-jogos commented Aug 19, 2022

@TokageItLab
I'm actually still having problems with this. For a while it seemed to have fixed, or became kind of hit or miss, some animations work perfectly and some just refuse to call the method.

Haven't found a predictive way to know when one animation will give me trouble or not (and avoid it), I'm not sure if the actual animation length influences this or not, but just realized now my AnimationTree has some BlendSpace1D nodes which it didn't before, so I have to check if the problem happens on the animations they use or not.

but the "workaround" I used before still works, which is to either turn off snap and move the method keyframe a few milliseconds back, or increase the length of the animation to be a few milliseconds longer, so it seems like the same problem to me?

Edit: I'm also using the AnimationPlayer in FPS mode rather than seconds (except for when I need to change to seconds to do the workaround), so I don't know if this influences anything

@eh-jogos
Copy link
Author

Yeah, just tested the same animation I'm having trouble with on BlendSpace1D in a simple AnimationNodeAnimation and had the same problem, will see if I can separate only the character skin in a MRP later

@eh-jogos
Copy link
Author

Seems to be related to animation length.
For the same animation, at 24 FPS:

  • when its length is 6 frames, a method at frame 6 never gets called.
  • when its length is 4 frames, a method at frame 4 always gets called.

@TokageItLab
Copy link
Member

Please post a new issue along with the reproduction project. If it is due to the precision of the FPS mode, I have a feeling that using floor instead of round for the internal decimal rounding process may solve the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment