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

Add FPS compatible option in second mode of the animation editor snapping #99013

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Nov 10, 2024

Makes it optional to snap the FPS to a close integer FPS when in SecondMode.

image

Partially revert #97569 to disallow fractional FPS because KeyEditor's spin slider syncs with the FPS of the Snap setting, but KeyEditor's spin slider only allows integers, so if fractional FPS is used for snapping, KeyEditor's spin slider will break. Moreover, it is not consistent with this FPS compatible option.

Even if the KeyEditor spin slider allows fractional values, this will still cause problems due to lack of precision1, so the fundamental solution is to separate the FPS values for KeyEditor/Timeline from the step(snapping) values. In other words, it means that we need to add a FPS value as well as a step value to the Animation property, but I feel that it should be an editor-only property because it is too specific to the niche case of fractional FPS.

Footnotes

  1. Since the value of time in KeyEditor exceeds 10.0 usually, there is a lack of precision if we want to use a step of 0.0625 since it will be like 10.0625 or 100.0625

@TokageItLab TokageItLab added this to the 4.4 milestone Nov 10, 2024
@TokageItLab TokageItLab requested a review from a team November 10, 2024 05:32
@TokageItLab TokageItLab requested review from a team as code owners November 10, 2024 05:32
@TokageItLab TokageItLab force-pushed the fps-compat-toggle branch 4 times, most recently from c5e78e1 to 03ad25d Compare November 10, 2024 05:46
@TokageItLab TokageItLab changed the title Add FPS compatible option in second mode of animation editor snapping Add FPS compatible option in second mode of the animation editor snapping Nov 10, 2024
@TokageItLab TokageItLab force-pushed the fps-compat-toggle branch 2 times, most recently from e603a72 to b5701fd Compare November 10, 2024 06:51
Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at animation meeting 👍

UnfavorableEnhancer added a commit to UnfavorableEnhancer/godot that referenced this pull request Nov 11, 2024
so PR godotengine#99013 could be merged properly
@Repiteo Repiteo merged commit 93815e7 into godotengine:master Nov 11, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 11, 2024

Thanks!

return; // Avoid zero div.
}

if (timeline->is_using_fps()) {
_clear_selection(true); // Needs to recreate a spinbox of the KeyEdit.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, can you explain/document this line better? It's causing the animation player to unselect keyframes when I click them in my fork. What is this line trying to solve? Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FPS in the key edit inspector is displayed dependent on the FPS of the snap value. For example, a key placed at 1s will appear as follows in FPS mode:

image image

I thought that changing the FPS with the keyedit inspector displayed would implicitly change the FPS in the inspector and have some negative effect, but sorry I did not realize that _update_snap_unit() is fired each key time is changed in the inspector (although I think it's a redundant).

There are some instability issues, such as undo after switching from second mode and keys not working due to spin box arrows, but this seems to be a problem that has been occurring for some time and probably needs to be revert in this area and stabilized from other parts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, thank you for these details! This is starting to make sense. Hopefully these redundant method calls can be untangled easily. For now I will remove this line and deal with any eccentricities on my end. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Animation editor key snapping is rounded to some predetermined value instead of using exact snap value
6 participants