Animation Player FPS shows rounded integer but still internally stores float converted from Seconds

[hsandt]

Tested versions

• Reproducible in: v4.3.stable.official [77dcf97d8]
• Not reproducible in: v4.2.1.stable.official [b09f793f5]

System information

Godot v4.3.stable - Ubuntu 22.04.4 LTS 22.04 - X11 - GLES3 (Compatibility) - NVIDIA GeForce GTX 860M (nvidia; 535.183.01) - Intel(R) Core(TM) i7-4710HQ CPU @ 2.50GHz (8 Threads)

Issue description

Context

I am working with fractional FPS for animations, since I set frame time in ms (in Aseprite) then calculate the corresponding FPS for an Animated Sprite (1000/frame duration in ms), then use a converter (Animated Sprite to Animation Player Convertor for Godot 4.0) from Animated Sprite to Animation Player animation. This leads to FPS such as 12.5 FPS for 80ms frames.

So far in Godot v4.2.1 I could enter either 12.5 FPS or 80ms at the bottom of the Animation Player to get the snapping I wanted:

Issue

Since Godot 4.3, the FPS field only allows entering an integer. Entering a float will round it and display the rounded integer. This leads to imperfect FPS such as 13 instead of 12.5, leading to snapping between frames (observe the vertical blue bar not being snapped to the start of a sprite square preview):

I need to switch back to Seconds, then enter 0.08 and then switch back to FPS to get 12.5 FPS internally:

However, note that the rounded value 13 FPS is still displayed:

although snapping shows we are really still at 12.5 FPS / 80ms:

This is confusing, and furthermore, trying to re-enter 13 FPS manually will not change the value to 13. However, entering a different value like 14, then the old value 13 again will force refresh to 13 instead of 12.5.

Fix suggestion

Revert to showing fractional FPS as before, since they are still stored internally as such as indirectly accessibly via setting Seconds, but this requires an extra step for the user.

Currently there is not even an up/down arrow widget to increase/decrease FPS by 1, so there is no big advantage in showing integers anyway (and we could still add an up/down arrow for people who really want to use integers if we want to).

Steps to reproduce

• Create an Animation Player node with a dummy animation track (the easiest to test timeline snapping is to manually increase the animation duration near the clock icon at the top-right)
• In the Animation Player panel, try to change FPS to a fractional value like 12.5 => rounded to 13
• Move the timeline vertical bar around to test snapping. Add some keyframes there to remember the positions.
• Switch to Seconds, enter 0.08, switch back to FPS => see 13 but internally it's 12.5
• Move the timeline vertical bar around to test snapping. See how it ends at different positions that the previous keyframes.
• Enter 14 FPS, confirm, then 13 again, confirm (to force value refresh).
• Move the timeline vertical bar around to test snapping. See how it's now snapping to the previous keyframes.

Minimal reproduction project (MRP)

N/A

[AtlaStar]

Looks possibly related to #92670

Gonna perform a bisect to be certain, but it also appears that it is intentional. It also seems likely this was the PR that produced the issue given this function given that swapping from seconds to FPS just uses the 1.0/step->get_value() to determine the new snapping value rather than ensuring that the FPS value is an integer value first, while it does appear to round when swapping from FPS to seconds.

[AtlaStar]

confirmed that commit b83dc9b from PR #92670 is what introduced this bug.

Also noting that it exists on Windows 10, along with every other platform afaict.

[AtlaStar]

I am wondering if a solution would be to allow fractional values, but have the result round to the nearest eighth, or sixteenth when read from the TSCN. This would implicitly fix the issue with loading 60 fps as 59.9999 FPS as it would round up to 60 FPS and negate the precision errors that would arise, and shouldn't be so precise as to allow issues to arise.

[TokageItLab]

As I said there, only integers are allowed in the FPS is intended, not a bug. Also, it is not a regression, since there was a problem with https://github.com/godotengine/godot/issues/92273 before and the float FPS was not handled correctly Unless the result of 1/FPS is rational and few decimals. If https://github.com/godotengine/godot/pull/97569 does not cause error problems after serialization, then it is fine and this issue can be fixed, but if not, then it needs some other approach, like serializing the FPS directly, not convert to second.

I am wondering if a solution would be to allow fractional values, but have the result round to the nearest eighth, or sixteenth when read from the TSCN

Wouldn't this result in a different FPS value being serialized than the last set and the snap being shifted the next time the editor is opened? If rounding is performed when loading tscn, the same rounding must be performed when setting the fractional FPS in the editor. In other words, we cannot allow high precision float FPS anywhere on the editor if rounding is performed when loading tscn.

The bug here is that (if I am correct in my understanding of description), after switching from Second to FPS, the FPS is internally set to a fractional FPS, so snapping at the time of switching would be the correct fixing way. Currently, the value that should be snapped is 1.0, and float FPS should not be allowed.

Given the precision issues in the time <-> FPS conversion, a snapping value of about 0.1 or 0.01 might be allowed theoretically. However, float FPS is not allowed in other places such as the Project settings. I understand that the editor can be manipulated independently of the drawing, but I doubt that float FPS snapping is really needed there.

[AtlaStar]

To clarify the train of thought here; instead of allowing any and all fractional values for the FPS, instead you clamp the entered value to a value rounded to the nearest 16th. So typing in 15.0541 would clamp it to 15.0625 which is 15 and 1/16. Typing in 12.46 would round to 12.4375 or 12 and 7/16. These values would then be used to save to the tscn.

As to what I believe this report was reporting; it seems to be reporting 2 different "bugs"; the first being the change in behavior which your PR closed as noted above which isn't a bug but a change in intended behavior. The second half and actual bug is that changing between FPS and Seconds doesn't actually change the FPS value, just how it is displayed, as you mentioned.

[TokageItLab]

Since KeyEditor only allows integers, so the snap-only fix as in solution #97569 was not sufficient, this is reopened. As I explained in #99013, due to accuracy issues, keys snapped in FPS mode cannot be placed at exact times beyond 10 or 100 seconds.

As long as we allow only integers in FPS, we can display what number of frames it is even if the time placement is not exact, but if we allow fractional FPS for KeyEditor, we cannot restore and display times such as 137.5 or 100.0625 from a placed key.

For precise implementation, I guess it is necessary to have a property for FPS setting on the Animation resource apart from the step value, but I am concerned that this would require expensive implementation for animation resources for the not so common case of a fractional FPS.

For now we can recommend the workaround for FPS like 12.5: use x times FPS to be integer and change the timescale to 1/x, or ignore the FPSCompatible mode and use Second mode. Although the latter must be chosen for broadcast formats like 29.97.

[AtlaStar]

Yeah, my fix ended up being more frail than realized, as such I think it should be reverted for now. Any fix involving supporting fractional FPS should likely revert the PR I originally mentioned after doing some fixups to how things are serialized to any generated .tscn and .tres files.