godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.51k stars 20.26k forks source link

User can't handle the defaults for the generated Animation track options #85981

Open Perversonality opened 9 months ago

Perversonality commented 9 months ago

Tested versions

System information

Windows 10 Godot_v4.2-stable_win64

Issue description

Trying to set up animations using AnimationPlayer and Sprite2D with a sprite sheet containing multiple 6-frame animations.

When specifying Animation.loop_mode = Animation.LOOP_LINEAR the animation runs forwards at the correct speed, then backwards at a slower speed. It should run forwards once and then loop, not run backwards and not at a different speed.

When specifying Animation.loop_mode = Animation.LOOP_PINGPONG, the animation runs forwards once and then stops, which is the same behaviour as LOOP_NONE, which seems to work just fine.

Steps to reproduce

I created a Node2D scene. I added an AnimationPlayer under Node2D. I added a sprite under Node2D, called DunceWizard. I dragged a sprite sheet (png file) into the Texture slot of the sprite. I left the animation settings on the sprite alone. I created a an Animation library through clicking Animation > New in the Animation window of the Animation player. This added a dictionary into the libraries called ""

My code sets the hframes and vframes for the sprite, then calls a function called _add_anim which is as follows.

func _add_anim(animName: String, modifier: int):
    var library: AnimationLibrary = get_animation_library("")
    var anim = Animation.new()
    anim.length = 5
    anim.loop_mode = Animation.LOOP_LINEAR
    library.add_animation(animName, anim)
    var track1 = anim.add_track(Animation.TYPE_VALUE)
    anim.track_set_path(track1, "DunceWizard:frame")
    var time: float = 0
    for frames in dunce_wizard.hframes:
        anim.track_insert_key(track1, time, frames + modifier)
        time+= 0.1

anim.length was set to 5 to highlight the issue of the slower reverse run of the animation - setting it to a value lower than 2 doesn't show the issue well enough.

I have attached the sample scene. Please ignore the second scene named node_2d as it was there for another purpose. Loop_issue.zip

Minimal reproduction project (MRP)

Loop_issue.zip

TokageItLab commented 9 months ago

I assume something is wrong, but I can't quite figure it out.

Is it possible to put the generated animation out as a file? It would be ideal for debugging if the animation could be generated in a way that it could be displayed in a remote scene tree or something. Or it might be fine to use @tools to generate the animation as an actual resource.

Perversonality commented 9 months ago

test_anim.zip

I used ResourceSaver.save to generate the attached tres file. If you want it in another format, please let me know - probably best to let me know how too, just incase it's not obvious... File content is as follows.

[gd_resource type="Animation" format=3]

[resource]
length = 5.0
loop_mode = 1
tracks/0/type = "value"
tracks/0/imported = false
tracks/0/enabled = true
tracks/0/path = NodePath("DunceWizard:frame")
tracks/0/interp = 1
tracks/0/loop_wrap = true
tracks/0/keys = {
"times": PackedFloat32Array(0, 0.1, 0.2, 0.3, 0.4, 0.5),
"transitions": PackedFloat32Array(1, 1, 1, 1, 1, 1),
"update": 0,
"values": [0, 1, 2, 3, 4, 5]
}

I might also build another version of the animation using the UI and export that animation to see if there are any obvious differences.

I'm in the UK, so my replies may seem delayed!

Thanks.

Perversonality commented 9 months ago

Right, think I've worked it out. Might not be a bug, but possibly could be an enhancement... Then again, there might be a bug, or at least an unexpected behaviour in UPDATE_CAPTURE...

Exporting the coded animation and the editor animation, the only obvious difference was the UpdateMode, which I hadn't set in the code, so so was set to 0, whereas the AnimationPlayer UI set it to 1.

If setting LOOP_LINEAR and leaving UpdateMode to the default of UPDATE_CONTINUOUS or 0, then the behaviour is as seen in my original attached scene. If, however, you use the following code to change to UPDATE_DISCRETE then it works, in that the loop runs at the specified frame rate and then pauses on the last frame until the animation length has expired.

