Animation: shear left slider #106521

Closed
AresDeveaux wants to merge 9 commits from AresDeveaux/blender:shear_left_slider into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
First-time contributor

New operator for the slider tool in the Graph Editor.

It affects the value of selected keys linearly keeping the same relationship between them using the left key as anchor. The purpose is to keep the animation on the selected keys but matching the neigboring poses.

The tool I have used that inspired this slider is a lattice in the graph editor. So, imagine you have a simple lattice without subdivision affecting the selected keys (a bounding box), and then push the right side of the lattice up or down.

It uses the structure already created by @ChrisLend. The main functionality change is in 'keyframes_general.c', the rest for the most part just follows what he did.

New operator for the slider tool in the Graph Editor. It affects the value of selected keys linearly keeping the same relationship between them using the left key as anchor. The purpose is to keep the animation on the selected keys but matching the neigboring poses. The tool I have used that inspired this slider is a lattice in the graph editor. So, imagine you have a simple lattice without subdivision affecting the selected keys (a bounding box), and then push the right side of the lattice up or down. It uses the structure already created by @ChrisLend. The main functionality change is in 'keyframes_general.c', the rest for the most part just follows what he did.
AresDeveaux added 3 commits 2023-04-04 06:15:14 +02:00
Nate Rupsis added the
Module
Animation & Rigging
label 2023-04-06 20:59:40 +02:00
Nate Rupsis added this to the Animation & Rigging project 2023-04-06 20:59:52 +02:00
AresDeveaux changed title from WIP: Animation&Rigging: shear_left_slider to Animation: shear left slider 2023-04-10 05:35:34 +02:00
AresDeveaux added 1 commit 2023-04-11 01:44:46 +02:00
AresDeveaux added 1 commit 2023-04-11 01:52:10 +02:00
Christoph Lendenfeld approved these changes 2023-04-14 17:43:34 +02:00
Christoph Lendenfeld left a comment
Member

I see nothing wrong with it except a small typo :)
adding sybren as a reviewer for the final check

I see nothing wrong with it except a small typo :) adding sybren as a reviewer for the final check
@ -1063,0 +1163,4 @@
ot->name = "Shear Left Keyframes";
ot->idname = "GRAPH_OT_shear_left";
ot->description =
"Affects the value of the keys linealy keeping the same \n\

I assume you mean "linearly"?

I assume you mean "linearly"?
Christoph Lendenfeld requested review from Sybren A. Stüvel 2023-04-14 17:43:44 +02:00
Author
First-time contributor

I see you approved it, did you change the typo? don't know if I should publish the error correction and ask for a new review.

I see you approved it, did you change the typo? don't know if I should publish the error correction and ask for a new review.
AresDeveaux added 1 commit 2023-04-14 21:35:32 +02:00
Author
First-time contributor

Well, I pushed the typo correction just in case.

Well, I pushed the typo correction just in case.

I see you approved it, did you change the typo? don't know if I should publish the error correction and ask for a new review.

I didn't change anything, but if the changes are so super tiny, the way it is usually handled is that the reviewer accepts it with the note it should be changed. And right after the change it can be commited because it's already accepted :)

In this case wait for Sybren to look over it though

> I see you approved it, did you change the typo? don't know if I should publish the error correction and ask for a new review. I didn't change anything, but if the changes are so super tiny, the way it is usually handled is that the reviewer accepts it with the note it should be changed. And right after the change it can be commited because it's already accepted :) In this case wait for Sybren to look over it though
Sybren A. Stüvel reviewed 2023-04-17 14:48:41 +02:00
Sybren A. Stüvel left a comment
Member

What are the future plans for this operator? Would it ever need to be able to sheer the other side as well?

  /* The factor goes from 0 to 1, but for this tool it needs to go from -1 to 1. */

Would it make sense to turn this into a bidirectional slider? That way the slider itself also goes from -100% to +100%, with a neutral 0% starting point (instead of 50%).

