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 auto fit timeline and bezier scale on animation editor #87078

Merged
1 commit merged into from
Mar 24, 2024

Conversation

Hilderin
Copy link
Contributor

@Hilderin Hilderin commented Jan 11, 2024

Add a button at the bottom of the animation editor that change the zoom based on the animation length and the bezier scale based on the values and handles of the displayed tracks. The icon and the tooltip of the button change depending if the bezier editor is displayed or not.

Some refactor was made in animation_track_editor.cpp to remove code duplication with the visibility check of the tracks.

This should help with the issue #85826

The objective was to help animation editor, above all the bezier editor which always starts with a strange scale. For small values, we always need to ctrl+alt+mouse wheel to see the cursves correctly. The auto fit helps a lot with that.

Auto fit button (outside bezier editor):
image

If you change the animation length, the zoom of the timeline can now be automatically updated based on the new length:
image

The size of the editor, the tracks names, etc... are taken into account exactly how it's done in the drawing of the timeline.

When i start Godot and open the Bezier editor for the first time, it looks like that:
image

The button is now updated with a different icon and tooltip for bezier:
image

After I pressed the new button:
image

It uses the min/max values of the keys and the handle positions. Only the visible tracks (not hidden and filtered) are used. That's why i refactored a little bit the code where the visibility of the tracks was made.

Tested with version 4.3.dev on Windows 11

editor/animation_track_editor.h Outdated Show resolved Hide resolved
editor/animation_track_editor.cpp Show resolved Hide resolved
editor/animation_track_editor.cpp Outdated Show resolved Hide resolved
editor/animation_track_editor.cpp Outdated Show resolved Hide resolved
editor/animation_track_editor.cpp Outdated Show resolved Hide resolved
editor/animation_bezier_editor.cpp Outdated Show resolved Hide resolved
editor/animation_bezier_editor.cpp Outdated Show resolved Hide resolved
editor/animation_bezier_editor.h Outdated Show resolved Hide resolved
editor/animation_track_editor.h Outdated Show resolved Hide resolved
editor/animation_track_editor.cpp Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member

Thank you for your contribution! I'd suggest you read this to see some of the style details for the code 🙂

editor/animation_track_editor.cpp Outdated Show resolved Hide resolved
editor/animation_track_editor.cpp Outdated Show resolved Hide resolved
editor/animation_track_editor.cpp Show resolved Hide resolved
@Hilderin Hilderin force-pushed the animation-autofit branch 2 times, most recently from 2a2ae88 to 3716959 Compare January 11, 2024 15:38
@AThousandShips
Copy link
Member

Please use the format tool, see the link I sent 🙂

@Hilderin
Copy link
Contributor Author

Hilderin commented Jan 11, 2024

Oh that's the point I missed, sorry, I'm really new to Open source project. I'm downloading LLVM right now and I'll try it. Thanks!

@fire fire requested a review from a team January 11, 2024 16:58
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

In the TrackEditor, this should be fine.

However, in the BezierEditor, this is similar in behavior to "focus" (Ctrl + F), so you can make a method with the range as an argument and "focus" and "auto fit" should share it.

Also, it performs a huge zoom when the vertical range is 0, so you must avoid the case as an exception, the same as with "focus". This should be avoidable by sharing methods.

@CookieBadger
Copy link
Contributor

@Hilderin are you still planning to finish this? Otherwise, I might help :)

@Hilderin
Copy link
Contributor Author

Yes, I was planning to work on it in the week to come. I was busy last week. Thanks for the offer @CookieBadger .

@CookieBadger
Copy link
Contributor

@Hilderin great to hear, and I really dig your PR :)

@Hilderin
Copy link
Contributor Author

I created a function _zoom_vertically in animation_bezier_editor and used it for the focus and auto_fit and I placed there a minimum height at 0.01 total (-0.005 to 0.005). I was not sure what should be a good minimum height value.
image

I think i messed up the PR with my attempt to merge from master and committing my last changes with a amend commit, amending the merge commit. You have a suggestion to fix that?

@fire fire requested a review from a team February 22, 2024 00:49
@CookieBadger
Copy link
Contributor

@Hilderin about the commits, as long as you did not accidentally delete anything that you weren't supposed to, squashing your commits should fix it, afaik

editor/animation_bezier_editor.cpp Outdated Show resolved Hide resolved
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

However, when clicking the autofit button, the timeline horizontal scroll isn't put at the beginning. This means that if you zoom in towards the end of an animation, the animation will still be scrolled after clicking the autofit button. You need to manually scroll towards the left to see the entire animation. I'd expect this scrolling to happen automatically.

@Hilderin
Copy link
Contributor Author

Wierd, I added this in AnimationTimelineEdit::auto_fit() to scroll the timeline to the left with my latest changes:

