The third argument of `SpriteFrames.add_frames()` is different in 3.x and 4.x, but is not processed by project converter

faulknermano commented 3 weeks ago

Tested versions

Reproducible in 4.0.stable, 4.2.2stable, 4.3.stable Not reproducible in Godot v3.4.4

System information

Godot v4.2.2.stable - Windows 10.0.19044 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3060 (NVIDIA; 31.0.15.2756) - AMD Ryzen 9 5950X 16-Core Processor (32 Threads)

Issue description

When adding frames to SpriteFrames using code, i.e SpriteFrames.add_frames, the resulting playback is laggy.
However, when adding frames using the SpriteFrames Editori UI in the Godot Editor, the resulting playback is smooth/is as expected.

I've packaged 2 repro projects which contains everything that is needed to demonstrate the problem.

This GIF is an example of the laggy playback godot-animatedsprite2d-anim_lag-2

And this is an example of playback that is normal godot-animatedsprite2d-anim_lag-1

Steps to reproduce

Open the uploaded G4 project in Godot 4.x.
Run the project.
The initial walk animation (anim-2) you will see should be rather slow in playback.
Click on the screen; this will switch to another animation (anim-1) which would play back normally.

In the project itself you will see:

In the SpriteFrames of the AnimatedSprite2D node, the anim-1 animation set has been created and loaded.
This is the animation that plays back smoothly.

In the AnimatedSprite.gd script in the project, you will see:

anim-2 has been loaded into the SpriteFrames using SpriteFrames.add_frames()
anim-2 is the animation that plays back slowly.

There are 2 projects, one for G4 and one for G3. Please note that this bug does not exist in G3, and you can test it with the G3 project.

Minimal reproduction project (MRP)

bugreport-animatedsprite2d-add_frames_runtime-godot4-1.zip bugreport-animatedsprite2d-add_frames_runtime-godot3-1.zip

faulknermano commented 3 weeks ago

I've just now discovered something that may circumvent the bug.

In AnimatedSprite2D.gd, there is this bit:

for ndx in range(0, 12):
        var res_file = res_file_format % str(ndx).lpad(3, "0")
        var new_texture = ResourceLoader.load(res_file)
        sprite_frames.add_frame(anim_name, new_texture, ndx)

Note that in SpriteFrames.add_frame I pass ndx. If I omit this:

for ndx in range(0, 12):
        var res_file = res_file_format % str(ndx).lpad(3, "0")
        var new_texture = ResourceLoader.load(res_file)
        sprite_frames.add_frame(anim_name, new_texture)

It seems that the problem is gone.

I think that it's good that I discovered a workaround, but obviously it is still a bug.

TokageItLab commented 3 weeks ago

The third argument of add_frame() has changed from at_pos to duration in 3.x to 4.x, which appears to be the cause. Since the default for duration is 1.0, it should be migrated to the following code:

for ndx in range(0, 12):
        var res_file = res_file_format % str(ndx).lpad(3, "0")
        var new_texture = ResourceLoader.load(res_file)
        sprite_frames.add_frame(anim_name, new_texture, 1.0, ndx)

Calinou commented 3 weeks ago

I'm not sure if the 4.x project converter is able to process method arguments to add a new parameter in the middle yet.

faulknermano commented 3 weeks ago

@TokageItLab, thanks for the input, makes sense. However, the 4.x docs have not been updated to reflect this.

From this change, it likely means that we can no longer insert frames at arbitrary indices?

EDIT: sorry I didn't see the 4th argument! Forget my comment, then. Thanks!

Mickeon commented 3 weeks ago

I'm not sure if the 4.x project converter is able to process method arguments to add a new parameter in the middle yet.

It kinda does several hardcoded conversions already, but this case was not accounted for. It would have to be written manually (which, good luck, that entire code is... something, alright).

faulknermano commented 3 weeks ago

Merely out curiosity, why was the new arg inserted midpoint rather than at the end? When I saw the 4 args, I initially thought that at_position was a naturally to go first before duration since I normally think of where to insert the frames first before setting how long I want to set the frames for.

Just my 2 cents. :-)

Mickeon commented 3 weeks ago

Yeah it is a stupid function signature in hindsight.

godotengine / godot