I'm also not 100% convinced by the "sheer left", but that might be a personal thing. Since it's the right-hand side of the selection area that is affected the most, wouldn't this be more of a "sheer right"?

What are the future plans for this operator? Would it ever need to be able to sheer the other side as well? ``` /* The factor goes from 0 to 1, but for this tool it needs to go from -1 to 1. */ ``` Would it make sense to turn this into a bidirectional slider? That way the slider itself also goes from -100% to +100%, with a neutral 0% starting point (instead of 50%). I'm also not 100% convinced by the "sheer left", but that might be a personal thing. Since it's the right-hand side of the selection area that is affected the most, wouldn't this be more of a "sheer right"?
Author
First-time contributor
What are the future plans for this operator? Would it ever need to be able to sheer the other side as well?
Would it make sense to turn this into a bidirectional slider? That way the slider itself also goes from -100% to +100%, with a neutral 0% starting point (instead of 50%).

I changed it to bidirectional and it worked. I need to revisit other sliders because I did this on some but didn't do what I was expecting. Must be something wrong I did.

I'm also not 100% convinced by the "sheer left", but that might be a personal thing. Since it's the right-hand side of the selection area that is affected the most, wouldn't this be more of a "sheer right"?

That is my instinct too, but every tool on the slider uses left or right based on where the reference point is. For example Scale left feels the same as this one, scale toward the right side of the selected keys because uses the left neighboring pose as a reference. The naming convention came from how it is in similar tools in the industry but let me know if I should change it.

