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

Allow jumping to previous/next keyframe in animation player #96013

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

passivestar
Copy link
Contributor

@passivestar passivestar commented Aug 23, 2024

Adds buttons hotkeys to go to previous/next keyframe

Edit: Instead of buttons this PR now only adds the functionality itself and exposes hotkeys for it. UI buttons may be added in the future, see discussion below. Leaving the video in for demonstration

If anything is selected in the scene it uses the keyframes of selected nodes. Otherwise it uses all keyframes in the current animation

keyframes.mp4

Works the same way in bezier mode, it just ignores non-bezier tracks because you can't see them in the bezier editor

Shift+Alt+A - Previous keyframe
Shift+Alt+D - Next keyframe

Note: Animation editor is using hardcoded shortcuts for all buttons, it would be nice to make them rebindable in a future PR - (Edit: Done)

Note 2: When no keyframe to the left/right exist it jumps to the beginning or the end of the animation. Although those aren't keyframes I think this behaviour can be useful because we don't have dedicated buttons for that

Note 3: I left the repeating code in the loops because moving it into a new function would be a bit awkward since it needs to modify the nearest_key_time and it would hurt readability, but let me know if you think I should do it anyway

Closes godotengine/godot-proposals#10142

@passivestar passivestar requested a review from a team as a code owner August 23, 2024 22:00
@passivestar passivestar force-pushed the keyframe-navigation branch 2 times, most recently from 2810df8 to b2ee1a4 Compare August 23, 2024 22:15
@Chaosus Chaosus added this to the 4.4 milestone Aug 24, 2024
@fire
Copy link
Member

fire commented Aug 24, 2024

I did not review the code yet, but I like the feature proposal.

@Mickeon
Copy link
Contributor

Mickeon commented Aug 24, 2024

Not against this in the slightest but I'm severely concerned about the sheer amount of play buttons present. 5 is already many as is and... somewhat hard to distinguish in their current state.

Main issue is that the current icons are not as distinct as Blender's, so I would be wary of that first.

@passivestar
Copy link
Contributor Author

Not against this in the slightest but I'm severely concerned about the sheer amount of play buttons present. 5 is already many as is and... somewhat hard to distinguish in their current state.

Personally I'd remove the "play from beginning/end" buttons and expose their functionality through Shift+Click on the regular play buttons + keep their hotkeys (Shift+A/D). I find it especially unlikely that "play backwards from end" is used often to justify a button for it. You can also click on the stop button when the animation is not playing to jump to the beginning. That's already 3 different ways to play from the beginning without a button for it

Main issue is that the current icons are not as distinct as Blender's, so I would be wary of that first.

I want to tweak them a little to make them consist of two visually distinct elements instead of one shape for better clarity. Like in blender

@Mickeon
Copy link
Contributor

Mickeon commented Aug 24, 2024

By all means, those sound good to me. Never liked the design of anything Animation-related, so it may be a step in the right direction.

@passivestar
Copy link
Contributor Author

By all means, those sound good to me. Never liked the design of anything Animation-related, so it may be a step in the right direction

I will tweak the icons in this PR, but open a separate one for additional cleanup because if we decide that those extra buttons should go I'll also need to make their hotkeys rebindable as I mentioned in the note above. I might also experiment with a design where play buttons are combined with the stop button

@fire fire requested a review from a team August 24, 2024 17:48
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.

Not against this in the slightest but I'm severely concerned about the sheer amount of play buttons present. 5 is already many as is and... somewhat hard to distinguish in their current state.

I agree with @Mickeon. Although I can agree only with the addition of pull-down menus (or right-click menus) and the addition of shortcuts, but I do not agree with the addition of buttons.

There are other methods to move the timeline cursor besides keyframes, such as moving to the snap position, but it is not made a button. Also, since we are planning to add markers in #91765, there is a possibility of moving to markers, but I don't think that all of them should be made into buttons for moving the timeline cursor in the same line with playback button. So I do not think that keyframes should be treated specially.

The play button should only be for the playback (play() or stop()) API, and if you want to add a button for movement, I believe it should be on a separate line or Somewhere completely recognizable as different from the playback API, like:

image

Personally, I think this PR can be approvable if the buttons are removed.

@passivestar
Copy link
Contributor Author

@TokageItLab Thanks for the feedback! I'm ok with it being shortcut-only for the time being!

I removed the buttons and exposed the shortcuts. We can get this in and then decide on the best UI once the animation editor is feature-complete, including markers and whatever else it may need

h

Since the hardcoded hotkeys needed to be removed from buttons' tooltips I also made the tooltips match the text of the shortcuts for less needed translations and removed the words "selected" and "current pos" which are arguably redundant in context

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.

I think it works well.

However, I think it is weird that the tooltip description for playback from the end is exactly the same for backward and forward playback, so I think some different wording is needed. Any one of the three suggestions below would be sufficient. If you have any other good ideas, feel free to suggest it.

@akien-mga akien-mga merged commit 7795849 into godotengine:master Sep 2, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@passivestar passivestar deleted the keyframe-navigation branch September 2, 2024 11:10
@tutabot
Copy link

tutabot commented Sep 11, 2024

I really love the feature, but removing the UI buttons is a really terrible choice.
Shortcuts are for pro users who use a feature every day. There should always be an easily discoverable option to use a features for people who do not use it every day. A feature which is only available through Shortcuts has the worst discoverability.

I strongly disagree with TokageItLab and Mickeon. The UI Buttons presented by passivestar are clear and easily understandable. I find the play button UI suggestion by TokageItLab much harder to grasp.

@Mickeon
Copy link
Contributor

Mickeon commented Sep 11, 2024

I want the UI buttons, too. Delegating this feature to a secret option is the worst option, but it's worth addressing the current UI in separate PRs, such that it is not too overwhelming for most users but still very much accessible.

@nuclear
Copy link

nuclear commented Sep 20, 2024

+1 for the keyframe buttons! This is a button the average user will want to use.
The play / play from start button should be merged. Play with a regular click, play from start with a shiftclick.

@TokageItLab
Copy link
Member

TokageItLab commented Sep 20, 2024

I have no strong insistence on the UI I have suggested above. However, we need to avoid confusing the "editor controls" with the "play/stop API which also available at runtime". So I would never agree to add it in the same line as the playback button.

Based on experience with AfterEffects, adding a keyframe move button next to TrackName in TrackEditor might be appropriate in terms of positioning in terms of functionality, although it may require a new implementation of moving to keyframes individual track. How about it?

image

Also, there are several cases where keyframe jumps for all tracks will be useless when editing an animation with a large number of keyframes, such as copy of the externally imported 3D animation, or baked tween or cubic interpolation. So handling keyframe-to-keyframe jumps per track helps to handle them correctly.

image

@nuclear
Copy link

nuclear commented Sep 21, 2024

Yes, this sounds like the best solution!

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.

Add buttons to Animation dock for moving cursor to next/previous keyframe
8 participants