Allow fractional FPS values in Animation Editor

[AtlaStar]

Closes #97548. Care also taken to not reopen issue #92273 by ensuring that the value rounds to the nearest sixteenth. Optionally any factor of 2 should work while ensuring that there isn't error accumulation.

Further testing to ensure issue #91729 isn't reopened, but initial testing suggests that the issue will not reopen with this PR.

[fire]

// The value stored within tscn cannot restored the original FPS due to lack of precision,
// so the value should be limited to integer.

Was this resolved?

[TokageItLab]

I have the same question with @fire. If it can restore float values with the correct precision after serialization (save -> close -> reopen project), that would be fine, but if not, then there needs to be some limit on the precision or other approach.

[AtlaStar]

Yes, so the issue with serialization does appear resolved because the site at which the rounding is occurring will automatically round the error towards the value it was saved as. So for example a length of 10.667 and a step size of 0.133333 appears to be saving and loading correctly because any error that would exist in loading said value will round to the nearest 16th while the error epsilon is significantly small, allowing the error to round correctly back to the value it was saved as.

That said, further testing has shown that there are other changes that need to be made as swapping between FPS and Seconds is introducing a small error to the total animation length with those values in the magnitude of a few ten thousandths. This actually appears to be something that happens in the current 4.4 dev build regardless. Likely warrants it's own bug report.

Edit: to clarify, this is beyond the normal .0001 that appears to be added to the total frame length in all versions due to how the current value is being retrieved/set in the range class for the UI element (It appears to add the minimum value on setting iirc?)

[TokageItLab]

Okay, I understand that instead of limiting the FPS digits, we snap to a rational number of seconds. I think this is acceptable as it is not a bad approach.

However, since such snapping is not consistent with other places, please add a comment in the code.

Also, the FPS must be limited to < 10000, because if the FPS exceeds 5 digits, the upper limit of precision will be exceeded and the snap will be broken.

[AtlaStar]

Currently building a small logic change that will likely need a review

[TokageItLab]

Alright LGTM, please squash this commit into one commit to follow the PR workflow.
https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html#the-interactive-rebase

[AtlaStar]

Rebased

[akien-mga]

Needs another rebase to solve a merge conflict, sorry about it. It seems this code is popular :D

[AtlaStar]

Alright, things are fine, appears to work properly on testing again.

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉

[Macksaur]

Heya, I am trying to untangle a local merge conflict around these changes and I'm not sure I understand this change properly. I have some animations in seconds, lets say 1s long, I am no longer allowed to specify a snap value of 0.1s? I have to choose either 0.0625s or 0.125s?

Why seconds being snapped so aggresively? What is the motivation here?

[AtlaStar]

Hey, @Macksaur great question!

So this doesn't apply to the snap value for seconds at all. What this applies to is the minimum step amount for FPS snap mode, so say you wanted an animation snap step to be 12.25 frames per second, this would internally convert that to the proper amount of seconds. The reason for the change was that previous versions of Godot allowed you to have fractional FPS values while a recent PR had changed that behavior, and that PR was specifically to resolve issues that occur when you try to save certain values to a scene file.

Now even something like 60 FPS was having issues due to the fact that 60 has 3 as a factor, and if it is used as a divisor for anything without a power of 3 you get an infinitely repeating amount of decimals. So when loading a scene it would load as 59.blahblah FPS because 60 FPS is the same thing as 16.66666666666... milliseconds. So there had to be some form of clamping so that the value could be saved to disk and then read from disk while retaining correct values. Before this PR it was clamped at integer values; this opens the door to have non-integer snap values for FPS snapping within reason. So something like a 12.5 FPS timed animation (which is 0.08 seconds between frames) can be supported. Now, the reason 0.0625 for this was simply because it is the reciprocal of a power of 2, specifically 1/16. It was chosen due to the way floats work and it guaranteeing that the least amount of error accumulation, although another value could be selected as well, it just would have to be tested to ensure that weird things don't occur like rounding to an incorrect value on loading.

[AtlaStar]

@Macksaur on second thought, lemme double check that this didn't aggressively screw up normal timing of things since a few things got borked when I was rebasing things and an issue could have slipped through causing the behavior you are describing

[Macksaur]

Thank you for this extra detail! I try to take care not to dirty my VCS with 60 -> 59.999... style changes but rightly I don't understand when it often occurs. This is complex! If this PR smooths that out that's going to be great.

I was sure I tested a clean master branch on 5ccbf6e and it was behaving strangely.

[AtlaStar]

@Macksaur confirmed mistakes were made with regards to changing the step value. Should probably put a guard in place for the specific cases.