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 methods may not be called at the end of the animation. #61836

Closed

Conversation

Rindbee
Copy link
Contributor

@Rindbee Rindbee commented Jun 9, 2022

Previously, the delta could be too small, preventing the methods at the end of the animation from being called.

Fix #61766.
Fix #61853.

I think it is necessary to explain what I think and why.

A small enough value was considered that the animation should be stopped.

I partially agree. The difference is that occasional small deltas (including 0) can actually be ignored, i.e., treated as normal deltas. Only consecutive small deltas are considered to be the end of the animation, that is, let go of the first small delta, and if the next delta is still small enough, the animation is stopped or finished.

If the animation is stopped or finished, delta==0 holds.

This is the conclusion from cause to effect. So, it should be uncontroversial.

The question is whether the cause can be inferred from the result, i.e. if delta==0, the animation has ended/stopped. It doesn't actually hold, but if we use the word continuous to limit, that is, if delta==0 appears continuously, the animation has ended/stopped, it holds, because 0 is also a small enough value.

Simply put, my job is to remap the range of deltas. It is only valid for consecutive small values (from the second one).

It is common and normal to have a small enough value before ending or stopping. But the current implementation treats it as an end flag and discards it, this is what caused #61766.

I put 0 as the stop/finish flag because 0 is special enough:

  1. It's a constant. Fixed values are easier to predict and judge than uncertain values.
  2. It has no effect on integrals. Although in some cases we would like to have an impact, such as in-place operations. But those cases are only extreme cases, and we can construct compound conditions with the help of other parameters.
  3. It is also a small enough value. Math::is_zero_approx(0) is true, so it should be compatible with existing code, unless Math::is_zero_approx(delta) && delta != 0.

@Rindbee Rindbee requested a review from a team as a code owner June 9, 2022 02:20
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

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 Author

Rindbee commented Jun 9, 2022

This should make the non-zero delta available to the integral function again. The delta in the loop seems unlikely to be a relatively small value.

Consecutive small values are considered to be the end of the animation, and occasional small values are considered to be values as normal.

@TokageItLab
Copy link
Member

TokageItLab commented Jun 9, 2022

I say again, it is a bad idea that using delta to assume end of the animation. When TimeScale is 0, there is a case where small deltas are generated continuously.

@Rindbee
Copy link
Contributor Author

Rindbee commented Jun 9, 2022

Although I'm not very familiar with CPP, isn't 0 * delta equal to 0?

When TimeScale is 0, there is case where small deltas are generated continuously.

Isn't this a bug of TimeScale? Or implement restrictions?

Peek 2022-06-07 18-57

I did some testing on my code and didn't feel like it made the situation worse

@TokageItLab
Copy link
Member

TokageItLab commented Jun 9, 2022

There is no bug in TimeScale.

Consider the case where the playback position stays in the same frame by TimeScale; In the case of directly setting a value, such as value track, it does not matter how many times it is set. However, in the case of firing events such as method track, it is wrong to fire it continuously but the animation itself is still in progress and has not ended. The current implementation prevents it. There is probably no need to change that.

I simply say that delta should not be used as an end-of-animation flag/checking.

@Rindbee Rindbee force-pushed the fix-methods-not-called-at-the-end branch from d223652 to 627fb0b Compare June 9, 2022 22:35
@Rindbee
Copy link
Contributor Author

Rindbee commented Jun 9, 2022

Reset the internal time and step while reseek in AnimationNodeAnimation::process.

Previously, the delta could be too small, preventing the methods at the end of
the animation from being called.
@Rindbee Rindbee force-pushed the fix-methods-not-called-at-the-end branch from 627fb0b to 2d092c0 Compare June 10, 2022 11:28
@Rindbee
Copy link
Contributor Author

Rindbee commented Jun 11, 2022

Superseded by #61885.

@Rindbee Rindbee closed this Jun 11, 2022
@Rindbee Rindbee deleted the fix-methods-not-called-at-the-end branch June 11, 2022 07:03
@akien-mga akien-mga added this to the 4.0 milestone Jun 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants