AnimationPlayer.seek does not work as documented (or logical at all) with call method tracks

reimgrab commented 4 months ago

Tested versions

Reproducible in 4.2.2.stable not Reproducible in 4.1.4.stable and 3.x

System information

Godot v4.2.2.stable.mono - Ubuntu 22.04.4 LTS 22.04 - X11 - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 1050 Ti (nvidia; 535.171.04) - Intel(R) Core(TM) i5-6600 CPU @ 3.30GHz (4 Threads)

Issue description

AnimationPlayer seeking does not work as described in the documentation in regards to functions called via call method tracks (I have not tested other tracks). Especially "Events between the current frame and seconds" are inconsistently not "skipped". Seeking is done via AnimationPlayer.play() directly followed by AnimationPlayer.seek(). Methods at time 0 are always called when they never should. Methods in between 0 and the 'seeked seconds' are skipped or not depending on the animation. And btw if events are skipped anyways (per documentation), what does 'update_only' actually do?

Steps to reproduce

It is easier to just look at the MRP than explaining it here.

Minimal reproduction project (MRP)

4.2 version: AnimPlayerBug_v4.2.zip Correctly working 4.1 version: AnimPlayer_v4.1.zip

TokageItLab commented 4 months ago

I think it was a bug that the behind method key was fired. It should have been fixed by https://github.com/godotengine/godot/pull/85569, please check 4.3.beta1.

Also if it is seeked, the event is not always skipped, as it depends on the update_only flag whether the events in between are skipped or not. So I assume it is a documentation issue.

reimgrab commented 4 months ago

Not fixed in v4.3.beta.custom_build [e96ad5af9]. Methods at time 0 are still called always. The 'update' parameter seems to be inversed. Methods that should be skipped may not be skipped ('update_only' = true) and methods that should not be skipped may be skipped ('update_only' = false). MRP taking into account that events should not be skipped for update_only = false: AnimPlayerBug_v4.2.zip

TokageItLab commented 4 months ago

I have looked into the project.

Start is always fired because play() process the key you are currently on. It should not be fired by seek(), but called by play(), so always called Start is intended behavior
The update option is the difference between processing the frame after the seek on the fly or on the next process
The update_only option is to process the frame just after seek, but not to call the method track if it is there

Taken together, the demo project is wrong in the expected statement and I do not see the problem about the flags. However, the documentation certainly seems to need proofreading as it may cause misunderstandings.

Also, if I look at the code, it does not fire all events during seek, but only the events of the previous key. This change was made a long time ago, and I don't remember where, but I think there was some kind of problem at the time. It might make sense to fix it, but it would require some research.

reimgrab commented 4 months ago

Regarding play(), that is a change in behavior compared to before 4.2 and the one which broke my real world project and why I started investigating at all. See the provided 4.1 project. So now we must set assigned_animation , then seek(), then play() to get the previous behavior, correct? Thinking about this, it is much more logical anyway and what I should have done from the start but the other way around worked just fine.

I certainly agree that the documentation is in dire need of an update but even after your explanations I am still a little confused. If you take other tracks into account it looks like

Start is always fired because play() process the key you are currently on

is only true for call method tracks and not property tracks. I added a property track that 'animates' a string value to the MRP (the print statement for the string is in the process function and only executed when the string changes and the keys are the same as for the call method track):

Anim 1: no seek()
String: Start at 0
Method: Start at 0
Method: 1 at 0.50942133333333
String: 1 at 0.50942133333333
Method: 2 at 1.50942133333333
String: 2 at 1.50942133333333
----------------------------
Anim 1: play() -> seek(1.0, false, false)
Method: Start at 1
Method: 1 at 1.01666666666667
String: 1 at 1.01666666666667
Method: 2 at 1.5
String: 2 at 1.5

The method is called but the string is not changed (key at position 0) when seeking after playing. Method and String at position 0.5 are executed/changed because the events of the previous key are processed, correct?

But only if the AnimationPlayer is already playing:

Anim 1: seek(1.0, false, false) -> play()
Method: 2 at 1.5
String: 2 at 1.5

I think I can follow your explanation there.

Anim 1: seek(1.0, true, false) -> play()
Method: 1 at 1
Method: 2 at 1.5
String: 2 at 1.5

But why is the method called but the string not changed here? Since there is no key at position 1.0 it looks like update=true forces the 'process previous key behavior' but only for the call method and not the property track.

If we look at the second animation (which has a key on position 1.0) there is also something I don't quite get:

Anim 2: seek(1.0, false, false) -> play()
String: 2 at 1
Method: 3 at 1.5
String: 3 at 1.5

Here only the property track is processed.

Anim 2: play() -> seek(1.0, true, false)
Method: Start at 1
Method: 2 at 1
Method: 3 at 1.5
String: 3 at 1.5

Here only the call method track is processed.

Anim 2: seek(1.0, true, false) -> play()
Method: 2 at 1
String: 2 at 1
Method: 3 at 1.5
String: 3 at 1.5

This works as expected.

The takeaway:

The documentation needs an update
I have a fix for my initial problem (at least I think so)
IMHO processing the previous event is a bug
Call method and other tracks should work consistently (if update_only = false) either both processed or none processed
I still don't know if I am just stupid or if there is something fishy going on, if the former the above is moot
I spent too much time looking into this, my brain hurts

DISCLAIMER: The updated MRP does not contain expectations as currently I do not have any ;) I double, triple, quadruple checked the MRP and my findings but there is always the possibility that I got something wrong. Hoping to be helpful one way or another and not just a nuisance

AnimPlayer_v4.2.zip

godotengine / godot