Animation: Graph Editor Handle Selection #111143
No reviewers
Sybren A. Stüvel
Christoph Lendenfeld
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Code Documentation
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
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
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#111143
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "nathanvegdahl/blender:select_handles_patch"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Continuation of PR #108142 from @cgtinker.
This patch adds an operator for the graph editor,
graph.select_key_handles,that changes the selection of the different parts of a bezier keyframe. It
operates on all keys that are either themselves selected or have either of
their handles selected, and changes whether the key itself and/or its handles
are selected.
The operator has three options:
left_handle_actionright_handle_actionkey_actionEach of which can be set to:
Co-authored-by: cgtinker Denys.Hsu@gmail.com
I think the operator parameters could be made both a little clearer and more flexible with this approach:
Three parameters:
left_handleright_handlekeyframeEach of which can be "select", "deselect", or "leave alone". (Better naming for the last one welcome!)
What do people think?
@nathanvegdahl the current state of the code looks good to me
But I agree the parameters should be broken up into 3 separate parameters
the last option I suggest is "keep"
Some minor remarks. Can be adjusted when landing.
@ -2067,0 +2126,4 @@* Select keyframe handles left/right/both or keys based on the given selection.** mode: Determines which handles to select - left, right, or both.* deselect_keyframe: Specifies whether keyframes should be deselected (true) or if the currentSuggestion: "whether keyframes should" → "whether the keys themselves should".
@ -2067,0 +2148,4 @@continue;}switch (mode) {case GRAPHKEYS_HANDLESEL_BOTH: {Remove the curly braces, they are only necessary when introducing new variables inside the
case.@ -2067,0 +2161,4 @@break;}case GRAPHKEYS_HANDLESEL_KEY: {ANIM_fcurve_keyframes_loop(NULL, fcu, NULL, bezt_sel_keys, NULL);Missing
break@ -2067,0 +2185,4 @@const eGraphKey_HandleSelect_Side leftright = static_cast<eGraphKey_HandleSelect_Side>(RNA_enum_get(op->ptr, "mode"));const bool deselect_keyframe = RNA_boolean_get(op->ptr, "deselect_keyframe");/* Perform selection changes. */You don't need to explain what the function does at the call site of the function. If it needs explanation, choose a better function name.
@ -2067,0 +2199,4 @@uiLayout *layout = op->layout;uiLayout *row;bool keyframe_selection = RNA_enum_get(op->ptr, "mode") == GRAPHKEYS_HANDLESEL_KEY;const, and move this line down to wherekeyframe_selectionis actually used.@ -2067,0 +2200,4 @@uiLayout *row;bool keyframe_selection = RNA_enum_get(op->ptr, "mode") == GRAPHKEYS_HANDLESEL_KEY;row = uiLayoutRow(layout, false);Declare
rowhere, and not at the start of the function. The days of AncientC are over.@ -2067,0 +2230,4 @@GRAPHKEYS_HANDLESEL_BOTH,"Mode","Selects corresponding keyframe handles (left, right, both or keys) based on the given ""selection");"Selects ... based on the given selection" is not the clearest explanation. Suggestion:
Since the functionality has become a bit more flexible, I changed the description. Would love to have your eyes on it again.
5c7fe5ed49to47e50a8383I've updated the PR to the new options I outlined earlier.
Now that I've done that, however, it's not clear to me that the current operator name is appropriate anymore, given its additional flexibility. Maybe something like "Select Keyframe Parts"? @ChrisLend @dr.sybren Any ideas?
(The menu entries can still be labeled with what they each do, though, I think. "Select Handles", "Select Left Handles", etc.)
Brought naming of the operator up at the module meeting, and consensus was "Select Key / Handles".
I changed the operator name. Still not happy with the tool tips. I'll work on improving those next.
967f92f89bto5c5be1b8b5@ -220,0 +229,4 @@props = layout.operator("graph.select_key_handles", text="Select Handles Right")props.left_handle_action = 'DESELECT'props.right_handle_action = 'SELECT'props.key_action = 'KEEP'I think this should be 'DESELECT' because right now using this option you still have the key selected, meaning when using G to move it you move the key and not the handle.
Good call.
But now that I think of it, do we even need the left/right versions in there at all? The primary use case is quickly selecting/deselecting both handles together. It's still good for the core operator to be flexible, so users with more niche use cases can do what they like. But for the provided menu items it might be best to just have "Select Handles" and "Select Keys".
Thoughts?
In the context of this issue it makes sense to only have "Select Handles" and "Select Keys"
When talking about animation in general it could make sense to have a shortcut to only select handles on one side. But since the functionality exists it is reasonably easy to expose in a GUI. Maybe even a pie menu. But we can leave that for the future I think
Yeah. I also checked with one of the animators here at the studio, and he said he would use Select Handles and Select Keys, but couldn't think of a common use for left/right. Of course, that's just a sample size of one. But I think it makes sense to keep things minimal now, and like you said we can always add more things and explore other ways to expose this in the future.
yep looks good to me :)
@ -2067,0 +2109,4 @@int i = 0;BezTriple *bezt = fcu->bezt;for (; i < fcu->totvert; bezt++, i++) {Personally I am not a fan of pointer incrementing. But I will leave this up to you
Ah, fair point. I basically just copy-pasted this from elsewhere. I agree that just using the index directly makes more sense here. Thanks for the catch!