blender
/
blender
136
407
Fork 589
Code Issues 6.3k Pull Requests 770 Projects 19 Wiki Activity

Anim: Graph Editor - automatically lock key translation to a single axis #117669

Merged
Christoph Lendenfeld merged 8 commits from ChrisLend/blender:graph_editor_auto_lock_transform into main 2024-02-06 23:31:19 +01:00
Conversation 16 Commits 8 Files Changed 4 +36 -2
4 changed files with 36 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
scripts/startup/bl_ui/space_graph.py
View File

@ -159,6 +159,7 @@ class GRAPH_MT_view(Menu):
layout.prop(st, "use_realtime_update")
layout.prop(st, "show_sliders")
layout.prop(st, "use_auto_merge_keyframes")
layout.prop(st, "autolock_translation_axis")
layout.separator()
if st.mode != 'DRIVERS':

27
source/blender/editors/transform/transform_convert_graph.cc
View File

@ -27,6 +27,7 @@
#include "UI_view2d.hh"
#include "transform.hh"
#include "transform_constraints.hh"
#include "transform_convert.hh"
#include "transform_mode.hh"
#include "transform_snap.hh"
@ -145,6 +146,24 @@ static bool graph_edit_use_local_center(TransInfo *t)
return ((t->around == V3D_AROUND_LOCAL_ORIGINS) && (graph_edit_is_translation_mode(t) == false));
}
static void enable_autolock(TransInfo *t, SpaceGraph *space_graph)
{
/* Locking the axis makes most sense for translation. We may want to enable it for scaling as
Sybren A. Stüvel commented 2024-02-05 11:10:51 +01:00
Outdated
Review
Copy Link

It might be good to add a comment to explain why this check is done. Right now we know that this was done because we feel autolock doesn't work well for rotating or scaling, but I will definitely forget about that soon. Also my gut feeling says that it could actually work nice for scaling (adjusting only timing or values, but not both, seems nice to me), and having this motivation written down will help making such adjustments in the future.

It might be good to add a comment to explain why this check is done. Right now we know that this was done because we feel autolock doesn't work well for rotating or scaling, but I will definitely forget about that soon. Also my gut feeling says that it _could_ actually work nice for scaling (adjusting only timing or values, but not both, seems nice to me), and having this motivation written down will help making such adjustments in the future.
👍 1
* well if artists require that. */
if (t->mode != TFM_TRANSLATION) {
return;
}
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl commented 2024-02-05 10:47:12 +01:00
Review
Copy Link

Language nit: "Those" -> "These".

Also, are these checks necessary, since there's now the only_handles_selected check in graph_bezt_get_transform_selection? I might be missing something, but I think this is redundant.

Language nit: "Those" -> "These". Also, are these checks necessary, since there's now the `only_handles_selected` check in `graph_bezt_get_transform_selection`? I might be missing something, but I think this is redundant.
Christoph Lendenfeld commented 2024-02-06 09:47:50 +01:00
Review
Copy Link

Yes because when using tweak mode directly on a handle, selected keys get ignored.

Yes because when using tweak mode directly on a handle, selected keys get ignored.
Nathan Vegdahl commented 2024-02-06 16:09:47 +01:00
Review
Copy Link

Oh, got it. In my initial playing around with tweaking, when I tweaked the handle everything else got deselected anyway. But apparently that only happens when the handle you're tweaking wasn't itself already selected. if it was already selected, the rest of the selection (including keys) is also kept. So indeed you're totally right.

Thanks for the explanation!

Oh, got it. In my initial playing around with tweaking, when I tweaked the handle everything else got deselected anyway. But apparently that only happens when the handle you're tweaking wasn't itself already selected. if it was already selected, the rest of the selection (including keys) is also kept. So indeed you're totally right. Thanks for the explanation!
/* These flags are set when using tweak mode on handles. */
if ((space_graph->runtime.flag & SIPO_RUNTIME_FLAG_TWEAK_HANDLES_LEFT) ||
(space_graph->runtime.flag & SIPO_RUNTIME_FLAG_TWEAK_HANDLES_RIGHT))
{
return;
}
initSelectConstraint(t);
}
/**
* Get the effective selection of a triple for transform, i.e. return if the left handle, right
* handle and/or the center point should be affected by transform.
@ -367,6 +386,8 @@ static void createTransGraphEditData(bContext *C, TransInfo *t)
}
}
bool at_least_one_key_selected = false;
/* loop 2: build transdata arrays */
LISTBASE_FOREACH (bAnimListElem *, ale, &anim_data) {
AnimData *adt = ANIM_nla_mapping_get(&ac, ale);
@ -405,7 +426,7 @@ static void createTransGraphEditData(bContext *C, TransInfo *t)
TransDataCurveHandleFlags *hdata = nullptr;
graph_bezt_get_transform_selection(t, bezt, use_handle, &sel_left, &sel_key, &sel_right);
at_least_one_key_selected |= sel_key;
if (is_prop_edit) {
bool is_sel = (sel_key || sel_left || sel_right);
/* we always select all handles for proportional editing if central handle is selected */
@ -616,6 +637,10 @@ static void createTransGraphEditData(bContext *C, TransInfo *t)
}
}
if (sipo->flag & SIPO_AUTOLOCK_AXIS && at_least_one_key_selected) {
enable_autolock(t, sipo);
}
/* cleanup temp list */
ANIM_animdata_freelist(&anim_data);
}

