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 snapping multiple keys in Animation #88498

Merged
1 commit merged into from
Mar 24, 2024

Conversation

CookieBadger
Copy link
Contributor

@CookieBadger CookieBadger commented Feb 18, 2024

Fixes an old bug, that snapped all moved keyframes in the Animation Player and Bezier editor to the same position, hence reducing a selection of keyframes to a single keyframe.

Before:
AnimationKeyframeSnapError

AnimationKeyframeSnapErrorBezier

After:
AnimationKeyframeSnapErrorAfter

AnimationKeyframeSnapErrorBezierAfter

@AThousandShips AThousandShips requested a review from a team February 18, 2024 15:14
@AThousandShips AThousandShips added bug topic:editor topic:animation cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Feb 18, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Feb 18, 2024
@CookieBadger CookieBadger force-pushed the animation-fix-snapping-multiple branch from 89ae481 to 51672ec Compare February 18, 2024 19:22
@CookieBadger CookieBadger marked this pull request as ready for review February 18, 2024 19:24
@CookieBadger
Copy link
Contributor Author

CookieBadger commented Feb 18, 2024

If you are testing this, in order to test the Bezier editor with multiple selected keyframes need to merge #88459 first (or wait for that one to be merged, then I'll rebase this one).

@CookieBadger CookieBadger force-pushed the animation-fix-snapping-multiple branch from 51672ec to 049550c Compare February 20, 2024 15:03
@CookieBadger
Copy link
Contributor Author

rebased on top of 25a52c6

@CookieBadger
Copy link
Contributor Author

This is ready for review btw, so somebody could give it a look, that'd be great :)

@lyuma
Copy link
Contributor

lyuma commented Mar 7, 2024

There's a weird glitch in the last gif where the top keyframe moves left before moving right. Is there a reason it appears to act differently depending on where the mouse is dragged?

I would think that snap would move the selected keyframes in units of one snap length. Though I've found snap tends to corrupt keyframes even when selecting them so I shy away from the feature in general.

@CookieBadger
Copy link
Contributor Author

CookieBadger commented Mar 9, 2024

@lyuma This is entirely normal and intended. When you select a keyframe and begin dragging the mouse, it snaps the key to the closest snapping point. Say you select it at t=0.6 and snapping is 0.5. Even though you move the mouse in the positive time direction, it would first snap at 0.5, hence the opposite direction of the mouse movement. Only when you move the mouse past t=0.75 it will snap the key to 1.0. This is what you observe in the gif.

Snapping always snaps to the absolute time points, not relatively. The fact that the snapping is corrupt is precisely what this PR tries to solve.

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.

There is a problem with keys teleporting to the mouse cursor position when dragging a key with a length. That must be fixed.

snap.mp4

In the preview video in description there is also a rapid movement at the start of the drag, which seems to be a problem as pointed out by lyuma. Even if that is the behavior you are assuming, it would appear to be a bug to the general user (looks buggy than the old behavior), so until that is resolved this change should not be approvable.

Perhaps the preferred behavior would be to use only the first key for snapping (then most old behaviors need not be changed), propagating the relative movement of the first key to the other keys.

@CookieBadger
Copy link
Contributor Author

CookieBadger commented Mar 16, 2024

I have two questions. As for the key with a length, thanks, that is indeed very problematic, and is an easy fix.

  • But this also raises the question of whether to snap only the beginning, or snap the end that is closer to the mouse to its nearest snapping point, hence enabling snapping the end of a track. This is common in DAWs, so I thought it might be useful here. What do you think?

I also agree that the rapid movement might be unpleasant, so one fix that I thought of, is leaving the current snapping position unchanged if snapping it would move it the opposite direction of the mouse. That would result in this behavior:
SnappingFix1

  • As you see, when passing back, the keys at some point snap to where they were originally. I do not think that this is a bad thing, though, but I could add a flag to prevent this behavior once the key has been moved, what do you think?

Finally, however, I have to disagree that using only the first key for snapping would be preferred, since this leaves the user with less control, and is - in my humble opinion - unintuitive.

@TokageItLab
Copy link
Member

TokageItLab commented Mar 16, 2024

Anyway, snapping based on the mouse position is definitely a strange behavior, so it must be based on one of the keys.

But this also raises the question of whether to snap only the beginning, or snap the end that is closer to the mouse to its nearest snapping point, hence enabling snapping the end of a track. This is common in DAWs, so I thought it might be useful here. What do you think?

In the case of clicking on a blank space in a multiple selection, it is better to snap based on the first key, since it is a rough use case to click on an empty space after multiple selections. It would be uncomfortable not knowing which key will be snapped in priority when dragging just middle of the space between two keys. So using the first key rather than any of them as the basis for snapping is desirable here because it is clearly consistent.