anim.value_track_set_update_mode(track1, Animation.UPDATE_DISCRETE)

For completeness, setting UPDATE_CAPTURE instead shows the ping-pong behaviour, which is the case in both code and editor. I'm not sure if that is the correct behaviour as it reads like it should just pick up playing the animation from the last frame, rather than reversing the loop at the end if LINEAR is set?

If this is working-as-designed, would it make sense to automatically set UPDATE_DISCRETE if LOOP_LINEAR is specified, rather than having to work things out as I have just had to do? I'm thinking that the behaviour of DISCRETE is implied by LINEAR. There may be some reason not to do it, but documenting it under LOOP_LINEAR would make sense to me.

From the docs there is no suggestion that CONTINUOUS should cause the reversal, based on LINEAR being set...

https://docs.godotengine.org/en/stable/classes/class_animation.html#enum-animation-loopmode LOOP_LINEAR = 1

At both ends of the animation, the animation will be repeated without changing the playback direction.

https://docs.godotengine.org/en/stable/classes/class_animation.html#enum-animation-updatemode UPDATE_CONTINUOUS = 0

Update between keyframes and hold the value.

Hope this helps, rather that just being a random wall of text :)

Let me know if you need more info, or a new scene with each of the combinations of UpdateMode and Loop.

bs-mwoerner commented 9 months ago

I think this is indeed how it is designed. The logic here is: If the animation is supposed to loop, then it should treat the first keyframe as being the successor of the last keyframe. In your case, that means that between 0.5 and 5.0/0.0 seconds, it interpolates DunceWizard:frame from 5 to 0. This is true for both UPDATE_CONTINUOUS and UPDATE_DISCRETE only that with UPDATE_DISCRETE, there's no visual difference between holding and interpolating back to the start frame. It makes a lot of sense when you think about it, but I won't deny I was stumped by this at one point, too. It would be fantastic to have a "hold frame" flag or keyframe-specific interpolation modes to make a linear animation stop for some time without having to add a second keyframe.

As for now, you can fix your animation by adding

anim.track_insert_key(track1, anim.length, dunce_wizard.hframes - 1)

after the loop.

And actually, if you do use UPDATE_CONTINUOUS instead of UPDATE_DISCRETE and your playback speed is constant, then you only ever need three keyframes for playing back any number sprite frames with a pause at the end:

anim.track_insert_key(track1, 0.0, 0)
anim.track_insert_key(track1, 0.1 * (dunce_wizard.hframes - 1), dunce_wizard.hframes - 1)
anim.track_insert_key(track1, anim.length, dunce_wizard.hframes - 1)

(This can also be taken as an argument why prohibiting the use of interpolation for all looping animations would be counterproductive in a lot of cases.)

UPDATE_CAPTURE should behave the same as UPDATE_CONTINUOUS here, since your first keyframe is at time 0, so there's no time to do any capturing.

As for documentation, I think there's a lot more info in Introduction to the animation features. There you'll also find statements like "When Animation Looping is set but there's no keyframe specified at the end of the animation, the first keyframe is also the last. This means we can extend the animation length to four seconds now, and Godot will also calculate the frames from the last keyframe to the first, moving our sprite back and forth."

Perversonality commented 9 months ago

Thanks for the comment.

it interpolates DunceWizard:frame from 5 to 0.

Whilst it may be working as designed, it feels counter-intuitive. LINEAR suggests playing frames 1-5, then playing 1-5, then playing 1-5, and so on. CONTINUOUS suggests the same - it feels like LINEAR should apply as you are effectively saying "play this in one direction only"

I would expect PINGPONG to be the only state that runs 1-5, 5-1, 1-5, and so on, as that's what PINGPONG is defined as, which is, admittedly maybe just for me, why LINEAR + CONTINUOUS is effectively PINGPONG by proxy...

It just feels like it's not quite what it should be - potentially too complex a set of combinations for what should be a simple set of three operations - "run forwards and repeat", "run forwards, run reverse and repeat" and "run once" - which is what the three LoopMode settings suggest should be happening.

