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

get_current_play_position() always returns 0 for all looping animations #82581

Closed
vaner-org opened this issue Sep 30, 2023 · 12 comments · Fixed by #87171
Closed

get_current_play_position() always returns 0 for all looping animations #82581

vaner-org opened this issue Sep 30, 2023 · 12 comments · Fixed by #87171

Comments

@vaner-org
Copy link

vaner-org commented Sep 30, 2023

Godot version

Godot v4.1.1.stable

System information

Windows 10.0.19044 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3080 (NVIDIA; 31.0.15.3713) - AMD Ryzen 9 5950X 16-Core Processor (32 Threads)

Issue description

When $AnimationTree["parameters/playback"].get_current_play_position() is called during playback of any animation that is set to loop, it always returns 0. This makes it impossible to track animation progress or intercept animations mid-cycle through code.

I have a complex project that has a looping walk cycles that require continuous step tracking, so animations can be queued against the player's left or right foot. To do this, I need to call get_current_play_position() every frame to know when the legs switch over. In 3.5, this function works as expected, and I cannot port my project over to 4 between this and #75825.

Steps to reproduce

  1. Open minimal reproduction project.
  2. Run it once and observe as console continuously returns 0 for the looped idle animation.
  3. Uncomment line 11: state_machine.travel("jump"), to travel to an unlooped animation.
  4. Run again, observe get_current_play_position() now prints values in console from 0 to 0.5.

Minimal reproduction project

get_current_play_position() returns 0.zip

@jsjtxietian
Copy link
Contributor

jsjtxietian commented Sep 30, 2023

Confirmed.

Looks like godot will return a huge number for a looped anim —— return is_looping ? HUGE_LENGTH : anim_size - cur_time;
Looks like it's by design. Although I still think it's a bug. cc @TokageItLab

@TokageItLab
Copy link
Member

HUGE_LENGTH is not related. The current_position should not have been allowed to be acquired in the first place.

@TokageItLab TokageItLab closed this as not planned Won't fix, can't repro, duplicate, stale Sep 30, 2023
@vaner-org
Copy link
Author

@TokageItLab Please stop closing my issues without so much as a discussion. It's really frustrating, you did the same with #75843 which now is back to normal in 4.1.

@TokageItLab
Copy link
Member

Isn't this the same with #23629 (comment)? I have been asked that many times and I remember there was no published method to get the playback position of a particular node.

@AThousandShips
Copy link
Member

AThousandShips commented Sep 30, 2023

