Skip to content

Add a Scaling Modifier to scale animation Keys and Markers#105151

Closed
PhairZ wants to merge 1 commit intogodotengine:masterfrom
PhairZ:key-scale
Closed

Add a Scaling Modifier to scale animation Keys and Markers#105151
PhairZ wants to merge 1 commit intogodotengine:masterfrom
PhairZ:key-scale

Conversation

@PhairZ
Copy link
Copy Markdown
Contributor

@PhairZ PhairZ commented Apr 8, 2025

Closes godotengine/godot-proposals#3532

Uses ALT Modifier when moving selected to scale selected instead. Pressing CTRL flips snapping (aka if snapping is on it behaves as it were off and vice versa) matching how moving keyframes works.

2025-04-13.02-06-34.mp4

@AThousandShips AThousandShips requested a review from a team April 9, 2025 11:34
@AThousandShips AThousandShips added this to the 4.x milestone Apr 9, 2025
@PhairZ PhairZ force-pushed the key-scale branch 3 times, most recently from ca31a04 to 50c4948 Compare April 11, 2025 11:36
@PhairZ PhairZ marked this pull request as ready for review April 11, 2025 11:37
@PhairZ

This comment was marked as outdated.

@PhairZ PhairZ changed the title Add a Scaling Modifier to scale animation Keys Add a Scaling Modifier to scale animation Keys and Markers Apr 12, 2025
@TokageItLab TokageItLab moved this to Ready for review in Animation Team Issue Triage Apr 22, 2025
@PhairZ PhairZ force-pushed the key-scale branch 2 times, most recently from fed81ae to c144513 Compare April 29, 2025 03:42
@viksl
Copy link
Copy Markdown
Contributor

viksl commented May 7, 2025

