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

Fix unstable AnimationTrackEditor snapping value #92670

Merged

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Jun 2, 2024

Fixes #91729, Fixes #92273. Fundamentally, both are due to lack of precision.

0.0333 second cannot be snapped to 1.0 second because the lack of precision; the result of 0.0333 * 30 is 0.9999.

At some point, for timeline current time, even in second mode, it seems snap was to be done by dividing after the FPS conversion by an int value. However, the snap method for moving keyframes was implemented differently, resulting in inconsistent results. Considering the use case, an internal/temporary int conversion seems to make sense.

The 60 fps value is actually converted to 0.01666666666667 second and stored in animation as 0.0166667 second. I don't know if this is a change in 4.0 or not, but I suspect it is the reason why it is not a problem in 3.x (it stored 60 fps without converting?). The problem in 4.0 is that after storing the value in .tscn, the inverse is computed.

The problem in 4.0 is that after storing the value in .tscn, the resuld of 1.0 / 0.0166667 is 59.9999 fps, although 1.0 / 0.01666666666667 is 60 fps before storing. This can be solved by reducing the user-specifiable precision when in fps mode. As I mentioned in my comment, considering the format for broadcasting (such as 29.97 fps), I think 2 decimal places are sufficient.


By the way, make sure you don't confuse #91729 with #92273. They are different calculations and need to be corrected in different places:

@TokageItLab
Copy link
Member Author

KeyTime in fps mode seems to be limited to int by KeyEditor to begin with.

AnimationTrackKeyEditEditor

    if (use_fps) {
        spinner->set_step(1);
        spinner->set_hide_slider(true);
        real_t fps = animation->get_step();
        if (fps > 0) {
            fps = 1.0 / fps;
        }
        spinner->set_value(key_ofs * fps);
    }

Considering this, limiting with int may be fine. Although for broadcast, it may need to be encoded externally, since the second decimal place is defined as a format, but it needs technically more precise (multiplication comes in to add frames for the signal), so it would not be very useful to support decimals here.

@TokageItLab TokageItLab marked this pull request as draft June 2, 2024 16:58
@TokageItLab TokageItLab force-pushed the fix-animation-track-editor-snap branch from f9de38c to 6c1a3b8 Compare June 2, 2024 17:39
@TokageItLab TokageItLab marked this pull request as ready for review June 2, 2024 17:40
@TokageItLab TokageItLab force-pushed the fix-animation-track-editor-snap branch from 6c1a3b8 to 47b5c52 Compare June 4, 2024 12:20
@TokageItLab TokageItLab force-pushed the fix-animation-track-editor-snap branch from 47b5c52 to a9e1fea Compare June 4, 2024 12:25
@TokageItLab TokageItLab force-pushed the fix-animation-track-editor-snap branch 2 times, most recently from b3e8444 to f4ab7e4 Compare June 4, 2024 12:41
editor/animation_track_editor.cpp Outdated Show resolved Hide resolved
@TokageItLab TokageItLab force-pushed the fix-animation-track-editor-snap branch from f4ab7e4 to b83dc9b Compare June 4, 2024 13:06
@akien-mga akien-mga merged commit 33b201f into godotengine:master Jun 7, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants