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

Issue

When moving keys in the Graph Editor you usually only want to move them on one axis.
While this is possible in a few ways (G+X, or G + Middle Mouse Button click), we could default the behavior to always lock on an axis.
This was suggested by Dreamworks animators during the Animation & Rigging module meeting.

Solution

This PR adds an option with which the movement is always locked to a single axis by default.
The option can be found in the Graph Editor under "View->Auto-Lock Axis".
image

The movement will then be restricted to the axis along which you've moved the cursor the most.
You can still manually override the lock behavior by pressing X or Y.

Build
https://builder.blender.org/download/patch/PR117669/

Open User Questions

  • I've made it only affect translation since rotation and scale seemed very counter productive to lock down. Needs confirmation that's ok.
  • The option is part of the Graph Editor, meaning when opening a new file or a new Graph Editor, the option will be reset to the default.
  • In my testing, for small tweaks it's still easier to manually lock the axis since it tends to jump when close to the point where the translation has been initiated.

Technical implementation

This was a bit easier than expected.
I am piggybacking off the auto locking feature you get when pressing the middle mouse button.
When the new feature is enabled I call that at the start of the transformation.
Except when:

  • only handles are selected
  • the tweak mode has been started on a handle
# Issue When moving keys in the Graph Editor you usually only want to move them on one axis. While this is possible in a few ways (G+X, or G + Middle Mouse Button click), we could default the behavior to always lock on an axis. This was suggested by Dreamworks animators during the [Animation & Rigging](https://devtalk.blender.org/t/2024-01-26-animation-rigging-module-meeting/33081#patch-review-decision-time-5) module meeting. # Solution This PR adds an option with which the movement is always locked to a single axis by default. The option can be found in the Graph Editor under "View->Auto-Lock Axis". ![image](/attachments/028d2f82-b4fc-443f-8cdf-a54f0916d4a7) The movement will then be restricted to the axis along which you've moved the cursor the most. You can still manually override the lock behavior by pressing `X` or `Y`. **Build** https://builder.blender.org/download/patch/PR117669/ # Open User Questions * I've made it only affect translation since rotation and scale seemed very counter productive to lock down. Needs confirmation that's ok. * The option is part of the Graph Editor, meaning when opening a new file or a new Graph Editor, the option will be reset to the default. * In my testing, for small tweaks it's still easier to manually lock the axis since it tends to jump when close to the point where the translation has been initiated. # Technical implementation This was a bit easier than expected. I am piggybacking off the auto locking feature you get when pressing the middle mouse button. When the new feature is enabled I call that at the start of the transformation. Except when: * only handles are selected * the tweak mode has been started on a handle
Christoph Lendenfeld added the
Module
Animation & Rigging
label 2024-01-30 16:44:24 +01:00
Christoph Lendenfeld added 1 commit 2024-01-30 16:44:37 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
94783ec04f
rough implementation
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR117669) when ready.
Contributor

This doesnt belong to View menu though, its not related to viewing stuff it actually changes workflow. It belongs to Key menu imho since it affects how keys are transformed, and Transform menu is there.

This doesnt belong to View menu though, its not related to viewing stuff it actually changes workflow. It belongs to Key menu imho since it affects how keys are transformed, and Transform menu is there.
Member

scale seemed very counter productive to lock down

I'm not an animator so you can ignore me, but I'd be curious what the original proposers of this functionality think. If the goal is to reduce the X/Y keypresses after G/S, then doing that for scaling sounds equally valid/useful to me as for translation. But if you tried it out and it didn't feel good then ohwell.

> scale seemed very counter productive to lock down I'm not an animator so you can ignore me, but I'd be curious what the original proposers of this functionality think. If the goal is to reduce the X/Y keypresses after G/S, then doing that for scaling sounds equally valid/useful to me as for translation. But if you tried it out and it didn't feel good then ohwell.
Member

I'd be curious what the original proposers of this functionality think.

We should certainly get their feedback, yeah. However, based on the discussion in the module meeting that led to this, I suspect only doing it for the keys themselves is consistent with what they were asking for.

The idea is that in practice animators want to modify either the value or the time of a key frame, but rarely (if ever) both at once. That doesn't have anything to do with key frame handles, however, since they only affect the interpolation between keys, not the keys themselves. (Handles do affect the timing and values of the animation, of course, but not in a way that can be cleanly separated into their horizontal and vertical positions.)

> I'd be curious what the original proposers of this functionality think. We should certainly get their feedback, yeah. However, based on the discussion in the module meeting that led to this, I suspect only doing it for the keys themselves is consistent with what they were asking for. The idea is that in practice animators want to modify either the value *or* the time of a key frame, but rarely (if ever) both at once. That doesn't have anything to do with key frame *handles*, however, since they only affect the interpolation between keys, not the keys themselves. (Handles do affect the timing and values of the animation, of course, but not in a way that can be cleanly separated into their horizontal and vertical positions.)
Author
Member

