blender
/
blender
134
398
Fork 579
Code Issues 6.3k Pull Requests 754 Projects 19 Wiki Activity

Fix #95855: prioritize selecting keys on active fcurve #107223

Merged
Nathan Vegdahl merged 6 commits from nathanvegdahl/blender:overlapping_key_selection into main 2023-05-04 10:10:41 +02:00
Conversation 0 Commits 6 Files Changed 1 +40 -20
1 changed files with 40 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

60
source/blender/editors/space_graph/graph_select.c
View File

@ -142,7 +142,8 @@ static void nearest_fcurve_vert_store(ListBase *matches,
nvi->frame = bezt->vec[1][0]; /* currently in global time... */
nvi->sel = BEZT_ISSEL_ANY(bezt); /* XXX... should this use the individual verts instead? */
/* NOTE: `hpoint` is -1,0,1, but `BEZT_ISSEL_IDX` expects 0,1,2. */
nvi->sel = BEZT_ISSEL_IDX(bezt, hpoint + 1);
/* add to list of matches if appropriate... */
if (replace == 0) {
@ -259,9 +260,6 @@ static void get_nearest_fcurve_verts_list(bAnimContext *ac, const int mval[2], L
/* helper for find_nearest_fcurve_vert() - get the best match to use */
static tNearestVertInfo *get_best_nearest_fcurve_vert(ListBase *matches)
{
tNearestVertInfo *nvi = NULL;
short found = 0;
/* abort if list is empty */
if (BLI_listbase_is_empty(matches)) {
return NULL;
@ -273,27 +271,49 @@ static tNearestVertInfo *get_best_nearest_fcurve_vert(ListBase *matches)
return BLI_pophead(matches);
}
/* try to find the first selected F-Curve vert, then take the one after it */
for (nvi = matches->first; nvi; nvi = nvi->next) {
/* which mode of search are we in: find first selected, or find vert? */
if (found) {
/* Just take this vert now that we've found the selected one
* - We'll need to remove this from the list
* so that it can be returned to the original caller.
*/
BLI_remlink(matches, nvi);
return nvi;
}
/* The goal of the remaining code below is to prioritize selecting verts on
* selected fcurves, but to still cycle through the vertices in `matches` if
* a selected-fcurve vertex is already selected. */
/* if vert is selected, we've got what we want... */
/* Try to find the first selected vert in `matches`. Additionally, if
* one exists, rotate `matches` to put it last in the list and the vert
* following it first, since that's the order we'll want to scan in. */
Sybren A. Stüvel commented 2023-04-25 16:17:57 +02:00
Outdated
Review
Copy Link

Use the LISTBASE_FOREACH() for such loops, they improve readability.

Use the `LISTBASE_FOREACH()` for such loops, they improve readability.
Nathan Vegdahl commented 2023-04-26 10:20:12 +02:00
Outdated
Review
Copy Link

Yes, much better! Thanks!

Yes, much better! Thanks!
tNearestVertInfo *nvi_first_selected = NULL;
LISTBASE_FOREACH (tNearestVertInfo *, nvi, matches) {
if (nvi->sel) {
found = 1;
nvi_first_selected = nvi;
BLI_listbase_rotate_last(matches, nvi_first_selected);
break;
}
}
/* if we're still here, this means that we failed to find anything appropriate in the first pass,
* so just take the first item now...
*/
/* Try to find the next vert that's on the active fcurve, falling back
* to the next vert on any selected fcurve if that's not found. */
tNearestVertInfo *nvi_to_select = NULL;
LISTBASE_FOREACH (tNearestVertInfo *, nvi, matches) {
if (nvi == nvi_first_selected) {
continue;
Nathan Vegdahl commented 2023-04-23 11:03:37 +02:00
Outdated
Review
Copy Link

Note to self when I'm back in the office:

Although the nvi != nvi_first_selected check works since nvi_first_selected will be null if no selected vert was found, this isn't immediately obvious skimming the code, and feels delicate and easily breakable by future changes. So perhaps being explicit and doing nvi && nvi != nvi_first_selected would be better here, even though it's functionally unnecessary.

Note to self when I'm back in the office: Although the `nvi != nvi_first_selected` check works since `nvi_first_selected` will be null if no selected vert was found, this isn't immediately obvious skimming the code, and feels delicate and easily breakable by future changes. So perhaps being explicit and doing `nvi && nvi != nvi_first_selected` would be better here, even though it's functionally unnecessary.
}
Sybren A. Stüvel commented 2023-04-25 16:20:13 +02:00
Outdated
Review
Copy Link

Maybe here do a LISTBASE_FOREACH, and put the nvi != nvi_first_selected into the body, something like this (haven't actually tested this):

LISTBASE_FOREACH(tNearestVertInfo *, nvi, matches) {
  if (nvi == nvi_first_selected) break;
  ...
}
Maybe here do a `LISTBASE_FOREACH`, and put the `nvi != nvi_first_selected` into the body, something like this (haven't actually tested this): ```c LISTBASE_FOREACH(tNearestVertInfo *, nvi, matches) { if (nvi == nvi_first_selected) break; ... } ```
Nathan Vegdahl commented 2023-04-26 10:22:30 +02:00
Outdated
Review
Copy Link

Yeah, this and your other comment below helped me flatten things out in a nice way that makes the code much clearer and a little less delicate, I think. Thanks!

Yeah, this and your other comment below helped me flatten things out in a nice way that makes the code much clearer and a little less delicate, I think. Thanks!
if (nvi->fcu->flag & FCURVE_ACTIVE) {
nvi_to_select = nvi;
break;
Sybren A. Stüvel commented 2023-04-25 16:20:29 +02:00
Outdated
Review
Copy Link

No else after a break. Also this seems like your editor isn't automatically using clang-format yet.

No `else` after a `break`. Also this seems like your editor isn't automatically using clang-format yet.
Nathan Vegdahl commented 2023-04-26 09:40:30 +02:00
Outdated
Review
Copy Link

Also this seems like your editor isn't automatically using clang-format yet.

Ah! Thanks for the reminder. I set it up in vscode, but forgot to do so in Helix as well.

> Also this seems like your editor isn't automatically using clang-format yet. Ah! Thanks for the reminder. I set it up in vscode, but forgot to do so in Helix as well.
}
if (nvi->fcu->flag & FCURVE_SELECTED && !nvi_to_select) {
nvi_to_select = nvi;
}
}
/* If we found a vert on a selected fcurve, return it. */
if (nvi_to_select) {
BLI_remlink(matches, nvi_to_select);
return nvi_to_select;
}
/* If we're still here, that means we didn't find any verts on selected
* fcurves. So return the head (which is also the item following
* `nvi_first_selected` if that was found). */
return BLI_pophead(matches);
}