Animation: Match Slope slider #110567

Christoph Lendenfeld · 2023-07-28T11:39:40+02:00

2023-07-28 11:39:40 +02:00

This patch has been authored by Ares Deveaux (#106517: Animation: blend to infinity slider)
I am just finishing it up.

It blends selected keys to the slant of neighboring ones.
It is used to push the segment closer to the values of the next or previous pose.

This patch has been authored by Ares Deveaux ([#106517: Animation: blend to infinity slider](https://projects.blender.org/blender/blender/pulls/106517)) I am just finishing it up. It blends selected keys to the slant of neighboring ones. It is used to push the segment closer to the values of the next or previous pose. <video src="/attachments/718c03f5-3b81-4dde-980b-4b3216e6b904" controls></video>

match_slope 2023-08-10 17-06-08.mp4

922 KiB

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2023-07-28 11:39:40 +02:00

Christoph Lendenfeld added 18 commits 2023-07-28 11:39:47 +02:00

6b0af2fa4d added functions

c13f51e13e changed "delta variable" to constant

aaf9ae70e0 changed declaration of some variables

74e78bfef6 fixed a bug when there is no key beyond the neighboring ones

0c3f7500be Fixed some formating

c3f8c3a35d eliminated vertor split in elements

5a4881e529 rearranged the code to avoid possible "index our of range" error, and simplified the code a bit

232dc85eea merged main

227980c797 formating error

f540381225 a lot of simplification

a0e005c3f6 added warning

5761ec49be cleaned the code a bit

f231c18d95 deleted lines that were not deeded and added bidirectional slider

7b2561452c blend_to_infinity function returns a boolean now. Took the warning out of the loop in the operator.

c2111305f4 if x_delta = 0 should return "false"

5100e560cd Merge branch 'main' into blend_to_infinity_slider

18dbb9819c add operator back to menu

d0399ba471 resolve merge issues

Christoph Lendenfeld requested review from Nate Rupsis 2023-07-28 11:40:02 +02:00

Christoph Lendenfeld added this to the Animation & Rigging project 2023-07-28 11:40:11 +02:00

Christoph Lendenfeld added 1 commit 2023-07-28 15:43:22 +02:00

8439ca48b3 use common header draw code

Nathan Vegdahl reviewed 2023-07-31 13:11:22 +02:00

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

						
				@ -682,0 +683,4 @@

				{

				  const BezTriple *left_key = fcurve_segment_start_get(fcu, segment->start_index);

				  const BezTriple *right_key = fcurve_segment_end_get(fcu, segment->start_index + segment->length);

Nathan Vegdahl commented

2023-07-31 13:11:22 +02:00

Nothing to do with this PR, but just a side-note that these fcurve_segment_*_get() functions are (IMO) poorly named. I would expect them to get the first and last keys within the selected segment, but in fact they attempt to get the keys before and after the segment, which are not themselves part of the segment (falling back to the actual start and end key of the segment only if the before/after don't exist).

Further, fcurve_segment_start_get() takes an inclusive start index whereas fcurve_segment_end_get() takes an exclusive end index. That makes sense from the standpoint of easily computing the index to pass from the segment data, but these clearly aren't actually specific to segments since you can pass any indices you want.

And that brings me to it being weird that segment is even part of their name, since they don't take segments, they directly take indices.

All in all, it took me way more time than it should have to figure out what was going on here, and at first I thought we had off-by-one indexing errors (which isn't the case) because of how confusing this was.

Nothing to do with this PR, but just a side-note that these `fcurve_segment_*_get()` functions are (IMO) poorly named. I would expect them to get the first and last keys *within* the selected segment, but in fact they attempt to get the keys *before* and *after* the segment, which are not themselves part of the segment (falling back to the actual start and end key of the segment only if the before/after don't exist). Further, `fcurve_segment_start_get()` takes an inclusive start index whereas `fcurve_segment_end_get()` takes an exclusive end index. That makes sense from the standpoint of easily computing the index to pass from the segment data, but these clearly aren't actually specific to segments since you can pass any indices you want. And that brings me to it being weird that `segment` is even part of their name, since they don't take segments, they directly take indices. All in all, it took me way more time than it should have to figure out what was going on here, and at first I thought we had off-by-one indexing errors (which isn't the case) because of how confusing this was.

Nate Rupsis commented

2023-07-31 16:31:07 +02:00

imo, In the context of getting *left_key and *right_key i think fcurve_segment_*_get() is understandable. I do think there needs to be some comments around inclusivity.

imo, In the context of getting `*left_key` and `*right_key` i think ` fcurve_segment_*_get()` is understandable. I do think there needs to be some comments around inclusivity.

Nathan Vegdahl commented

2023-07-31 17:13:04 +02:00

Ah, interesting. I interpreted it to mean "left/right-most key of the segment". Although that's probably because of the fcurve_segment_*_get names, in my case.

In any case, I think the fcurve_segment_*_get() names are confusing, at least on their own. But that's definitely not for this PR (and also not @ChrisLend's code, ha ha).

Ah, interesting. I interpreted it to mean "left/right-most key of the segment". Although that's probably *because* of the `fcurve_segment_*_get` names, in my case. In any case, I think the `fcurve_segment_*_get()` names are confusing, at least on their own. But that's definitely not for this PR (and also not @ChrisLend's code, ha ha).

Nathan Vegdahl commented

2023-07-31 13:45:09 +02:00

The code looks good to me. But I have a couple of thoughts:

The name "Blend to Infinity" was not at all suggestive to me of what this feature does. How does "Blend to Slope" sound?
I wonder if it would be worth matching the slope of the handle of the prev/next key when that's been manually set (not for auto-handles).

The code looks good to me. But I have a couple of thoughts: - The name "Blend to Infinity" was not at all suggestive to me of what this feature does. How does "Blend to Slope" sound? - I wonder if it would be worth matching the slope of the *handle* of the prev/next key when that's been manually set (not for auto-handles).

Nathan Vegdahl referenced this pull request

2023-07-31 14:24:35 +02:00

Animation: Blend Offset Slider #110544

Nate Rupsis approved these changes 2023-07-31 16:37:54 +02:00

Nate Rupsis left a comment

lgtm.

I think we should bring up the name in the next Animation module meeting. I think an argument could be made either way, but would be good to get others feedback.

lgtm. I think we should bring up the name in the next Animation module meeting. I think an argument could be made either way, but would be good to get others feedback.

👍 2

Nathan Vegdahl commented

2023-07-31 17:24:29 +02:00

I think we should bring up the name in the next Animation module meeting.

Good call. Added to the agenda.

> I think we should bring up the name in the next Animation module meeting. Good call. Added to the agenda.

Nathan Vegdahl commented

2023-08-10 11:49:09 +02:00

The consensus in the module meeting was to rename this to "Match Slope". Other than that, this looks ready to land to me.

Christoph Lendenfeld added 2 commits 2023-08-10 16:52:12 +02:00

9920070098 Merge branch 'main' into blend_to_infinity_slider

4fad9838c4 resolve merge issues

Christoph Lendenfeld added 1 commit 2023-08-10 17:10:20 +02:00

c5bcd354fa rename to match slope

Nathan Vegdahl reviewed 2023-08-10 17:18:28 +02:00

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

						
				@ -1077,0 +1129,4 @@

				  WM_event_add_notifier(C, NC_ANIMATION | ND_KEYFRAME | NA_EDITED, NULL);

				}

				static int blend_to_infinity_invoke(bContext *C, wmOperator *op, const wmEvent *event)