3
source/blender/makesdna/DNA_space_types.h
View File

@ -509,7 +509,8 @@ typedef enum eGraphEdit_Flag {
SIPO_NOTRANSKEYCULL = (1 << 1),
/* don't show any keyframe handles at all */
SIPO_NOHANDLES = (1 << 2),
/* SIPO_NODRAWCFRANUM = (1 << 3), DEPRECATED */
/* Automatically lock the transform to whichever axis the cursor has moved the most. */
SIPO_AUTOLOCK_AXIS = (1 << 3),
/* show timing in seconds instead of frames */
SIPO_DRAWTIME = (1 << 4),
/* draw names of F-Curves beside the respective curves */

7
source/blender/makesrna/intern/rna_space.cc
View File

@ -6422,6 +6422,13 @@ static void rna_def_space_graph(BlenderRNA *brna)
RNA_def_property_ui_text(prop, "Show Handles", "Show handles of Bézier control points");
RNA_def_property_update(prop, NC_SPACE | ND_SPACE_GRAPH, nullptr);
prop = RNA_def_property(srna, "autolock_translation_axis", PROP_BOOLEAN, PROP_NONE);
RNA_def_property_boolean_sdna(prop, nullptr, "flag", SIPO_AUTOLOCK_AXIS);
RNA_def_property_ui_text(prop,
"Auto-Lock Key Axis",
"Automatically locks the movement of keyframes to the dominant axis");
RNA_def_property_update(prop, NC_SPACE | ND_SPACE_GRAPH, nullptr);
Sybren A. Stüvel commented 2024-02-05 11:15:06 +01:00
Outdated
Review
Copy Link

Either I don't quite understand the code, or this tooltip is incorrect.

The code seems to only activate auto-locking when there are no handles selected. So if there is any handle selected, auto-locking is disabled. This is rather the opposite of ignoring keyframe handles -- it doesn't just ignore them, it runs away and hides in a corner if any of them are selected.

What I'd expect from the tooltip is that you can have handles selected, but it would just not apply the auto-locking to them.

Note that I'm not arguing for any specific behaviour here. I'm just pointing out that the description of the functionality doesn't seem to match the behaviour I understand from the code changes in this PR.

Either I don't quite understand the code, or this tooltip is incorrect. The code seems to only activate auto-locking when there are *no* handles selected. So if there is any handle selected, auto-locking is disabled. This is rather the opposite of ignoring keyframe handles -- it doesn't just ignore them, it runs away and hides in a corner if any of them are selected. What I'd expect from the tooltip is that you can have handles selected, but it would just not apply the auto-locking to them. Note that I'm not arguing for any specific behaviour here. I'm just pointing out that the description of the functionality doesn't seem to match the behaviour I understand from the code changes in this PR.
Christoph Lendenfeld commented 2024-02-06 10:01:43 +01:00
Outdated
Review
Copy Link

So similar to my comment to @nathanvegdahl, when starting tweak mode on a handle auto locking is disabled even though there might be keys selected. This is because tweak mode in that case only moves the handles.

Also it's not quite correct that the code only activates when there are no handles selected. It only activates if the selection has at least one keyframe.

I am unsure what to do with this tooltip though. My gut reaction was keyframe handles are not affected. But that's not true, there is the case of having one handle and a key (from a different keyframe) selected. This will still snap the handle

Edit: I inverted the boolean variable to now read at_least_one_key_selected. That should hopefully clear things up a bit

So similar to my comment to @nathanvegdahl, when starting tweak mode on a handle auto locking is disabled even though there might be keys selected. This is because tweak mode in that case only moves the handles. Also it's not quite correct that the code only activates when there are *no* handles selected. It only activates if the selection has at least *one* keyframe. I am unsure what to do with this tooltip though. My gut reaction was `keyframe handles are not affected`. But that's not true, there is the case of having one handle and a key (from a different keyframe) selected. This will still snap the handle Edit: I inverted the boolean variable to now read `at_least_one_key_selected`. That should hopefully clear things up a bit
Sybren A. Stüvel commented 2024-02-06 17:45:43 +01:00
Outdated
Review
Copy Link

when starting tweak mode on a handle auto locking is disabled even though there might be keys selected. This is because tweak mode in that case only moves the handles.

I think for tweaking it works well now. For regular translation (G key), there's a little nag though. From what I could gather, it looks like it's now dependent on selection order. If the last thing I selected was a handle, pressing 'G' and moving things around will not auto-lock. If, however, I deselect and reselect the key itself, resulting in visually the same selection state, all of a sudden the auto-locking works again. I think it's fine to land the patch as-is, and deal with this as a polish commit in bcon3.

I am unsure what to do with this tooltip though. My gut reaction was keyframe handles are not affected. But that's not true, there is the case of having one handle and a key (from a different keyframe) selected. This will still snap the handle

Maybe it's better to not mention handles in the tooltip, then. Handles are not keyframes, and given that the tooltip only talks about keyframes, it might be clear enough. In any case, if it turns out to need further clarification, that can always happen in bcon3.

Edit: I inverted the boolean variable to now read at_least_one_key_selected. That should hopefully clear things up a bit

👍

> when starting tweak mode on a handle auto locking is disabled even though there might be keys selected. This is because tweak mode in that case only moves the handles. I think for tweaking it works well now. For regular translation (G key), there's a little nag though. From what I could gather, it looks like it's now dependent on selection order. If the last thing I selected was a handle, pressing 'G' and moving things around will *not* auto-lock. If, however, I deselect and reselect the key itself, resulting in visually the same selection state, all of a sudden the auto-locking works again. I think it's fine to land the patch as-is, and deal with this as a polish commit in bcon3. > I am unsure what to do with this tooltip though. My gut reaction was `keyframe handles are not affected`. But that's not true, there is the case of having one handle and a key (from a different keyframe) selected. This will still snap the handle Maybe it's better to not mention handles in the tooltip, then. Handles are not keyframes, and given that the tooltip only talks about keyframes, it might be clear enough. In any case, if it turns out to need further clarification, that can always happen in bcon3. > Edit: I inverted the boolean variable to now read `at_least_one_key_selected`. That should hopefully clear things up a bit :+1:
prop = RNA_def_property(srna, "use_only_selected_keyframe_handles", PROP_BOOLEAN, PROP_NONE);
RNA_def_property_boolean_sdna(prop, nullptr, "flag", SIPO_SELVHANDLESONLY);
RNA_def_property_ui_text(