Anim: Graph Editor Push/Pull operator #112388

Christoph Lendenfeld · 2023-09-14T17:59:21+02:00

2023-09-14 17:59:21 +02:00

This finishes the work that Ares Deveaux started here #106527: Animation: push pull slider

Scales the selected keys from an imaginary line that runs from the start to the end of the segment.

This finishes the work that Ares Deveaux started here [#106527: Animation: push pull slider](https://projects.blender.org/blender/blender/pulls/106527) Scales the selected keys from an imaginary line that runs from the start to the end of the segment. <video src=/attachments/c136eb7f-770a-4365-986a-34d79ecd0043 controls></video>

2023-09-21 12-44-02.mp4

755 KiB

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2023-09-14 17:59:21 +02:00

Christoph Lendenfeld added 6 commits 2023-09-14 17:59:28 +02:00

9b6b41918b Added functions

78a283cc22 simplified the function a bit

33258ccd03 variable name change

c15a4bf98a Merge branch 'main' into ge_push_pull

c2cfb9b158 resolve merge issues

c95d2fe379 improvements

Christoph Lendenfeld added this to the Animation & Rigging project 2023-09-14 17:59:37 +02:00

Christoph Lendenfeld changed title from ~~Animation: Graph Editor Push/Pull operator~~ to Anim: Graph Editor Push/Pull operator

2023-09-15 11:57:25 +02:00

Nathan Vegdahl requested review from Nathan Vegdahl 2023-09-19 11:31:50 +02:00

Nathan Vegdahl commented

2023-09-19 11:44:40 +02:00

Something I didn't think about before in the other PRs for these operators is: how should handles be affected?

When the handles are auto or auto-clamped, the current operators work great. But if they're manually positioned, the handles just translate with the motion of the keys. Sometimes that's probably what we want. But for this operator (and I think some of the others), that leads to weird-feeling results.

Something I didn't think about before in the other PRs for these operators is: how should handles be affected? When the handles are auto or auto-clamped, the current operators work great. But if they're manually positioned, the handles just translate with the motion of the keys. Sometimes that's probably what we want. But for this operator (and I think some of the others), that leads to weird-feeling results.

Christoph Lendenfeld commented

2023-09-21 10:11:25 +02:00

That is a good question. I am not sure we can do anything.
If the user has positioned the handles, that signals an intention for them to stay like that.

On second thought maybe not. The animators intention is to have the curve stay in the shape it is in. The handles are just a means to get that shape.
We could probably move the handles in such a way that the shape of the curve stays intact. But that will be a separate PR

sorry for the rambling

That is a good question. I am not sure we can do anything. If the user has positioned the handles, that signals an intention for them to stay like that. On second thought maybe not. The animators intention is to have the curve stay in the shape it is in. The handles are just a means to get that shape. We could probably move the handles in such a way that the shape of the curve stays intact. But that will be a separate PR sorry for the rambling

Nathan Vegdahl commented

2023-09-21 11:35:38 +02:00

Yeah, I was thinking that with this operator, it behaves as a specific kind of "scaling", so applying the same transform to the manually-set handles probably makes sense. For example, when playing around with this operator, when I'm "pulling" (i.e. making the animation less extreme), the handles not getting squashed down along with the keys results in remaining humps in the curve, which very much goes counter to what I would expect and want.

I'm fine leaving that for another PR, though. Maybe we can tackle all of these new operators together, figuring out what the handle behavior should be for each one.

Yeah, I was thinking that with this operator, it behaves as a specific kind of "scaling", so applying the same transform to the manually-set handles probably makes sense. For example, when playing around with this operator, when I'm "pulling" (i.e. making the animation less extreme), the handles not getting squashed down along with the keys results in remaining humps in the curve, which very much goes counter to what I would expect and want. I'm fine leaving that for another PR, though. Maybe we can tackle all of these new operators together, figuring out what the handle behavior should be for each one.

Nathan Vegdahl requested changes 2023-09-21 12:30:30 +02:00

Nathan Vegdahl left a comment

Just some minor nits in comments and tooltips. Otherwise looks ready to land!

source/blender/editors/animation/keyframes_general.cc Outdated

						
				@ -884,0 +896,4 @@

				  }

				  for (int i = segment->start_index; i < segment->start_index + segment->length; i++) {

				    /* For easy calculation of the curve, the  values are normalized. */

Nathan Vegdahl commented

2023-09-21 12:18:34 +02:00

Very minor typo: a double space between "the" and "values".

nathanvegdahl marked this conversation as resolved

source/blender/editors/space_graph/graph_slider_ops.cc Outdated

						
				@ -2078,0 +2140,4 @@

				  /* Identifiers. */

				  ot->name = "Push Pull Keyframes";

				  ot->idname = "GRAPH_OT_push_pull";

				  ot->description = "Exagerates or minimize the value of the selected keys";

Nathan Vegdahl commented

2023-09-21 12:29:52 +02:00

Spelling: exaggerates.

Also keep the verb form consistent: either "Exaggerates or minimizes" or "Exaggerate or minimize". I think the latter is standard in Blender?

Spelling: exaggerates. Also keep the verb form consistent: either "Exaggerates or minimizes" or "Exaggerate or minimize". I think the latter is standard in Blender?

Christoph Lendenfeld commented

2023-09-21 12:35:36 +02:00

Good catch, thanks!

nathanvegdahl marked this conversation as resolved

Christoph Lendenfeld added 2 commits 2023-09-21 12:35:15 +02:00

36404cd6b3 Merge branch 'main' into ge_push_pull

365c0c4c7b implement review comments

Christoph Lendenfeld requested review from Nathan Vegdahl 2023-09-21 12:35:55 +02:00

Nathan Vegdahl approved these changes 2023-09-21 12:37:02 +02:00

Nathan Vegdahl left a comment

Looks good to me!

Christoph Lendenfeld merged commit 25031f8a86 into main