Animation: Move Snapping to Scene #109015
No reviewers
Brad Clark
Nathan Vegdahl
Sybren A. Stüvel
Labels
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
6 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#109015
Loading…
Reference in New Issue
No description provided.
Delete Branch "ChrisLend/blender:frame_snapping"
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?
Part of #91973
Moving the snapping code for the
into the common system. This includes the Magnet icon for turning snapping on and off.
The old settings translate to the new in the following way:
Frame Step->FrameSecond Step->SecondNearest Frame->Frame+Absolute Time SnapNearest Second->Second+Absolute Time SnapNearest Marker->Nearest MarkerSince this moves the location of the snapping settings from the editor to the scene, it changes the behavior.
Previously each editor could have different snapping settings, where now they are all synced.
A side effect of moving to the new system is that pressing CTRL now disables snapping, instead of enabling snapping to seconds as it did in 3.6I changed it so the old behavior is kept.Currently all animation editors share their flags, but potentially the NLA can have different flags (e.g. snap to strips). It has been decided in the A&R module meeting that we will change that if we implement new snapping modes.
First I think I didn't realize that the snapping was per editor and I can't think of why I would want to have to manage it for each place I can move a keyframe.. I either want it to be on frame or not everywhere vs. forget or have one that allows me to end up in subframes by mistake and then not be able to get to them and snap them somewhere else so from a concept of consistency, of user expectations and of "don't make me tell you twice Blender" I think that is an improvement.
so far this only applies to the on/off switch
do you think we should also make the actual snapping option sync between editors?
I think that's the right way to go, but please check with a few animators ;-)
A quick test with the NLA shows that the 'to nearest marker' seems to be broken there. This could be unrelated to this particular PR; without it I cannot even find how to enable the snapping in the NLA, so it could be that it was always broken (but nobody found out).
Yes!, I think this is definitelly in the right direction, as per brad's note I think it should stay the same for multiple animation editors, but this is definitelly the right idea!
Do you still need help with this?
yes please
Animation: Snapping options new GUIto WIP: Animation: Snapping options new GUItaking this back into WIP status to completely migrate the snapping system to live on the scene like it does for all other editors
this will make it easy to also implement new snapping behavior in the future
WIP: Animation: Snapping options new GUIto Animation: Move Snapping to Scene@blender-bot package
I moved snapping completely to the new system
@BClark @LucianoMunoz would be great if you two could have a play with it
NLA marker snapping is still broken but that is for a separate PR
Package build started. Download here when ready.
@blender-bot package
@BClark when you are back, here is a fresh build with your issues addressed :)
Package build started. Download here when ready.
Over all looks good to me! Mainly just some questions + nits.
@ -592,2 +592,4 @@}static void invert_snap(eSnapMode &snap_mode){Since this function is duplicated in three places, maybe it makes sense to just define it once in one place to be reused everywhere? Not sure if there's a semantic difference between the three copies that I'm missing.
Maybe we could also name it something like
swap_frame_second_snap_mode()? Maybe that's still a bit obtuse. But since there's a third option,SCE_SNAP_TO_MARKERS, "invert" doesn't sound right to me. And on first reading that actually made me think there were only those two options.this goes back to the whole thing of every editor potentially has different snapping options. Because of that I wanted to keep the code separate.
and I named it invert snap because it gets called when the invert button is pressed (CTRL in this case). There can potentially be different inversion rules in the future.
let me know what you think
Got it. Yeah, definitely sounds like there's a semantic difference between the "duplicates", then. So thumbs-up on leaving them separate.
Your explanation of the name also makes sense to me. Maybe just add a comment briefly explaining that its purpose is for the invert hotkey, and that there are more than two options/behaviors in reality. Just to make the code a little easier to follow locally.
@ -51,3 +51,3 @@float delta_x = val - ival;if (ELEM(autosnap, SACTSNAP_SECOND, SACTSNAP_TSTEP)) {if (ELEM(snap_mode, SCE_SNAP_TO_SECOND)) {Probably this can just be
snap_mode == SCE_SNAP_TO_SECONDnow.@ -88,2 +78,2 @@float val_final,float *r_val_final);void snapFrameTransform(TransInfo *t, eSnapMode autosnap, float val_initial, float val_final, float *r_val_final);Would it be worth taking the time to switch from
r_val_finalto just returning the value? (transform_snap_anim_flush_data()is also like this.) Maybe not—maybe better as a separate clean up PR.yes but I agree this should be in a separate PR
@ -2302,2 +2305,4 @@SCE_SNAP_NOT_TO_ACTIVE = (1 << 4),SCE_SNAP_ABS_GRID = (1 << 5),/* Same value with different name to make it easier to understand in time based code. */SCE_SNAP_ABS_TIME_STEP = (1 << 5),Instead of adding an alias, it might make sense to change the existing name to a new name that encompasses both. Maybe
SCE_SNAP_ABS_INCREMENT?hm I wanted to avoid touching the rest of the snapping code. I saw that pattern of different aliases used for the same value in other blender code and thought it's quite easy to read.
imo it's clearer to have them separate and use them in their respective context. But let me know what you think
Ah... okay. I tested out Blender's current behavior, and it's different than I thought. I assumed that the option applied to any increment snapping, but it turns out that in the 3D viewport it only affects translating, not rotating or scaling. So I think you're right that the aliases are better, because the specificity is meaningful right now.
(In the future it might be good to extend the feature to also affect e.g. rotation and scaling, and at that point the enum value could be merged into a single name. But that's definitely not for this PR.)
@ -27,6 +27,8 @@ DEF_ENUM(rna_enum_proportional_falloff_curve_only_items)DEF_ENUM(rna_enum_snap_source_items)DEF_ENUM(rna_enum_snap_element_items)DEF_ENUM(rna_enum_snap_node_element_items)DEF_ENUM(rna_enum_snap_action_element_items)Is
rna_enum_snap_action_element_itemsused anywhere? I couldn't find it. Then again, RNA stuff still seems a little magical to me sometimes, so I easily could have just missed it.No you are right. I removed this one and renamed the other to
rna_enum_snap_animation_element_itemsto indicate they are used in animation in general.@blender-bot package
Failed to start build, can't connect to buildbot.
@blender-bot package
Package build started. Download here when ready.
In just trying it again and a bit out of context but what I expected..
Marker snap, having both ends snap to makers still creates a point where the clip gets scaled to zero if you don't have two markers or you drag it past the last marker.
Expectation is that only the entire strip offset would snap, not the start/end but that is because I would think markers for aligning things not changing strip length...maybe I am wrong.
Not having absolute on by default means seeing snap on, I think I am safe from creating sub-frame value but I am able to but then it keeps those values when I snap.
the snap to markers in NLA will be fixed later, this PR is solely for moving to the new system
the reason why I kept absolute snap off for now is because when you are moving keys to seconds (Dope Sheet or Graph Editor), it would snap to the actual second, collapsing all the keys.
We can revisit this later, but having it off doesn't actually produce any subframes, it just keeps them if you already have them
This looks good to me. Probably wait until @dr.sybren (currently on vacation) also gets a chance do a final review before landing.
@ -479,3 +477,1 @@* (eAnimEdit_AutoSnap in DNA_action_types.h).*/short autosnap;/* Snapping now lives on the Scene. */👍 for pointing to the new location.
Nice work! Just one note that can be handled when landing.
The buildbot failed on a Cycles test, so I doubt that this has anything to do with this PR.
Please link to the meeting notes on devtalk, so that the decision can be traced back.
@ -820,6 +820,18 @@ static void flushTransIntFrameActionData(TransInfo *t)}}static void invert_snap(eSnapMode &snap_mode)Add a comment that explains what this 'inversion' is for. I had to read about the "CTRL key to switch between seconds & frames" behaviour of 3.6 that you put in the PR description to know this was a thing (so 👍 for putting it there), and I think it'll be helpful for future devs (including future us) if this info is actually in the source.