As a compromise, it is recommended to separate the operations according to whether or not you are actually clicking on the key. If you are actually pointing down/dragging a specific key in multiple selections, it is fine to snap based on that key. This means that it only needs to determine whether the key is actually pressed, rather than searching for the shortest key.

@CookieBadger
Copy link
Contributor Author

There is effectively no use case for "clicking on a blank space in a multiple selection" here. You can only drag keys if you explicitly click on one, which makes perfect sense. Clicking on blank space just deselects everything.
Regarding tracks with length, which you originally mentioned, I hope I gather correctly that you would not find snapping the end of an audio track helpful.
I may however note, that you are not responding to my second question, or at least not in a clear way. I am sure we have the same interest in getting this bug fixed that exists probably as long as snapping in the animation player, so it would be more helpful to get a solution-oriented answer here.

@TokageItLab
Copy link
Member

I may however note, that you are not responding to my second question, or at least not in a clear way.

As you see, when passing back, the keys at some point snap to where they were originally. I do not think that this is a bad thing, though, but I could add a flag to prevent this behavior once the key has been moved, what do you think?

I think it doesn't make sense to snap it back to its original position.

If you are trying to implement it as a hack to prevent unintended fast key shaking at the start of a drag, you need to address the root cause of that shaking instead of doing so. Perhaps the cause may be that keys are copied and deleted individually in sequence, so changing the process so that multiple keys are deleted at the same time and added at the same time may fix the problem.

@CookieBadger
Copy link
Contributor Author

I don't think we have a clear mutual understanding of what is meant by the "shaking". I understood that you meant that the keys are snapped at positions that the user would not intend to e.g. when he moves the key in the positive time direction, but the nearest snapping point is in the negative direction. It does look weird in the GIFs I posted, but simply because I am always grabbing a keyframe that is sort of "on the edge" of two snap points, being zoomed out quite a lot. If you try the feature yourself slowly, you might notice what this issue stems from. However, there are no issues in the way keyframes are deleted or inserted, the issue that this PR fixes is simply that the insertion positions are calculated in a wrong way. However, at this point I am unsure what behavior you actually want me to implement. Maybe you want to take a look at the PR again, and reorder your thoughts.

@TokageItLab
Copy link
Member

TokageItLab commented Mar 17, 2024

There is effectively no use case for "clicking on a blank space in a multiple selection" here. You can only drag keys if you explicitly click on one, which makes perfect sense. Clicking on blank space just deselects everything.

So never mind that I was misunderstood about this.

I don't think we have a clear mutual understanding of what is meant by the "shaking".

Shaking means that the key is shaken in an unintended direction for a moment at the start of the drag in the first video as lyuma and I said. I don't know the details of what causes that, but you must fix that behavior anyway.

I am always grabbing a keyframe that is sort of "on the edge" of two snap points

If you are thinking of using two snap points for the entire range, I think it is not a good behavior in Godot which basically has discrete keys; You need to refer to the use case of editing keyframes in AfterEffects or Blender, not DAW. The snap points should only refer the one key that is actually clicked down, and the other keys should only follow relative to it.

@TokageItLab
Copy link
Member

TokageItLab commented Mar 17, 2024

This is entirely normal and intended. When you select a keyframe and begin dragging the mouse, it snaps the key to the closest snapping point. Say you select it at t=0.6 and snapping is 0.5. Even though you move the mouse in the positive time direction, it would first snap at 0.5, hence the opposite direction of the mouse movement. Only when you move the mouse past t=0.75 it will snap the key to 1.0. This is what you observe in the gif.

I confirmed this with the old behavior, but I think it was less noticeable in the old behavior because it snapped individually when multiple selections were made. This PR makes it appear that all keys move in the wrong direction at the same time, so it should be fixed.

To prevent unintended movement, I think it would be better to add a special case at the first movement of the drag (the moment the abs of mouse movement value exceeds CMP_EPSILON), snap to right if it is positive, snap to left if it is negative.

@CookieBadger CookieBadger force-pushed the animation-fix-snapping-multiple branch from 049550c to b2422ae Compare March 17, 2024 17:08
@CookieBadger
Copy link
Contributor Author

I implemented a fix for both the audio track issue, and the issue of keyframes snapping against the direction of mouse movement. Feel free to test it, and let me know if this behavior was what you had in mind.

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.

Okay LGTM! Thanks for adjusting the behavior.

@CookieBadger
Copy link
Contributor Author

my pleasure! :)

@CookieBadger
Copy link
Contributor Author

CookieBadger commented Mar 18, 2024

I discovered there is still a small issue with reselection of the keys. Going to fix now.
EDIT: fixed.

@CookieBadger CookieBadger force-pushed the animation-fix-snapping-multiple branch from b2422ae to d88df64 Compare March 18, 2024 10:21
akien-mga added a commit that referenced this pull request Mar 24, 2024
…ltiple

Fix snapping multiple keys in Animation
@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release topic:animation topic:editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants