Details

Reviewers

Sybren A. Stüvel (sybren)
Sebastian Parborg (zeddb)
Luciano Muñoz Sessarego (looch)
Wayde Moss (GuiltyGhost)
Hans Goudey (HooglyBoogly)

Group Reviewers

Animation & Rigging

Summary

Hi!

In Blender, in the graph editor, it is for now impossible to select a curve directly without selecting a keyframe of that curve.
This can sometimes be a bit frustrating, especially for people coming from softwares where this is possible. Being able to select curves directly allows to hide or lock them a bit faster, for instance.

This patch enables the user to directly select animation curves with the usual selection operators. When doing a selection, the usual keyframe selection is first performed, and if no keyframe could be found, then it tries selecting the curve. This is done on a per-curve basis (per-channel), which means that the user may select both a keyframe for a curve A, and a curve B with no keyframe. I found it to be more intuitive, but maybe this is something that should change.

The selection of the curves is done by sampling them in the interval defined by the xmin and the xmax of the selection area, and checking if those points are inside of this area.

Selecting curve is available in the box selection operator, the lasso, and the circle selection operators.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Updated the code to implement zeddb's idea: if the user selects a keyframe, then no curves can be selected (besides from the one the keyframe belongs to).

[Inactive] (DR3D) added a reviewer: Sebastian Parborg (zeddb).Sep 11 2020, 7:46 PM

Fixed a C++ style comment.

CobraA (CobraA) awarded a token.Sep 13 2020, 6:01 PM

@[Inactive] (DR3D) Please edit the patch description so that it's up to date with the current implementation.

Something I was wondering is, if the user happens to select only one curve, should I make it active (and not just selected like it is with the current patch) so that the curve modifiers become available for that curve?

Yes, it should become active if there is nothing else selected. We should try and minimize (ideally, avoid) these cases:

Something is selected, but nothing is active.
Something is active, but not selected.

The patch is corrupted, as the code now contains parts of diffs and is no longer valid C code, so it can't be reviewed in its current state.

This revision now requires changes to proceed.Sep 14 2020, 12:57 PM

Removed the diff files.

Added context (-U1000) in the patch for easier review.

I've split the selection logic in multiple functions, as advised by @Sebastian Parborg (zeddb) .

Todo: ensure no keyframe is currently selected before selecting a curve, as I think keyframe selection should always have the priority over curve selection.

Now checking whether there is already a keyframe selected somewhere, in which case we don't perform curve selection.

Refactored the code to make two completely separate functions for curves and keyframes selection.

There is now an option to enable/disable curve selection in the lasso, box, and circle selection operators, just in case the user does not want to selected curves.
Refactored the code a bit too, and fixed some comments.

[Inactive] (DR3D) edited the summary of this revision. (Show Details)Sep 17 2020, 6:31 PM

LGTM.

Does it look good to you @Sybren A. Stüvel (sybren) ?

if i select the curves, can I move them?, or whats going on ?
by the look of the videos it seems like selecting the curves doesnt have anything to do with selecting the keys
the point of selecting the curves is that it will select the keys it has, so you can also perform "actions" in all the keyframes involved., am I getting somethingwrong?

The point of selecting curves is not to select all the keys.
For now, it is to let the user hide/lock curves more quickly, or add modifiers to them without having to select a key.

I was aiming to change the key selection behaviour too, but @Sebastian Parborg (zeddb) told me it would be better to do it in a future patch: when two keys are overlapping, it would be ideal to prioritize the keys from the curve that is currently selected. Right now, if two or more keys are overlapping, it is often necessary to click many times before selecting the one key we're interested in.

Maybe it could be nice to add the ability to move an entire curve as a whole too, if no key are selected, but I don't think it should be done in this patch.

In D8687#219228, @Luciano Muñoz Sessarego (looch) wrote:

if i select the curves, can I move them?, or whats going on ?
by the look of the videos it seems like selecting the curves doesnt have anything to do with selecting the keys
the point of selecting the curves is that it will select the keys it has, so you can also perform "actions" in all the keyframes involved., am I getting somethingwrong?