I would also argue that the filling in of the extra time is potentially inconsistent, although it's probably a side conversation. If your base animation is 0.5 seconds, then the setting a length of 1 second would create a perfect start-to-end-to-start loop. If however your length is 1.5 seconds, then the start-to-end still plays in 0.5 seconds, but the end-to-start plays in 1 second, so is slower. I can potentially see some use cases for this, but the three-frame animation you mentioned is probably the overwhelmingly most common use case.

I appreciate this could just be a "me thing" :)

Edit: Not sure if that's you on the Reddit thread, but looks like wrap mode being set to true by default is also a factor. Setting anim.track_set_interpolation_loop_wrap(track1, false) instead of anim.value_track_set_update_mode(track1, Animation.UPDATE_DISCRETE) also fixes the "problem", regardless of the UpdateMode.

Perversonality commented 9 months ago

I think I'm happy for this one to be closed as it's working as designed, even if it is potentially more complex than it could be.

I guess a lot of this is down to it not being immediately apparent what the interactions are between UpdateMode, loop_mode and wrap mode, as well as the code appearing to default to different values compared to the editor UI, leading to more settings needing to be set in code to get the same effect.

bs-mwoerner commented 9 months ago

Not sure if that's you on the Reddit thread, but looks like wrap mode being set to true by default is also a factor.

No, that's not me, I didn't even think of that! It's a bit of a hassle if you want a hold in the middle of an animation, but if your hold is at the end, then yes, just toggling this should be enough. I'd say that's probably the most straight-forward way to handle your case: Do everything as before and then just set this flag to false to tell it to jump back to the first frame instead of moving there smoothly. (The UI actually presents this setting as two different modes: Wrap and Clamp).

Regarding the differences between the code and UI approaches: I think when you add a track in the editor, it chooses a default UPDATE mode based on the data type of the property you're animating. When you create a track in code, however, it always uses the default mode. That's probably for the better, because I can imagine varying defaults could be even more confusing, but it also means that you may get different results seemingly doing the same thing in the editor and in code.

Perversonality commented 9 months ago

Thanks again.

In case anyone else has the same "problem" as me, the final answer to stop the animation from reversing was to set

anim.track_set_interpolation_loop_wrap(track1, false)

which then simply plays the animation and restarts it from the beginning when the length time has elapsed. The rest of the code is as linked in the original report.

dobin commented 9 months ago

I am using AnimationPlayer to play animation (frames) from spritesheets (link). For example for "walking left" using frames 1-9 of a sprite (in a loop, of course).

I was pulling my hair out to figure out why my animation rewinds, even though i explicitly set it to Animation.LOOP_LINEAR. This has a very clear description:

LOOP_LINEAR = 1
  At both ends of the animation, the animation will be repeated without changing the playback direction.

Which it did NOT (and instead behaved like PING_PONG?). Without this github issue, I would have never been able to solve this seemingly trivial problem. So I am with @Perversonality with this, but i didnt read and understand all the details of @bs-mwoerner description of why the current implementation should be the intended behaviour.

If it is the intended behaviour, the pitfall should be documented in other places than in the middle of a introduction tutorial of AnimationPlayer. For example it could be noted in enum LoopMode.

My code, if relevant:

    var anim_library_walk = AnimationLibrary.new()
    for row in dirs:
        var l = 0.3
        var animation = Animation.new()
        animation.length = l * 8
        animation.loop = Animation.LOOP_LINEAR

        for sprite_path in sprite_paths:
            var track = animation.add_track(Animation.TYPE_VALUE)
            animation.track_set_path(track, sprite_path + ":frame")
            for i in range(0, 8):
                var idx = (row * 9) + 1 + i
                var t = i * l
                animation.track_insert_key(track, t, idx)

            animation.track_set_interpolation_loop_wrap(track, false)  # the FIX for some reason
            anim_library_walk.add_animation("walk_" + dirs[row], animation)

    $AnimationPlayer.add_animation_library("walk", anim_library_walk)