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

Nathan Vegdahl · 2023-04-21T18:55:58+02:00

Nathan Vegdahl commented

2023-04-21 18:55:58 +02:00

This addresses #95855 by prioritizing the selection of keys in the following order:

Keys on the active curve.
Keys on any selected curve.
Other keys.

This addresses #95855 by prioritizing the selection of keys in the following order: 1. Keys on the active curve. 2. Keys on any selected curve. 3. Other keys.

Nathan Vegdahl added 1 commit 2023-04-21 18:56:15 +02:00

fd86c286e4 Fix #95855 : prioritize selecting keys on active fcurve

Nathan Vegdahl added this to the Animation & Rigging project 2023-04-21 18:56:33 +02:00

Nathan Vegdahl added the

Module

Animation & Rigging

label 2023-04-21 18:57:56 +02:00

Nathan Vegdahl referenced this pull request

2023-04-21 19:02:05 +02:00

Graph editor - when keys on two different curves overlap, and one curve is active, trying to select the key on the active curve sometimes results in the key on the non-active curve being selected. #95855

Nathan Vegdahl requested review from Sybren A. Stüvel 2023-04-21 19:05:38 +02:00

Nathan Vegdahl reviewed 2023-04-23 11:03:37 +02:00

source/blender/editors/space_graph/graph_select.c Outdated

						
				@ -291,0 +292,4 @@

				  /* 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;

				  for (tNearestVertInfo *nvi = matches->first; nvi != nvi_first_selected; nvi = nvi->next) {

Nathan Vegdahl commented

2023-04-23 11:03:37 +02:00

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.

Nathan Vegdahl added 1 commit 2023-04-24 13:06:03 +02:00

3a417b467c Cleanup: add explicit null pointer check

Not functionally necessary in this case, but it makes the intent
more obvious and makes the code more resilient to future changes.

Nathan Vegdahl force-pushed overlapping_key_selection from 71b280bf1d to 091d3508f5

2023-04-25 11:55:15 +02:00

Compare

Nathan Vegdahl referenced this pull request

2023-04-25 12:06:39 +02:00

Graph editor - when keys on two different curves overlap, and one curve is active, trying to select the key on the active curve sometimes results in the key on the non-active curve being selected. #95855

Sybren A. Stüvel reviewed 2023-04-25 16:21:32 +02:00

Sybren A. Stüvel left a comment

I've left a few ideas to test out.

source/blender/editors/space_graph/graph_select.c Outdated

						
				@ -288,1 +278,3 @@

				    /* if vert is selected, we've got what we want... */

				  /* Find the first selected vert in `matches` if one exists. */

				  tNearestVertInfo *nvi_first_selected = NULL;

				  for (tNearestVertInfo *nvi = matches->first; nvi; nvi = nvi->next) {

Sybren A. Stüvel commented

2023-04-25 16:17:57 +02:00

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

Yes, much better! Thanks!

source/blender/editors/space_graph/graph_select.c Outdated

						
				@ -291,0 +293,4 @@

				  /* 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;

				  for (tNearestVertInfo *nvi = matches->first; nvi && nvi != nvi_first_selected; nvi = nvi->next) {

Sybren A. Stüvel commented

2023-04-25 16:20:13 +02:00

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

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!

source/blender/editors/space_graph/graph_select.c Outdated

						
				@ -291,0 +297,4 @@

				    if (nvi->fcu->flag & FCURVE_ACTIVE) {

				      nvi_to_select = nvi;

				      break;

				    } else if (nvi->fcu->flag & FCURVE_SELECTED && !nvi_to_select) {

Sybren A. Stüvel commented

2023-04-25 16:20:29 +02:00

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

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.

Nathan Vegdahl added 1 commit 2023-04-26 10:19:36 +02:00

5336ce2cd0 Cleanup: switch to LISTBASE_FOREACH and other cosmetic changes

Nathan Vegdahl added 1 commit 2023-04-26 10:44:51 +02:00

bd6249f6b0 Cleanup: improve code comments.

Nathan Vegdahl added 1 commit 2023-04-26 11:13:05 +02:00

b8c5ee028f Cleanup: small refactor of `get_best_nearest_fcurve_vert()`

Sybren A. Stüvel referenced this pull request

2023-05-02 11:03:18 +02:00

Graph editor - when keys on two different curves overlap, and one curve is active, trying to select the key on the active curve sometimes results in the key on the non-active curve being selected. #95855

Sybren A. Stüvel requested changes 2023-05-02 11:08:34 +02:00

Sybren A. Stüvel left a comment

LGTM!