This method does exist though and has since at least 3.2 (cherry picked, from #42188)

@TokageItLab
Copy link
Member

TokageItLab commented Sep 30, 2023

Ah I see, so StateMachine has its own time. Sorry for my misunderstood. (However #42188 is a very old PR, and I think it was probably a bad thing that it was added whilst not even the NodeAnimation time could be retrieved.)

But this probably doesn't get the correct time due to (or even before) the reworking by #75759. When it transitions without a reset, it does not remember the original length of the state because the current time is zero at the moment of transition.

For example, if you leave at 2s in a 10s state and come back to that state without resetting, you will be recognized as being at 0s in an 8s state AFAIR.

Also, in looping, the current time could be an illegal value due to the infinite length of the current loop due to HUGE_LENGTH.

I believe in order to make those time retrieve properly, it must be necessary to have each Node have the correct time, as mentioned in the comment godotengine/godot-proposals#7867 (comment). In any case, there is currently no semantic way to get the time of any AnimationNode.

BTW, there is no guarantee that the time of the StateMachine's Node and its upstream Nodes are synchronized, so using it to obtain the time of a particular animation is dangerous in the first place.

@vaner-org
Copy link
Author

In 3.5 the playback position always resets to 0 after every loop. So getting a value from 0 to 1 which can then be put to use is simple as $AnimationTree["parameters/playback"].get_current_play_position()/$AnimationTree["parameters/playback"].get_current_length()

It's so silly that in 4 even the bar in the UI doesn't loop.
loop-godot35
loop-godot41

@TokageItLab
Copy link
Member

TokageItLab commented Sep 30, 2023

The progress bar not looping is certainly a GUI issue see also #81715 and there is hack fix PR #82089 but it is not good way. However without a huge refactoring regarding retrieving time, we cannot provide a more stable state than we currently have: using HUGE_LENGTH1.

Footnotes

  1. The current AnimationNode has no way to propagate downstream that information of it is looping or not. The old method would separately determine downstream that it was a loop if the time had been rewound, but this implementation was also silly. Reverse playback was broken, and loops could be falsely detected, such as in nested AnimationNode transitions.

@vaner-org
Copy link
Author

Unfortunately, this means Godot 4's AnimationTree is immature for any game with a moderately complex state machine. Even with whatever suboptimal code 3.5 may have had, it was extremely reliable for my project.

The workflow I'm describing isn't niche, just one that would become a problem fairly late into development, once animation seams start getting stitched together. I don't mind sticking to 3.5 for now, but I think it should be noted in the docs that the get_current_play_position function is non-functional, if only to deter anyone that might start a project in 4 expecting it to work.

@elXill
Copy link

elXill commented Sep 30, 2023

@lullabyist I want to do this as well, get name of the animation and frame, to see how transitions going for debuging/checking purposes. While getting them from engine would be best, I have a "bad" solution for this which is using animation_started and animation_finished signals and counting frames inbetween from _physics_process. I will play around with this more later on but for now it doesn't look bad.. You may wanna try to use it like that if your project doesn't need to be too precise.

@TokageItLab
Copy link
Member

TokageItLab commented Oct 1, 2023

suboptimal code 3.5 may have had, it was extremely reliable for my project

As I mentioned above, the current AnimationNodes (e.g., NodeBlend2, NodeStateMachine and etc) are not linked to any Animation resource except for AnimationNodeAnimation , so it is not possible to get the exact elapsed time for a specific animation.

The 3.x method may work to some extent, but you should be aware that it does not "directly" retrieve the animation's time. It would be working on the unstable scaffolding of the constraint that the AnimationStateMachine "always force to reset any State". But this scaffold has already fallen.

So, the upstream animation time is not exactly synchronized with the downstream in the first place, and even if the 4.x change makes it impossible to get the upstream animation time correctly, it is more of a specification than a bug, since we have not defined the correct behavior for that even if you want it.

Even within 3.x, if you put nested AnimationBlendTree/ StateMachine/BlendSpace1D&2D in a StateMachine, it should break when transitions occur in the nest if there is AtEnd transitions or looped animation with different periods.

I agree that StateMachine is still not mature, but counting elapsed time on the restriction of forced resets as in 3.x, is completely more useless in some cases due to the behavior of forced resets than 4.x.

While getting them from engine would be best, I have a "bad" solution for this which is using animation_started and animation_finished signals and counting frames inbetween from _physics_process.

While this approach may work to some extent, it has the restriction that it is not possible to distinguish between them in the case of there are multiple same animations.

A fundamental problem with the current AnimationTree is that the only time information propagated downstream is "remaining time" and "seeked or not". This means, when "animation length", "current time" and "looping or not" are needed, there is no way to retrieve them downstream, and the only way is to estimate them from the "remaining time" and "seeked or not" information although it is unsafe.

For "remain time", there is also a lack of "selectivity" as to which length should be prioritized when blending short animations and long animations. Currently, the longer animations are basically forced to take precedence, and we prioritize to propagate "looping or not" over the "remaining time" by using HUGE_LENGTH; In other words, "remain time" is replaced by "looping or not" information by HUGE_LENGTH.

Ideally, they should have been propagated separately, but the current AnimationTree cannot, so to stabilize something is forced to sacrifice something. This is why huge refactoring is needed and why this problem is so hard to solve.

This issue can be labeled a bug since it is not good to have elapsed time always 0. However, note that future fixes for accurate time retrieval do not guarantee that your project will be able to migrate without modifications (there was no defined correct behavior so 3.x behavior is not always correct to begin with as I mentioned above) but you can expect the project to work with some property settings.

@vaner-org
Copy link
Author

Thank you so much @TokageItLab

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Bad
Development

Successfully merging a pull request may close this issue.

5 participants