The idea is that in practice animators want to modify either the value or the time of a key frame, but rarely (if ever) both at once. That doesn't have anything to do with key frame handles, however, since they only affect the interpolation between keys, not the keys themselves. (Handles do affect the timing and values of the animation, of course, but not in a way that can be cleanly separated into their horizontal and vertical positions.)

The PR currently locks it for both, that's a point I missed. Will see if I can get it to run only on keys.

> The idea is that in practice animators want to modify either the value *or* the time of a key frame, but rarely (if ever) both at once. That doesn't have anything to do with key frame *handles*, however, since they only affect the interpolation between keys, not the keys themselves. (Handles do affect the timing and values of the animation, of course, but not in a way that can be cleanly separated into their horizontal and vertical positions.) The PR currently locks it for both, that's a point I missed. Will see if I can get it to run only on keys.

based on the discussion in the module meeting that led to this, I suspect only doing it for the keys themselves is consistent with what they were asking for.

Ares explicitly mentioned that his approach with the Blender keymap also does this for the handles, and that this was something unwanted (but unavoidable for him given how the keymap works).

> based on the discussion in the module meeting that led to this, I suspect only doing it for the keys themselves is consistent with what they were asking for. Ares explicitly mentioned that his approach with the Blender keymap also does this for the handles, and that this was something unwanted (but unavoidable for him given how the keymap works).
Sybren A. Stüvel reviewed 2024-02-01 16:22:37 +01:00
@ -969,1 +969,4 @@
static bool editor_forces_constraints(TransInfo *t)
{
switch (t->spacetype) {

I don't know much about the transform system. Would it be possible to query this from whatever space it's running in? Or as some option that the invoking code can pass? I think that would allow for some more separation, compared to hard-coding space-dependent code here.

I don't know much about the transform system. Would it be possible to query this from whatever space it's running in? Or as some option that the invoking code can pass? I think that would allow for some more separation, compared to hard-coding space-dependent code here.
Author
Member

good point. All the code has been moved to transform_convert_graph.cc

good point. All the code has been moved to `transform_convert_graph.cc`

Just gave this a try, and I really like it on the selected keys - totally intuitive and a great way of working!

However, it seems to break things pretty quickly when you use the handles. Is it possible to not have this work on handles, but only on keys?

Thanks! :)

Just gave this a try, and I really like it on the selected keys - totally intuitive and a great way of working! However, it seems to break things pretty quickly when you use the handles. Is it possible to not have this work on handles, but only on keys? Thanks! :)
Christoph Lendenfeld added this to the 4.2 LTS milestone 2024-02-01 18:45:57 +01:00
Christoph Lendenfeld added 1 commit 2024-02-02 15:26:53 +01:00
Christoph Lendenfeld added 1 commit 2024-02-02 17:29:18 +01:00
Christoph Lendenfeld added 1 commit 2024-02-02 17:31:21 +01:00
buildbot/vexp-code-patch-lint Build done. Details
buildbot/vexp-code-patch-darwin-arm64 Build done. Details
buildbot/vexp-code-patch-darwin-x86_64 Build done. Details
buildbot/vexp-code-patch-linux-x86_64 Build done. Details
buildbot/vexp-code-patch-windows-amd64 Build done. Details
buildbot/vexp-code-patch-coordinator Build done. Details
533d3d2bf2
update description
Author
Member

@blender-bot package

I've changed it so it only affects keys and not handles.

@blender-bot package I've changed it so it only affects keys and not handles.
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR117669) when ready.
Nathan Vegdahl requested changes 2024-02-05 11:04:32 +01:00
Nathan Vegdahl left a comment
Member

Just some minor nits, but looks good to me (pending approval of functionality from users).