hscroll->set_value(0);

Do you means like that:
Before:
image
Alter:
image

Maybe you can show me with a screenshot?

@CookieBadger
Copy link
Contributor

Something that you should take into account, is that there can be keyframes in the negative time before the animation starts, and keyframes placed after the animation ends. This is for example useful when you want the animation to start or end in the middle of a non-linear easing. If you press the zoom button, it only adjusts to the animation length, not to the keyframes. Since in the Bezier editor, the vertical zoom also adjusts to the keyframes, I think it would be logical, if the button either adjusted
a. for the time from the first to the last keyframe (+ buffer time in the end, as it is currently)
b. for the time from the first keyframe to the animation end or the last keyframe, whichever is longer (+ buffer in the end)
image

@Hilderin
Copy link
Contributor Author

Very interesting suggestion... I did not thought about that!
Just to be sure I understand what you suggest, do you think the following should work:

  1. Set begin to zero and end at the animation end (animation lenght)
  2. Loop for all keyframes:
  3. If < begin, set begin to keyframe
  4. If > end, set end to keyframe
  5. Adjust horizontal zoom using (end - begin) + buffer in the end
  6. Scroll horizontally to begin

This should work in Bezier Editor or not.

b. for the time from the first keyframe to the animation end or the last keyframe, whichever is longer (+ buffer in the end)

@CookieBadger
Copy link
Contributor

CookieBadger commented Feb 23, 2024

@Hilderin yes, that would work. Instead of scrolling to a begin timestamp you could also scroll to get_min() of the HScrollBar. This way, you don't have to check where the first keyframe is.

@Hilderin
Copy link
Contributor Author

I did make the modifications suggested. I did not known we could create keyframes before zero or after the animation end. Good to known! Thanks!

I found why the scroll to the beginning was not working everytime. If the vertical and the horizontal zoom was updated at the same time, the modification on the timeline did not update the horizontal scrollbar before the hscroll->set_value was called. I created a _scroll_to_start which I can call deferred and the problem was fixed. Is it a good way to fix this?

It looks like that:
Before:
image
After:
image

@Hilderin
Copy link
Contributor Author

I thought about it and realized that I did not add shortcut for the new button. The text I used for the tooltip was too long and unclear so I renamed it for "Fit to panel", like in a text editor (ex: Google Docs, zoom: Fit). I choose Ctrl-Shift-F for the default keys, it does not seems to be in conflict with other shortcuts and the usage of the F key that is used for Focus seems appropriate.

image
image
image

What do you think?

@CookieBadger
Copy link
Contributor

I love the fact that it has a shortcut, I was actually going to suggest that. Maybe Ctrl+F or Shift+F would be more accessible?

@Hilderin
Copy link
Contributor Author

I changed the default shortcut for Ctrl-F, it's easier to use and still no conflict with others shortcuts (I think).

I'm just waiting on the answer of @TokageItLab about the minimum vertical height (CMP_EPSILON or something else).

editor/animation_bezier_editor.cpp Outdated Show resolved Hide resolved
@TokageItLab TokageItLab modified the milestones: 4.x, 3.x, 4.3 Mar 18, 2024
Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

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

LGTM! Please squash the PR into one commit as follow Pull request workflow.

editor/animation_bezier_editor.h Outdated Show resolved Hide resolved
editor/animation_bezier_editor.cpp Outdated Show resolved Hide resolved
@Hilderin Hilderin force-pushed the animation-autofit branch 3 times, most recently from 69614d3 to 3bfa943 Compare March 20, 2024 00:53
@Hilderin
Copy link
Contributor Author

I was able to squash my commits and made corrections on my comments.

Add a button at the bottom of the animation editor that change the zoom based on the animation length and the bezier scale based on the values and handles of the displayed tracks. The icon and the tooltip of the button change depending if the bezier editor is displayed or not.

Some refactor was made in animation_track_editor.cpp to remove code duplication with the visibility check of the tracks.

This should help with the issue godotengine#85826
@Hilderin Hilderin force-pushed the animation-autofit branch from 3bfa943 to b46d0a6 Compare March 20, 2024 01:02
akien-mga added a commit that referenced this pull request Mar 24, 2024
Add auto fit timeline and bezier scale on animation editor
@akien-mga akien-mga closed this pull request by merging all changes into godotengine:master in 06abc86 Mar 24, 2024
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@Hilderin
Copy link
Contributor Author

Thanks!! And thanks all of you for your help and support :)

@Proggle
Copy link
Contributor

Proggle commented Mar 24, 2024

A much-needed UI improvement, great job :)

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

Successfully merging this pull request may close these issues.

Add a button/shortcut to fit zoom to duration in the animation editor
7 participants