The point of this patch is to allow users to select curves as they can do in the channel list without having to move over to the channel list and click on the curve name.
When selecting curves from the channel list, no keyframes are selected on that curve.

Now this can of course change in the future (and probably should), but I feel that this should be in an other patch.

Roman (rwman) added a subscriber: Roman (rwman).Oct 2 2020, 7:38 AM

Christoph Lendenfeld (ChrisLend) awarded a token.Oct 2 2020, 7:23 PM

Sybren A. Stüvel (sybren) added a reviewer: Animation & Rigging.Oct 5 2020, 4:12 PM

In D8687#219228, @Luciano Muñoz Sessarego (looch) wrote:

if i select the curves, can I move them?, or whats going on ?
by the look of the videos it seems like selecting the curves doesnt have anything to do with selecting the keys
the point of selecting the curves is that it will select the keys it has, so you can also perform "actions" in all the keyframes involved., am I getting somethingwrong?

Selecting a curve also makes Blender show its FCurve Modifiers, or allows you to insert a keyframe with I, "Only Selected Channels".

I agree with @Sebastian Parborg (zeddb) that we shouldn't mix two changes together. Blender currently already the concepts "selected curve" and "selected keyframes", and this patch just works with the first. We can have a different design session for handling how selecting a curve relates to the selection of keyframes.

Okay yes, I see a point to it now
:)

Gilberto Rodrigues (gilberto_rodrigues) awarded a token.Oct 6 2020, 4:02 AM

Please separate functional changes (i.e. changes in functionality) from non-functional ones (refactoring, reformatting, commenting). If the feature you're working on requires a cleanup of the existing code, do that cleanup first and submit it as a separate patch. This will make it easier (and thus faster!) to review the changes now, and it will also make it easier to understand the changes later.

This revision now requires changes to proceed.Oct 6 2020, 2:09 PM

Sybren A. Stüvel (sybren) added 1 blocking reviewer(s): Animation & Rigging.Oct 6 2020, 2:09 PM

[Inactive] (DR3D) mentioned this in D9196: Graph Editor - Code refactoring for the selection operators.Oct 13 2020, 4:28 PM

Separated the functional changes (D9196) from curves selection (this diff).
This patch now depends on D9196.

Steve Aplin (stevesilver) awarded a token.Oct 13 2020, 10:51 PM

I think it looks fine code wise still.

Janne Aliu (Jaydead) added a subscriber: Janne Aliu (Jaydead).Oct 18 2020, 9:47 AM

TheRedWaxPolice (TheRedWaxPolice) awarded a token.Oct 19 2020, 4:16 PM

Eric (goodsoulbad) awarded a token.Oct 26 2020, 10:42 AM

Lorenzo Franco (LorenzoFranco) added a subscriber: Lorenzo Franco (LorenzoFranco).Nov 30 2020, 11:32 PM

Updated the diff to reflect the latest changes on the master branch.
The curves selection needed the "filter" variable, which had previously been moved to the separate initialization function (and therefore wasn't retrievable directly). A separate function for the initialization of the filter was created to address this issue.

Gehrig (otterremixchamp) awarded a token.Dec 21 2020, 7:49 PM

The code has some unused variables & function parameters now, probably as a side-effect of the refactoring.

You already imply in the description that the patch that it doesn't allow for click-selects. Is that something you want to add as well at some point? Or maybe in a different patch? Or is it something that was intentionally left out (in which case I'm curious about the reasons).

This gets rid of the unused stuff:

diff --git a/source/blender/editors/space_graph/graph_select.c b/source/blender/editors/space_graph/graph_select.c
index 0eaa6ce9bd2..d38d7c7dd33 100644
--- a/source/blender/editors/space_graph/graph_select.c
+++ b/source/blender/editors/space_graph/graph_select.c
@@ -521,9 +521,7 @@ static int initialize_filter(const SpaceGraph *sipo)
   return filter;
 }
 