``` What are the future plans for this operator? Would it ever need to be able to sheer the other side as well? Would it make sense to turn this into a bidirectional slider? That way the slider itself also goes from -100% to +100%, with a neutral 0% starting point (instead of 50%). ``` I changed it to bidirectional and it worked. I need to revisit other sliders because I did this on some but didn't do what I was expecting. Must be something wrong I did. ``` I'm also not 100% convinced by the "sheer left", but that might be a personal thing. Since it's the right-hand side of the selection area that is affected the most, wouldn't this be more of a "sheer right"? ``` That is my instinct too, but every tool on the slider uses `left` or `right` based on where the reference point is. For example `Scale left` feels the same as this one, scale toward the right side of the selected keys because uses the left neighboring pose as a reference. The naming convention came from how it is in similar tools in the industry but let me know if I should change it.
AresDeveaux added 1 commit 2023-04-18 05:31:51 +02:00
AresDeveaux requested review from Christoph Lendenfeld 2023-04-18 05:35:48 +02:00

That is my instinct too, but every tool on the slider uses left or right based on where the reference point is. For example Scale left feels the same as this one, scale toward the right side of the selected keys because uses the left neighboring pose as a reference. The naming convention came from how it is in similar tools in the industry but let me know if I should change it.

I think this should be discussed in the A&R meeting, there is no clear right or wrong to it

> That is my instinct too, but every tool on the slider uses `left` or `right` based on where the reference point is. For example `Scale left` feels the same as this one, scale toward the right side of the selected keys because uses the left neighboring pose as a reference. The naming convention came from how it is in similar tools in the industry but let me know if I should change it. > I think this should be discussed in the A&R meeting, there is no clear right or wrong to it

I changed it to bidirectional and it worked. I need to revisit other sliders because I did this on some but didn't do what I was expecting. Must be something wrong I did.

It should expand the slider's range from [0, 1] to [-1, 1], nothing more. Of course the code has to change as well, since the 'neutral' point changes from 0.5 to 0.0 and the range of the sider becomes twice a large.

That is my instinct too, but every tool on the slider uses left or right based on where the reference point is. For example Scale left feels the same as this one, scale toward the right side of the selected keys because uses the left neighboring pose as a reference. The naming convention came from how it is in similar tools in the industry but let me know if I should change it.

I've put it on the module meeting agenda as @ChrisLend suggested 👍

Also, if there is going to be a "shear right" operator as well, I think the code would need some changes to avoid future code duplication. Some of the functions would need to be renamed to no longer have left in their name (like shear_left_fcurve_segment). The operator can get an extra parameter to indicate the left/right side (probably a two-value enum is best), which is then passed down to the shearing functions.

@AresDeveaux would you need/want some help with this restructuring?

> I changed it to bidirectional and it worked. I need to revisit other sliders because I did this on some but didn't do what I was expecting. Must be something wrong I did. It should expand the slider's range from `[0, 1]` to `[-1, 1]`, nothing more. Of course the code has to change as well, since the 'neutral' point changes from `0.5` to `0.0` and the range of the sider becomes twice a large. > That is my instinct too, but every tool on the slider uses `left` or `right` based on where the reference point is. For example `Scale left` feels the same as this one, scale toward the right side of the selected keys because uses the left neighboring pose as a reference. The naming convention came from how it is in similar tools in the industry but let me know if I should change it. I've put it on the module meeting agenda as @ChrisLend suggested :+1: Also, if there is going to be a "shear right" operator as well, I think the code would need some changes to avoid future code duplication. Some of the functions would need to be renamed to no longer have `left` in their name (like `shear_left_fcurve_segment`). The operator can get an extra parameter to indicate the left/right side (probably a two-value enum is best), which is then passed down to the shearing functions. @AresDeveaux would you need/want some help with this restructuring?
Christoph Lendenfeld approved these changes 2023-04-20 16:33:32 +02:00
Christoph Lendenfeld left a comment
Member

small thing about the factor soft minimum
no need to re-review from my side after that change

small thing about the factor soft minimum no need to re-review from my side after that change
@ -1063,0 +1184,4 @@
FLT_MAX,
"Curve Bend",
"Control the bend of the curve",
0.0f,

tiny thing I noticed, the soft minimum should be -1 if the slider has a range of -1/1

tiny thing I noticed, the soft minimum should be -1 if the slider has a range of -1/1
AresDeveaux added 1 commit 2023-04-21 05:08:27 +02:00
AresDeveaux added 1 commit 2023-04-21 05:13:42 +02:00
Author
First-time contributor
Also, if there is going to be a "shear right" operator as well, I think the code would need some changes to avoid future code duplication. Some of the functions would need to be renamed to no longer have left in their name (like shear_left_fcurve_segment). The operator can get an extra parameter to indicate the left/right side (probably a two-value enum is best), which is then passed down to the shearing functions.

@AresDeveaux would you need/want some help with this restructuring?

hey @dr.sybren , I think I might need some help with that. The function modification is something I could do, but I'm not sure I know how to add a parameter to an operator in Blender.

``` Also, if there is going to be a "shear right" operator as well, I think the code would need some changes to avoid future code duplication. Some of the functions would need to be renamed to no longer have left in their name (like shear_left_fcurve_segment). The operator can get an extra parameter to indicate the left/right side (probably a two-value enum is best), which is then passed down to the shearing functions. @AresDeveaux would you need/want some help with this restructuring? ``` hey @dr.sybren , I think I might need some help with that. The function modification is something I could do, but I'm not sure I know how to add a parameter to an operator in Blender.
Author
First-time contributor

The same will need to be done with "Scale left", 'Scale right" and "Scale average" then.

The same will need to be done with "Scale left", 'Scale right" and "Scale average" then.

You can check the Graph_OT_decimate operator for an example on how an enum property is added.
line 528 in graph_slider_ops.c

You can check the `Graph_OT_decimate` operator for an example on how an enum property is added. line 528 in `graph_slider_ops.c`
Christoph Lendenfeld self-assigned this 2023-08-31 10:37:02 +02:00
Landed in [#111735: Animation: Shear operator for Graph Editor](https://projects.blender.org/blender/blender/pulls/111735)

Pull request closed

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
3 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#106521
No description provided.