I'll find some time to test it though I'm sick right now.
If you don't mind a question, does this take into account the curve editor for animation too for scaling of keys.
If so, does it work with scaling in both X and Y axis?
If so, is there a way to lock it to work with just one axis for precisions (let's say by pressing X or Y to specify a single axis)?

@PhairZ
Copy link
Copy Markdown
Contributor Author

PhairZ commented May 13, 2025

Scaling in the curve editor has already been handled in PR #100470

@KoBeWi
Copy link
Copy Markdown
Member

KoBeWi commented Jun 12, 2025

I find this somewhat difficult to use. For example I'd want to stretch 1s animation to 2s:

godot.windows.editor.dev.x86_64_ngZ2NWWCyQ.mp4

Dragging the keys to the right always shrinks them, so I have to overshoot the animation, then move them back. Also it involves guess work on how much I need to scale, there is no way to scale in fixed increments (steps).

I think scaling should be opposite, i.e. dragging to the right should expand. Or at least there should be modifier to reverse the direction.
Not sure about steps though. Maybe there could be another modifier to use animation snapping setting for increments (so e.g. with 0.1 snapping, the scale will increase to 1.1, 1.2, 1.3 etc., instead of linearly). Though some separate step value would be better.

@PhairZ
Copy link
Copy Markdown
Contributor Author

PhairZ commented Jun 12, 2025

I tried to match how blender behaves when scaling, because its the animation software most users would be familiar with (and it was mentioned in the proposal).

It scaled the animation relative to the cursor (The timeline current time).

2025-06-12.18-28-30.mp4

I hope you can see the similarities.
This PR also tackles a visual bug where right clicking while moving doesn't visually reset keys.

Edit: Similar to blender, holding CTRL while dragging snaps too. Updated the PR description.

@KoBeWi
Copy link
Copy Markdown
Member

KoBeWi commented Jun 12, 2025

Ok I completely missed that it scales from cursor. Makes sense.
And snapping seems to be working too.

However I noticed there is some weird line appearing when you Alt+drag:

godot.windows.editor.dev.x86_64_gOZO0160jv.mp4

Is this intended?

@PhairZ
Copy link
Copy Markdown
Contributor Author

PhairZ commented Jun 12, 2025

Is this intended?

No, this is not. I'll try to fix it ASAP. Fixed.

@KoBeWi
Copy link
Copy Markdown
Member

KoBeWi commented Jun 12, 2025

When you try to scale 2 markers, with cursor being at the same position as one of them, the marker might move uncontrollably.

jSknDyBM1R.mp4

Notice how it ended up outside timeline, after minimal mouse movement.

@KoBeWi
Copy link
Copy Markdown
Member

KoBeWi commented Jun 25, 2025

I don't think limiting the scaling factor delta would be a good idea.

I printed the delta out of curiosity and it went up to 12k. Eventually I even managed to make it shoot to infinity.
image
This makes the markers unusable.

While it's not that easy to trigger unreasonable deltas, it can happen by accident, potentially breaking the animation.

@PhairZ
Copy link
Copy Markdown
Contributor Author

PhairZ commented Jun 28, 2025

Maybe that can be resolved by making scalings that are from really close to the cursor use a different scaling factor?

@PhairZ PhairZ force-pushed the key-scale branch 2 times, most recently from ecd93c3 to 20dba46 Compare July 3, 2025 18:37
@PhairZ
Copy link
Copy Markdown
Contributor Author

PhairZ commented Jul 3, 2025

@KoBeWi Addressed the issue by making it so that whenever the mouse cursor is too close to pivot, we add a small constant value to increase the difference of the cursors. This seems to work fine, I would be pleased to hear your feedback.
Fixed a minor issue where scaling with a negative factor had no effect and instead scaled as if the factor were absolute.
The intended behavior was that scaling with a negative factor (i.e. scaling from the left of the animation cursor to the right and vice versa) should reverse the keyframes. Keep that in mind when testing.

Copy link
Copy Markdown
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Functionality-wise looks fine now.
Code still needs review from animation team.

@KoBeWi KoBeWi modified the milestones: 4.x, 4.6 Jul 4, 2025
@YeldhamDev
Copy link
Copy Markdown
Member

This honestly feels kinda unpolished. The scaling of bezier keys follows on Unity's footsteps by showing handles for scaling, and I think scaling for basic keys should do the same.

@PhairZ
Copy link
Copy Markdown
Contributor Author

PhairZ commented Sep 3, 2025

I kinda do agree, but scaling like this is usually faster and matches with blender, and works well with the already existent Keyframe/Marker move action.
I don't know if having handles is necessary. For Bezier curves you're working in 2D space in which scaling shortcuts won't click intuitively with users.

I'd like to hear more of your thoughts on this.

@YeldhamDev
Copy link
Copy Markdown
Member

Handles work better because they are more intuitive. Instead of being in a shortcut that the user will not be aware of from the get-go unless they already have experience with it on things like Blender, handles are tangible things they can be interacted with and be can be understood immediately.

Besides, handles and shortcuts aren't mutually exclusive, they can co-exist. But I think handles should be the ones implemented first.

@lyuma
Copy link
Copy Markdown
Contributor

lyuma commented Oct 3, 2025

Feedback from animation meeting. We agree the feature of being able to scale animation keyframes is important, but we some UX fixes. Here are some of the notes of what people brought up in the meeting:

  • Needs visual feedback. Since there is a comparison to Blender, blender will show a dotted line to the origin of the scale operation.
  • By default should scale relative to the midpoint the start and end.
  • "stretch operation" for scaling from the start and end from the edge of the bounding box of what is selected (Unity does this).
  • Allow negative scale

@PhairZ
Copy link
Copy Markdown
Contributor Author

PhairZ commented Oct 3, 2025

Needs visual feedback. Since there is a comparison to Blender, blender will show a dotted line to the origin of the scale operation.

I think this should be added too, currently there's no real feedback to transforming.

By default should scale relative to the midpoint the start and end.

Can I have a reason for why is that? I think being able to scale relative to the timeline head, because it allows for more freedom. Maybe we can have those as an option in the animation settings tho.

"stretch operation" for scaling from the start and end from the edge of the bounding box of what is selected (Unity does this).

YeldhamDev did say so and I am convinced. When I get to working on updating this PR this will definitely be the first thing to be added.

Allow negative scale

The current implementation does allow for this by crossing the origin of scaling with the mouse cursor (If starting from the right side of the anchor, negative scaling would occur when moving the mouse cursor to the left side of the anchor).
This also matches the blender implementation.

@YeldhamDev
Copy link
Copy Markdown
Member

The scale draggers should look something like this, to follow the design of the bezier ones:
scaling

@PhairZ
Copy link
Copy Markdown
Contributor Author

PhairZ commented Oct 5, 2025

This only implements the handles and is still unfinished but I decided to push it for feedback from the animation team and to discuss some bits of the code implementation.

Copy link
Copy Markdown
Contributor Author

@PhairZ PhairZ left a comment

Choose a reason for hiding this comment

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

I hacked away this and pushed it mostly for feedback. This implements the scaling handles. I still didn't settle on the Implementation so that is still to be discussed.
It's still rough around the edges here and there but the idea itself works, but I just want feedback on the behavior and the code.

int track_last = -1;
if (editor->is_selection_active()) {
for (int i = 0; i < animation->get_track_count(); i++) {
for (int j = 0; j < animation->track_get_key_count(i); j++) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this bit should be wrapped up in it's own function because it's quite big and used a couple of times around the code.

Though, I find needing a nested for loop to find the leftmost and rightmost keys inside the entire animation quite demanding, and this can get quite heavy with animations with tens of tracks and hundreds of keys. I did this for now just to get the idea rolling but I don't know if we can just leave this thing living here. :/

scaling_selection = false;
emit_signal(SNAME("scale_selection_cancel"));
moving_selection = false;
emit_signal(SNAME("move_selection_cancel"));
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Someone said this before, maybe this bit should be wrapped in a function. I personally think that "Yeah, less repetition = better", but this line is kinda repeated only once or twice with other copies having a slight change, so either we make a function with more complex logic (maybe something like set_transform_state(TransformState p_state)), or just leave every variation be.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Personally I only make a function if I reuse the code three times..

@Arnklit
Copy link
Copy Markdown
Contributor

Arnklit commented Nov 7, 2025

Tried out this PR as I had been working on the same thing without realising you were working on it.

The scaling with the mouse works well, I don't know if the plan was to add both that and the handle scaling. It appears you haven't done much work on the handle scaling yet and I have that mostly done in my PR. Not sure what the best way to proceed is.

Link to my PR. #112488

@PhairZ
Copy link
Copy Markdown
Contributor Author

PhairZ commented Nov 20, 2025

I have that mostly done in my PR

That's really cool! May I proceed with the rest? I'm not that used to Github yet so I'd like you to tell me if there's a way to get you as an author on this PR too, or give you some credit since you've done basically a lot of the work.

I'm also totally fine by closing this PR as superseded if a request for scaling with Alt key modifier isn't really favored by the animation team.

@Arnklit
Copy link
Copy Markdown
Contributor

Arnklit commented Nov 25, 2025

I have that mostly done in my PR

That's really cool! May I proceed with the rest? I'm not that used to Github yet so I'd like you to tell me if there's a way to get you as an author on this PR too, or give you some credit since you've done basically a lot of the work.

I'm also totally fine by closing this PR as superseded if a request for scaling with Alt key modifier isn't really favored by the animation team.

I think the best thing is just to get one of them merged and then rebase the other after to add the missing features. I think mine is quite ready to merge at this point, so when that's point I could look at rebasing this or making a new PR that adds the mouse shortcut scaling as well using your code and set us as co-authors.

For now I'd just leave this open until we see if mine get's merged :).

@PhairZ PhairZ marked this pull request as draft December 18, 2025 15:59
@PhairZ
Copy link
Copy Markdown
Contributor Author

PhairZ commented Dec 18, 2025

I will close this to be re-implemented after #112488 is hopefully merged.
The re-implementation will focus on the scaling modifier as @lyuma mentioned in the #112488 discussion :)

@PhairZ PhairZ closed this Dec 18, 2025
@github-project-automation github-project-automation bot moved this from Ready for review to Done in Animation Team Issue Triage Dec 18, 2025
@KoBeWi KoBeWi removed this from the 4.6 milestone Dec 18, 2025
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 easier way to scale keyframes in AnimationPlayer Editor