-static ListBase initialize_box_select_anim_data(const SpaceGraph *sipo,
-                                                const int filter,
-                                                bAnimContext *ac)
+static ListBase initialize_box_select_anim_data(const int filter, bAnimContext *ac)
 {
   ListBase anim_data = {NULL, NULL};
 
@@ -591,7 +589,7 @@ static void box_select_graphkeys(bAnimContext *ac,
   const rctf rectf = initialize_box_select_coords(ac, rectf_view);
   SpaceGraph *sipo = (SpaceGraph *)ac->sl;
   const int filter = initialize_filter(sipo);
-  ListBase anim_data = initialize_box_select_anim_data(sipo, filter, ac);
+  ListBase anim_data = initialize_box_select_anim_data(filter, ac);
   rctf scaled_rectf;
   KeyframeEditData ked;
   int mapping_flag;
@@ -664,15 +662,14 @@ static void box_select_graphkeys(bAnimContext *ac,
 }
 
 /* This function checks whether a keyframe is already selected. */
-static bool is_key_already_selected(bAnimContext *ac, ListBase *anim_data)
+static bool is_key_already_selected(ListBase *anim_data)
 {
   for (bAnimListElem *ale = anim_data->first; ale; ale = ale->next) {
-    AnimData *adt = ANIM_nla_mapping_get(ac, ale);
-    FCurve *fcu = (FCurve *)ale->key_data;
+    const FCurve *fcu = (FCurve *)ale->key_data;
     BezTriple *bezt;
     uint i;
     for (bezt = fcu->bezt, i = 0; i < fcu->totvert; bezt++, i++) {
-      if ((bezt->f2 & SELECT) || (bezt->f1 & SELECT) || (bezt->f3 & SELECT)) {
+      if (BEZT_ISSEL_ANY(bezt)) {
         return true;
       }
     }
@@ -693,7 +690,7 @@ static void box_select_graphcurves(bAnimContext *ac,
   const rctf rectf = initialize_box_select_coords(ac, rectf_view);
   SpaceGraph *sipo = (SpaceGraph *)ac->sl;
   const int filter = initialize_filter(sipo);
-  ListBase anim_data = initialize_box_select_anim_data(sipo, filter, ac);
+  ListBase anim_data = initialize_box_select_anim_data(filter, ac);
   rctf scaled_rectf;
   KeyframeEditData ked;
   int mapping_flag;
@@ -706,12 +703,11 @@ static void box_select_graphcurves(bAnimContext *ac,
 
   /* Curve selection should only be done if there is no keyframe already selected, unless
    * the user has chosen not to care about already selected keys. */
-  if (check_for_key_selection && is_key_already_selected(ac, &anim_data)) {
+  if (check_for_key_selection && is_key_already_selected(&anim_data)) {
     return;
   }
 
   FCurve *selected_curve = NULL;
-  bool key_selected = false;
   uint selected_count = 0;
 
   for (bAnimListElem *ale = anim_data.first; ale; ale = ale->next) {

source/blender/editors/space_graph/graph_select.c
530	The `sipo` parameter is now no longer necessary.
661	`ac` is unused
664	`adt` is unused
665	`fcu` can be `const`
669	This can be shortened to `BEZT_ISSEL_ANY(bezt)`
698–699	Both `select_cb` and `ok_cb` are unused.
708	`key_selected` is unused

This revision now requires changes to proceed.Dec 22 2020, 10:53 AM

Juan Gabriel (Juangra_Membata) awarded a token.Dec 22 2020, 11:02 AM

Applied Sybren's fixes.

I chose not to implement click selection of curves in this patch to keep things separate, as this patch is already a bit... messy. I also wanted to add only one functionnality per patch (here the area-based selection, i.e. with the lasso, box, and circle selections). I will add click select for curves in a later patch.

[Inactive] (DR3D) marked 7 inline comments as done.Dec 22 2020, 1:31 PM

Again removed a unused variable.

In D8687#247572, @[Inactive] (DR3D) wrote:

I chose not to implement click selection of curves in this patch to keep things separate, as this patch is already a bit... messy. I also wanted to add only one functionnality per patch (here the area-based selection, i.e. with the lasso, box, and circle selections). I will add click select for curves in a later patch.

👍

The patch LGTM. @Luciano Muñoz Sessarego (looch) can you give it one last test to see that it works properly & intuitively?

Jason schleifer (jasonschleifer) added a subscriber: Jason schleifer (jasonschleifer).Dec 22 2020, 7:08 PM

Where do i get a build?

@Luciano Muñoz Sessarego (looch) I've pushed a custom build. https://builder.blender.org/download/temp-D8687-directly_select_fcurves/temp-D8687-directly_select_fcurves-blender-2.92.0-0cf06e247b7b-windows64.zip

I found a few problems. The box select never shows up if a single channel is shown and has an Envelope Fmodifier. It seems to not work whenever an FModifier exists that overrides keys (like Generator and Built-In Function). Other problems noted in the code.

source/blender/editors/space_graph/graph_select.c
713–716	This directly affects the animator's keyframe data. It will lead to keyframes being offset when NLA strips exist after every box selection. You need to undo the mapping at the end of the loop (note: consider the other note below. You may be able to just delete these lines) if (adt) { ANIM_nla_mapping_apply_fcurve(adt, ale->key_data, 1, incl_handles == 0); }
746–747	Only keyframes are remapped by `ANIM_nla_mapping_apply_fcurve()` but Fmodifiers aren't. Currently there is a bug with FModifier drawing that makes it seem like this patch works fine with FModifiers but it doesn't. See https://developer.blender.org/D9953 Instead of applying NLA remapping to keyframes, you can instead apply it by remapping `x` from scene time to fcurve time: eval_time = BKE_nla_tweakedit_remap(adt, x, NLATIME_CONVERT_UNMAP); current_point[1] = (evaluate_fcurve(fcu, eval_time ) + offset) * unit_scale; and removing the keyframe remapping calls.

Wayde Moss (GuiltyGhost) requested changes to this revision.Dec 29 2020, 11:55 PM

Wayde Moss (GuiltyGhost) added a reviewer: Wayde Moss (GuiltyGhost).

This revision now requires changes to proceed.Dec 29 2020, 11:55 PM

I'll download it and test it out !

Hans Goudey (HooglyBoogly) requested changes to this revision.Dec 30 2020, 2:09 AM

Hans Goudey (HooglyBoogly) added a subscriber: Hans Goudey (HooglyBoogly).

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/editors/space_graph/graph_select.c
683	Looks like this is always set to true, why have this as an argument at all in that case? It's nice to avoid these sorts of behavior-changing boolean arguments where possible. If this is kept, at least mark it as const which is the convention and at least lets the reader know it's not going to change. Same goes for the other arguments.
704	For new code please use `LISTBASE_FOREACH` to iterate through a `ListBase`
1047	I might suggest `use_curve_selection` here, which makes it clear that this is a boolean property when the operator property names are listed together.
1048	Use `true` here instead of `1`
1050	Property descriptions for booleans should be phrased to describe what happens when the property is turned on. I also don't think this is a case where two sentences are necessary to describe the feature, which we generally try to avoid. What about something like: `Select the calculated f-curves directly instead of just the keyframes` (just an idea)

slwk1d (Slowwkidd) awarded a token.Dec 30 2020, 4:20 PM

I've tested the build.

Sorry if I sound a bit harsh, but I'm going to try to be as clear and direct as possible, I really appreciate all the work put into this patch but:

I feel I cant support this patch because I don't agree with the functionality.
The point selecting the curve is to select everything that is connected to it, not just the curve, at this point just selecting a curve is for making it active to add modifiers, but you can do that already just as easily and without this functionality... just adding an other layer of complexity to selection in the graph editor.

We need to ask the question:
Should keyframe selection and curve selection be separate?, I think no.
I think curve selection should follow keyframe selection, a keyframe is selected that curve should be active, if you select a curve it should select every key that is part of it:
I know this came from users asking I want to be able to select the curves but nobody asked what that means, I know exactly what it means because it comes from being used to other packages where: selecting a curve will select all keyframes in it, where selecting several curves (either by clicking them or box selecting them) will let you select all the keyframes in them also making it easy to move all those keyframes around, or using it to deselect the keyframes on those curves, it has never meant to be a separate thing from selecting keys, selecting curves has always meant selecting everything that is part of it.
And indeed it's very useful in many cases like when you box select a keyframe and there is an other keyframe underneath and so you can deselect the curve where that keyframe belongs to and that will help.

So in conclusion there are many reasons that this "selecting the curve just because of selecting the curve" is wrong.

IMHO I think we need to bring this back to the general drawing board and make a bigger picture, because I don't see this going the right direction atm.

Again I really appreciate all the work put into getting this functionality together, but I think we need to first take a stab at the bigger picture before committing to things that will likely make things harder.

Hi!

This reminds me a lot of the points that were raised previously.
I indeed feel like I did not discuss enough what the direction of all this is so far. I of course wasn't planning on adding curve selection and just letting things like that, I have other ideas to make this functionnality actually useful (this commit alone is, indeed, quite pointless). I'll describe them here, so that we can debate them.

First of all, we talked about this idea of being able to move the curves directly with Sebastian recently, and he proposed the idea that if a curve is selected while no keyframe is selected, then the "move" operator can move the entire curve (and all the keyframes) directly (kind of like if the keyframes were "implicitely selected"). I think this is a powerful idea, because it means it is compatible with the following functionality that could be very useful:
Box selecting in the graph should have a mode that, when enabled, selects only keyframes from the selected curves (so that when there is overlapping, you can directly select keys only on a given set of curves - you don't even have to select all the overlapped keyframes and then deselect the curves corresponding to the keyframe you don't want). If we did actually select all the keys when a curve gets selected, this functionnality wouldn't be possible, and we'd need to have a system like the one you described: select all the overlapping keys, deselect the curves corresponding to the channels you didn't want. This is slower since most of the time you're only tweaking one channel.
The "L" operator (select linked or something), for now, works in such a way that when you select a key and press "L", the other keys in the curve are selected. It would be nice to extend this operator in a way that when only a set of curves are selected (and no keys), pressing "L" results in selecting the keys from those curves. So you'd get the behaviour you described (the ones we have in other packages).

I would like getting feedback on those ideas before I change anything to the commit (I'll just fix the bugs and little issues pointed out since my last edit).

In D8687#249706, @[Inactive] (DR3D) wrote:

First of all, we talked about this idea of being able to move the curves directly with Sebastian recently, and he proposed the idea that if a curve is selected while no keyframe is selected, then the "move" operator can move the entire curve (and all the keyframes) directly (kind of like if the keyframes were "implicitely selected"). I think this is a powerful idea, because it means it is compatible with the following functionality that could be very useful:

I think this is a bad idea, as it basically moves away from the data-oriented nature of Blender. Instead of selection state being part of the selected thing (i.e. part of the data), this would make it part of the operation on that data. This opens the door to different operators having different opinions on what consistutes "selected".

Box selecting in the graph should have a mode that, when enabled, selects only keyframes from the selected curves (so that when there is overlapping, you can directly select keys only on a given set of curves - you don't even have to select all the overlapped keyframes and then deselect the curves corresponding to the keyframe you don't want).

That raises the question why you're displaying curves, and have them unlocked, while you don't want to work on them.

The "L" operator (select linked or something), for now, works in such a way that when you select a key and press "L", the other keys in the curve are selected. It would be nice to extend this operator in a way that when only a set of curves are selected (and no keys), pressing "L" results in selecting the keys from those curves. So you'd get the behaviour you described (the ones we have in other packages).

I think what @Luciano Muñoz Sessarego (looch) described wouldn't require pressing L (or any other key)to select those keys. If I interpret things correctly, right-clicking on a curve (or box-selecting part of the curve and no keys) would just select all keys of that curve.

Having said that, I don't see a problem with the behaviour you describe, as it's an extension that I don't think will get in the way of anything.

I'm un-accepting the patch in light of this discussion. Good discussion, code is fine too, but clearly the bigger picture is more important here ;-)

Mylo Pinto Rizvi (Mylo) awarded a token.Feb 27 2021, 5:38 PM

Directly select animation curves in the graph editor
Needs RevisionPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 28979

source/blender/editors/space_graph/graph_select.c

Directly select animation curves in the graph editorNeeds RevisionPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 28979

source/blender/editors/space_graph/graph_select.c

Directly select animation curves in the graph editor
Needs RevisionPublic
Actions

Revision Contents
Changeset List