Fix: Inserting keys no longer sets the FCurve channel color #115297

Christoph Lendenfeld · 2023-11-23T12:54:36+01:00

2023-11-23 12:54:36 +01:00

When pressing I in the viewport, the colors of the FCurve channels were no longer set correctly.

Caused by #113504: Anim: Insert keyframes without keying sets

Fix by removing the flag that determined that in the first place,
and read straight from the user preferences. Then move the code
that sets the mode on the FCurve into the function that actually creates it.

For 99% of cases the code went to the user preference flag AUTOKEY_FLAG_XYZ2RGB
and if that was set, the INSERTKEY_XYZ2RGB would be set. The only case where this
was not from the user preferences was from custom keying sets.
There was an override flag for FCurve colors on custom keying sets.
I removed that with this patch since the use case is not apparent
and custom keying sets are hardly used.

When pressing `I` in the viewport, the colors of the FCurve channels were no longer set correctly. Caused by [#113504: Anim: Insert keyframes without keying sets](https://projects.blender.org/blender/blender/pulls/113504) Fix by removing the flag that determined that in the first place, and read straight from the user preferences. Then move the code that sets the mode on the FCurve into the function that actually creates it. For 99% of cases the code went to the user preference flag `AUTOKEY_FLAG_XYZ2RGB` and if that was set, the `INSERTKEY_XYZ2RGB` would be set. The only case where this was not from the user preferences was from custom keying sets. There was an override flag for FCurve colors on custom keying sets. I removed that with this patch since the use case is not apparent and custom keying sets are hardly used.

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2023-11-23 12:54:36 +01:00

Christoph Lendenfeld added 1 commit 2023-11-23 12:54:49 +01:00

the fix ac4eceb150

Christoph Lendenfeld requested review from Nathan Vegdahl 2023-11-23 12:55:21 +01:00

Christoph Lendenfeld added 1 commit 2023-11-23 13:18:14 +01:00

remove unused includes 0d667d9c9e

Christoph Lendenfeld added 1 commit 2023-11-23 13:23:35 +01:00

remove unused imports 48d0c789e4

Christoph Lendenfeld added 1 commit 2023-11-23 16:50:16 +01:00

remove python usage of keying set override ff18c92ccb

Nathan Vegdahl approved these changes 2023-11-24 12:33:37 +01:00

Nathan Vegdahl left a comment

Looks good to me! I just have one comment, but it might be best to address it in a separate cleanup PR.

source/blender/animrig/intern/action.cc

						
				@ -57,6 +60,22 @@ FCurve *action_fcurve_ensure(Main *bmain,

				  fcu->rna_path = BLI_strdup(rna_path);

				  fcu->array_index = array_index;

				  if (U.autokey_flag & AUTOKEY_FLAG_XYZ2RGB) {

Nathan Vegdahl commented

2023-11-24 12:31:49 +01:00

It feels weird to me that this is in autokey_flag when it's not specific to auto keying. I think this should be cleaned up to put the flag in a more obviously "for all keying" place, but not sure if that belongs in this PR or if it should be a separate cleanup PR. I'll leave that up to you.

It feels weird to me that this is in `autokey_flag` when it's not specific to auto keying. I think this should be cleaned up to put the flag in a more obviously "for all keying" place, but not sure if that belongs in this PR or if it should be a separate cleanup PR. I'll leave that up to you.

Christoph Lendenfeld commented

2023-11-24 12:52:33 +01:00

I feel the same but I'll leave that for a separate PR
do you know if there are any issues with versioning when renaming that?

I feel the same but I'll leave that for a separate PR do you know if there are any issues with versioning when renaming that?

Nathan Vegdahl commented

2023-11-24 13:00:53 +01:00

As I understand it, if you just rename a DNA field without doing anything else, DNA sees that as removing the old field and creating a new unrelated one. So if you just e.g. rename autokey_flag -> keying_flag or something like that, then loading older files would ignore whatever was in autokey_flag and end up with whatever the default flags are.

However, I was imagining keeping autokey_flag for auto-key specific flags, and instead moving just that one flag to somewhere more appropriate. Maybe a new keying_flag field. And then in the versioning code you could check autokey_flag for that one flag to move it over. Or something like that.

Having said that, I haven't looked super closely at it, so maybe there are reasons to take another approach. But that's what I'm thinking off the top of my head, at least.

As I understand it, if you just rename a DNA field without doing anything else, DNA sees that as removing the old field and creating a new unrelated one. So if you just e.g. rename `autokey_flag` -> `keying_flag` or something like that, then loading older files would ignore whatever was in `autokey_flag` and end up with whatever the default flags are. However, I was imagining keeping `autokey_flag` for auto-key specific flags, and instead moving just that one flag to somewhere more appropriate. Maybe a new `keying_flag` field. And then in the versioning code you could check `autokey_flag` for that one flag to move it over. Or something like that. Having said that, I haven't looked super closely at it, so maybe there are reasons to take another approach. But that's what I'm thinking off the top of my head, at least.

Christoph Lendenfeld commented

2023-11-24 13:04:28 +01:00

thanks for the insight :)
Yeah splitting the flag would also work. I'll see what I am going to do when I get to it

thanks for the insight :) Yeah splitting the flag would also work. I'll see what I am going to do when I get to it