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 separate timeline snapping control to Animation Editor #95638

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

Arnklit
Copy link
Contributor

@Arnklit Arnklit commented Aug 16, 2024

This adds a separate toggle for timeline snapping (disabled by default as per @Calinou 's suggestion in the proposal)
Closes godotengine/godot-proposals#10225

godot.windows.editor.dev.x86_64_fFM33ZjKip.mp4

Both can still be toggled by holding Ctrl/Command

Note I changed the _get_editor_step() function to no longer check for whether snapping was enabled as it was already done before the function call in the _seek_value_changed function. Then I added a check the _animation_key_editor_seek function as well.

@Arnklit Arnklit requested a review from a team as a code owner August 16, 2024 13:26
@AThousandShips AThousandShips added this to the 4.x milestone Aug 16, 2024
@AThousandShips AThousandShips requested a review from a team August 16, 2024 13:35
@TokageItLab
Copy link
Member

TokageItLab commented Aug 16, 2024

It's feedback, but the icons and descriptions are unclear. Although I think it's fine for the function :)

With SnapTimeline and SnapKey, there is a misconception that it snaps to the grid of the timeline and the keys of other tracks (like smart grid); we need to avoid confusing it with such as the "grid snap" and "point snap" in Draw software / DCC software.

So more thought needs to be given to icons. It may be exaggerated to use Enum and explain it in text, but at least the description should be something like "Apply snapping to current time/key", not "Snap Timeline".

Thought:
image

@Arnklit
Copy link
Contributor Author

Arnklit commented Aug 19, 2024

@TokageItLab yeah good point. I'll improve the description. I quite like your icons idea... I'll fire up Inkscape and see what I can do!

@Arnklit Arnklit force-pushed the separate-timeline-snapping branch from 31aee28 to ff40b3a Compare August 19, 2024 12:17
@Arnklit
Copy link
Contributor Author

Arnklit commented Aug 19, 2024

Redesigned the icons and improved the descriptions and renamed a few things.

CquxyPfm4s.mp4

I'm not sure if that timeline snap icon reads well. it kind of looks like a taller version of the filter icon with a magnet next to it.

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.

The tooltip explanation needs some consideration, but I see that the icons and the behavior are fine.

@TokageItLab TokageItLab modified the milestones: 4.x, 4.4 Aug 26, 2024
@Arnklit Arnklit force-pushed the separate-timeline-snapping branch from ff40b3a to 25c3717 Compare September 2, 2024 13:43
@Arnklit
Copy link
Contributor Author

Arnklit commented Sep 2, 2024

@akien-mga rebased and conflict resolved

@Arnklit Arnklit force-pushed the separate-timeline-snapping branch from 25c3717 to 88a866f Compare September 2, 2024 13:58
@Arnklit
Copy link
Contributor Author

Arnklit commented Sep 2, 2024

Sorry @TokageItLab , forgot to update that as I had to do a few and thought it was just the merge conflict on this one. Should be updated now as suggested.

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

Thanks!

@Arnklit Arnklit deleted the separate-timeline-snapping branch September 2, 2024 16:19
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.

Make timeline snapping and keying snapping separate buttons in the animation editor
5 participants