Just some minor nits, but looks good to me (pending approval of functionality from users).
@ -148,0 +152,4 @@
return;
}
/* Those flags are set when using tweak mode on handles. */
Member

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.
Author
Member

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.
Member

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!
nathanvegdahl marked this conversation as resolved
@ -459,6 +478,7 @@ static void createTransGraphEditData(bContext *C, TransInfo *t)
}
}
else {
Member

Stray added blank line.

Stray added blank line.
nathanvegdahl marked this conversation as resolved
Sybren A. Stüvel requested changes 2024-02-05 11:15:26 +01:00
Sybren A. Stüvel left a comment
Member

Found some smaller things.

Found some smaller things.
@ -147,1 +148,4 @@
static void enable_autolock(TransInfo *t, SpaceGraph *space_graph)
{
if (t->mode != TFM_TRANSLATION) {

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.
@ -6425,0 +6427,4 @@
RNA_def_property_ui_text(prop,
"Auto-Lock Key Axis",
"Automatically locks the movement of keyframes to the dominant axis. "
"Ignores keyframe handles");

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.
Author
Member

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

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:
Christoph Lendenfeld added 2 commits 2024-02-06 10:02:20 +01:00
Christoph Lendenfeld added 1 commit 2024-02-06 10:03:45 +01:00
Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-02-06 10:04:16 +01:00
Christoph Lendenfeld requested review from Nathan Vegdahl 2024-02-06 10:04:19 +01:00
Sybren A. Stüvel approved these changes 2024-02-06 17:47:39 +01:00
Sybren A. Stüvel left a comment
Member

LGTM, with a few things that might be interesting to address after landing this PR.

LGTM, with a few things that might be interesting to address after landing this PR.
Christoph Lendenfeld added 1 commit 2024-02-06 23:21:09 +01:00
Christoph Lendenfeld merged commit 446b92d2ce into main 2024-02-06 23:31:19 +01:00
Christoph Lendenfeld deleted branch graph_editor_auto_lock_transform 2024-02-06 23:31:21 +01:00
First-time contributor

It would be nice to have double G press (GG) assigned to temporal axis restriction eliminating

It would be nice to have double G press (GG) assigned to temporal axis restriction eliminating
Author
Member

It would be nice to have double G press (GG) assigned to temporal axis restriction eliminating

you can disable the setting while in the modal operator with the middle mouse button. To stay consistent with the 3D viewport I think reusing GG here isn't a good idea.

> It would be nice to have double G press (GG) assigned to temporal axis restriction eliminating you can disable the setting while in the modal operator with the middle mouse button. To stay consistent with the 3D viewport I think reusing GG here isn't a good idea.

To stay consistent with the 3D viewport I think reusing GG here isn't a good idea.

I agree. For context to people who may not know (because Nathan had to tell me about it too): in mesh edit mode, G moves a vertex freely, and pressing G again changes the behaviour to 'vertex slide' along the edges.

Having GG slide a key along the F-Curve while keeping the curve itself as much as-is as possible, that would be sweet, but it's a whole other thing than this PR :)

> To stay consistent with the 3D viewport I think reusing GG here isn't a good idea. I agree. For context to people who may not know (because Nathan had to tell me about it too): in mesh edit mode, G moves a vertex freely, and pressing G again changes the behaviour to 'vertex slide' along the edges. Having GG slide a key along the F-Curve while keeping the curve itself as much as-is as possible, that would be sweet, but it's a whole other thing than this PR :)
First-time contributor

For example in object mode RR already works like that - first R starts viewport restricted rotation and second R disables restriction, all the next R pressings toggles the restriction.
Quite common behaviour, feels pretty nice, doesnot cause mindset contradiction with vertex sliding even if it works in reverse - since in both cases the initial behaviour is the most relevant one.

In short, Blenders doublekey approach is not about predefined restricting order, but about relevancy order.

For example in object mode RR already works like that - first R starts viewport restricted rotation and second R disables restriction, all the next R pressings toggles the restriction. Quite common behaviour, feels pretty nice, doesnot cause mindset contradiction with vertex sliding even if it works in reverse - since in both cases the initial behaviour is the most relevant one. In short, Blenders doublekey approach is not about predefined restricting order, but about relevancy order.
Author
Member

this was discussed in the recent Animation & Rigging module meeting.
The conclusion was that GG is something we will implement.
It will function like a toggle, so if the setting is enabled pressing GG will disable it and the other way around.

this was discussed in the recent [Animation & Rigging](https://devtalk.blender.org/t/2024-02-13-animation-rigging-module-meeting/33400#patches-review-decision-time-4) module meeting. The conclusion was that GG is something we will implement. It will function like a toggle, so if the setting is enabled pressing GG will disable it and the other way around.
First-time contributor

I agree that toggle it is the most worthful behaviour.
For example, the same way holding ctrl temporarily enables snapping if it is disabled and vice versa, disables when enabled - providing an additional flexibility degree which was confirmed to be useful in practice.

GG toggle is a nice solution that will provide similar flexibility without blocking numeric input in both scenaries (when you need to move keys at specific distance along some axis)

I agree that toggle it is the most worthful behaviour. For example, the same way holding ctrl temporarily enables snapping if it is disabled and vice versa, disables when enabled - providing an additional flexibility degree which was confirmed to be useful in practice. GG toggle is a nice solution that will provide similar flexibility without blocking numeric input in both scenaries (when you need to move keys at specific distance along some axis)
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
8 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#117669
No description provided.