blender/blender
149
475
Fork 701
Code Issues 6.7k Pull Requests 890 Projects 18 Wiki Activity

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

Merged
Christoph Lendenfeld merged 4 commits from ChrisLend/blender:fix_fcurve_color into main 2023-11-24 13:05:19 +01:00
Conversation 0 Commits 4 Files Changed 7 +19 -65
7 changed files with 19 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
scripts/startup/bl_ui/properties_scene.py
View File

@ -95,10 +95,6 @@ class SceneKeyingSetsPanel:
"use_insertkey_override_visual", "use_insertkey_visual",
userpref_fallback="use_visual_keying",
)
SceneKeyingSetsPanel._draw_keyframing_setting(
context, layout, ks, ksp, iface_("XYZ to RGB"),
"use_insertkey_override_xyz_to_rgb", "use_insertkey_xyz_to_rgb",
)
@staticmethod
def _draw_keyframing_setting(context, layout, ks, ksp, label, toggle_prop, prop, userpref_fallback=None):

19
source/blender/animrig/intern/action.cc
View File

@ -13,6 +13,9 @@
#include "BLI_string.h"
#include "DEG_depsgraph_build.hh"
#include "DNA_anim_types.h"
#include "RNA_access.hh"
#include "RNA_path.hh"
#include "RNA_prototypes.h"
namespace blender::animrig {
@ -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
Review
Copy Link

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
Review
Copy Link

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
Review
Copy Link

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
Review
Copy Link

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
/* For Loc/Rot/Scale and also Color F-Curves, the color of the F-Curve in the Graph Editor,
* is determined by the array index for the F-Curve.
*/
PropertyRNA *prop;
PointerRNA r_ptr;
RNA_path_resolve_property(ptr, rna_path, &r_ptr, &prop);
PropertySubType prop_subtype = RNA_property_subtype(prop);
if (ELEM(prop_subtype, PROP_TRANSLATION, PROP_XYZ, PROP_EULER, PROP_COLOR, PROP_COORDS)) {
fcu->color_mode = FCURVE_COLOR_AUTO_RGB;
}
else if (ELEM(prop_subtype, PROP_QUATERNION)) {
fcu->color_mode = FCURVE_COLOR_AUTO_YRGB;
}
}
if (group) {
bActionGroup *agrp = BKE_action_group_find_name(act, group);

16
source/blender/animrig/intern/keyframing.cc
View File

@ -37,10 +37,8 @@
#include "ED_keyframing.hh"
#include "MEM_guardedalloc.h"
#include "RNA_access.hh"
#include "RNA_define.hh"
#include "RNA_path.hh"
#include "RNA_prototypes.h"
#include "RNA_types.hh"
#include "WM_api.hh"
#include "WM_types.hh"
@ -547,20 +545,6 @@ static bool insert_keyframe_fcurve_value(Main *bmain,
const bool is_new_curve = (fcu->totvert == 0);
/* Set color mode if the F-Curve is new (i.e. without any keyframes). */
if (is_new_curve && (flag & INSERTKEY_XYZ2RGB)) {
/* For Loc/Rot/Scale and also Color F-Curves, the color of the F-Curve in the Graph Editor,
* is determined by the array index for the F-Curve
*/
PropertySubType prop_subtype = RNA_property_subtype(prop);
if (ELEM(prop_subtype, PROP_TRANSLATION, PROP_XYZ, PROP_EULER, PROP_COLOR, PROP_COORDS)) {
fcu->color_mode = FCURVE_COLOR_AUTO_RGB;
}
else if (ELEM(prop_subtype, PROP_QUATERNION)) {
fcu->color_mode = FCURVE_COLOR_AUTO_YRGB;
}
}
/* If the curve has only one key, make it cyclic if appropriate. */
const bool is_cyclic_action = (flag & INSERTKEY_CYCLE_AWARE) && BKE_action_is_cyclic(act);

5
source/blender/editors/animation/keyframing.cc
View File

@ -91,11 +91,6 @@ eInsertKeyFlags ANIM_get_keyframing_flags(Scene *scene, const bool use_autokey_m
if (is_autokey_flag(scene, AUTOKEY_FLAG_INSERTNEEDED)) {
flag |= INSERTKEY_NEEDED;
}
/* default F-Curve color mode - RGB from XYZ indices */
if (is_autokey_flag(scene, AUTOKEY_FLAG_XYZ2RGB)) {
flag |= INSERTKEY_XYZ2RGB;
}
}
/* only if including settings from the autokeying mode... */

5
source/blender/editors/animation/keyingsets.cc
View File

@ -294,10 +294,6 @@ static int add_keyingset_button_exec(bContext *C, wmOperator *op)
keyingflag |= ANIM_get_keyframing_flags(scene, false);
if (blender::animrig::is_autokey_flag(scene, AUTOKEY_FLAG_XYZ2RGB)) {
keyingflag |= INSERTKEY_XYZ2RGB;
}
/* call the API func, and set the active keyingset index */
ks = BKE_keyingset_add(
&scene->keyingsets, "ButtonKeyingSet", "Button Keying Set", flag, keyingflag);
@ -1011,7 +1007,6 @@ static eInsertKeyFlags keyingset_apply_keying_flags(const eInsertKeyFlags base_f
*/
APPLY_KEYINGFLAG_OVERRIDE(INSERTKEY_NEEDED)
APPLY_KEYINGFLAG_OVERRIDE(INSERTKEY_MATRIX)
APPLY_KEYINGFLAG_OVERRIDE(INSERTKEY_XYZ2RGB)
#undef APPLY_KEYINGFLAG_OVERRIDE

2
source/blender/makesdna/DNA_anim_types.h
View File

@ -1035,8 +1035,6 @@ typedef enum eInsertKeyFlags {
/* INSERTKEY_FASTR = (1 << 3), */ /* UNUSED */
/** only replace an existing keyframe (this overrides INSERTKEY_NEEDED) */
INSERTKEY_REPLACE = (1 << 4),
/** transform F-Curves should have XYZ->RGB color mode */
INSERTKEY_XYZ2RGB = (1 << 5),
/** ignore user-prefs (needed for predictable API use) */
INSERTKEY_NO_USERPREF = (1 << 6),
/**

33
source/blender/makesrna/intern/rna_animation.cc
View File

@ -51,12 +51,6 @@ const EnumPropertyItem rna_enum_keying_flag_items[] = {
0,
"Visual Keying",
"Insert keyframes based on 'visual transforms'"},
{INSERTKEY_XYZ2RGB,
"INSERTKEY_XYZ_TO_RGB",
0,
"XYZ=RGB Colors",
"Color for newly added transformation F-Curves (Location, Rotation, Scale) "
"and also Color is based on the transform axis"},
{0, nullptr, 0, nullptr, nullptr},
};
@ -72,12 +66,6 @@ const EnumPropertyItem rna_enum_keying_flag_api_items[] = {
0,
"Visual Keying",
"Insert keyframes based on 'visual transforms'"},
{INSERTKEY_XYZ2RGB,
"INSERTKEY_XYZ_TO_RGB",
0,
"XYZ=RGB Colors",
"Color for newly added transformation F-Curves (Location, Rotation, Scale) "
"and also Color is based on the transform axis"},
{INSERTKEY_REPLACE,
"INSERTKEY_REPLACE",
0,
@ -877,17 +865,6 @@ static void rna_def_common_keying_flags(StructRNA *srna, short reg)
RNA_def_property_flag(prop, PROP_REGISTER_OPTIONAL);
}
prop = RNA_def_property(srna, "use_insertkey_override_xyz_to_rgb", PROP_BOOLEAN, PROP_NONE);
RNA_def_property_boolean_sdna(prop, nullptr, "keyingoverride", INSERTKEY_XYZ2RGB);
RNA_def_property_ui_text(
prop,
"Override F-Curve Colors - XYZ to RGB",
"Override default setting to set color for newly added transformation F-Curves "
"(Location, Rotation, Scale) to be based on the transform axis");
if (reg) {
RNA_def_property_flag(prop, PROP_REGISTER_OPTIONAL);
}
/* value to override defaults with */
prop = RNA_def_property(srna, "use_insertkey_needed", PROP_BOOLEAN, PROP_NONE);
RNA_def_property_boolean_sdna(prop, nullptr, "keyingflag", INSERTKEY_NEEDED);
@ -905,16 +882,6 @@ static void rna_def_common_keying_flags(StructRNA *srna, short reg)
if (reg) {
RNA_def_property_flag(prop, PROP_REGISTER_OPTIONAL);
}
prop = RNA_def_property(srna, "use_insertkey_xyz_to_rgb", PROP_BOOLEAN, PROP_NONE);
RNA_def_property_boolean_sdna(prop, nullptr, "keyingflag", INSERTKEY_XYZ2RGB);
RNA_def_property_ui_text(prop,
"F-Curve Colors - XYZ to RGB",
"Color for newly added transformation F-Curves (Location, Rotation, "
"Scale) is based on the transform axis");
if (reg) {
RNA_def_property_flag(prop, PROP_REGISTER_OPTIONAL);
}
}
/* --- */