Details

Reviewers

Maniphest Tasks

T87173: Some Auto-Snap options in animation editors don't work correctly

Summary

This was partially broken with rBde9ea94fc6f8: Transform: Deduplicate time snap code.

The Frame Step and Second Step snapping options were working as if
they were Nearest Frame and Nearest Second respectively in the
Dope Sheet and NLA editors.

In the Graph Editor the problem was more serious:
"Second Step: ... The keyframe itself moves along as though in snapping
were active at all, while its handles 'stay behind' until it reaches
the next second boundary, at which point the teleport handles to
'catch up'".

Also, for the Graphic Editor, snap inversion (by pressing Ctrl) would
trigger incremental snapping (which should not be supported).

The snapping code for these modes was spread across the transform
mode code and recalcData of each data type. Therefore, create a
unified snapping code for these options so that all issues are fixed in
one place.

Diff Detail

Repository

rB Blender

Branch

T87173_a (branched from master)

Build Status

Buildable 16463
Build 16463: arc lint + arc unit

Event Timeline

Philipp Oeser (lichtwerk) requested review of this revision.Aug 17 2021, 12:03 PM

Philipp Oeser (lichtwerk) created this revision.

Harbormaster completed remote builds in B16463: Diff 40775.Aug 17 2021, 12:03 PM

The idea behind rBde9ea94fc6f8, is to join all snap types in the doAnimEdit_SnapFrame function.
This would be ideal as this function is called for different modes.
Isolating SACTSNAP_TSTEP and SACTSNAP_STEP in a specific mode only seems to work around the problem.
Could the problem be inside snapFrameTransform?

In D12241#317445, @Germano Cavalcante (mano-wii) wrote:

The idea behind rBde9ea94fc6f8, is to join all snap types in the doAnimEdit_SnapFrame function.
This would be ideal as this function is called for different modes.
Isolating SACTSNAP_TSTEP and SACTSNAP_STEP in a specific mode only seems to work around the problem.
Could the problem be inside snapFrameTransform?

I know this is somewhat going against the idea behind the refactor.
The whole thing would need to be refactored further (if we want this all in one place), because (and this is the main thing):

for stepping, you do the rounding prior to adding it anywhere, for snapping you add then round.

calculate delta value in snapping

Harbormaster completed remote builds in B16469: Diff 40781.Aug 17 2021, 3:16 PM

Second Step (dopesheet) is now snapping to full seconds again.
(Graph Editor is another story -- this needs more refactor since it is doing the rounding in flushTransGraphData which is too late, it needs to round the delta while transforming before adding)

Fix snapping of second step

Harbormaster completed remote builds in B16475: Diff 40787.Aug 17 2021, 3:39 PM

Germano Cavalcante (mano-wii) commandeered this revision.Aug 17 2021, 4:03 PM

Germano Cavalcante (mano-wii) edited reviewers, added: Philipp Oeser (lichtwerk); removed: Germano Cavalcante (mano-wii).

The solution was to deduplicate the code and merge all the logic into the transform_snap_anim_flush_data and transform_convert_flush_handle2D functions

Harbormaster completed remote builds in B16486: Diff 40809.Aug 17 2021, 10:31 PM

functionality now seems to work correctly, thx!
one exception is the Graph Editor were holding Ctrl gets you in this weirs state of snapping to 5 frames step interval [this was the case prior to this patch as well, but might be a good occasion to have a look at]
you might want to update the Diff description (since this was quite a big refactor, an outline of the required changes would be nice)?

Germano Cavalcante (mano-wii) retitled this revision from Fix wrong Anim Auto-Snap (Frame Step / Second Step) to Fix T87173: Incorrect snap in animation editors.Aug 18 2021, 3:51 PM

Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)

Also, for NLA and Graph Editor the snap invert (Ctrl pressing) was not working.

this was already fixed by rB0246128b7f9c: Fix wrong Anim Auto-Snap Ctrl toggle, can be removed from the description, no?

Cleanup/Refactor Action conversion
Fix incremental snap used in Graph Editor
Workaround: Edit the Graph Editor header in transform translate mode

Harbormaster completed remote builds in B16501: Diff 40836.Aug 18 2021, 5:07 PM

Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)Aug 18 2021, 5:30 PM

Germano Cavalcante (mano-wii) retitled this revision from Fix T87173: Incorrect snap in animation editors to Fix T87173: wrong Auto-Snap in animation editors.Aug 18 2021, 6:43 PM

Philipp Oeser (lichtwerk) mentioned this in T87173: Some Auto-Snap options in animation editors don't work correctly.Aug 19 2021, 9:19 AM

Quoting this from T87173 (but I dont think this should be holding this back and I asked to report this separately):

In T87173#1208308, @Philipp Oeser (lichtwerk) wrote:

Nearest Marker: interestingly, moving an action with this snapping option enabled changes the action's length. If you drag it between two markers, it'll be stretched so it starts at one marker and ends at the other. If you drag it past the first or last marker, its length becomes a tenth of a frame.

In the NLA, this is actually true for all of the auto-snapping options (not just Nearest Marker).
This is the one thing that is still not solved with D12241: Fix T87173: wrong Auto-Snap in animation editors and I think at this point it would be good to report this separately (I assume this will involve some design decision, it has always taken into account the underlying keyframes [instead of only one strip border as one might expect] -- this might be interesting for @Wayde Moss (GuiltyGhost) or @Sybren A. Stüvel (sybren) )

I tested functionality and all LGTM.
You were mentioning this in chat: not sure if this should have separate commits? (I dont really think it would neccessarily have to be split, only thing I was wondering is if this could somehow make its way back into 2.93 LTS (not sure if is too risky)?)
So green light from me (codewise I could not spot a smoking gun, but have to pretty much trust you on this).

This revision is now accepted and ready to land.Aug 19 2021, 9:29 AM

Germano Cavalcante (mano-wii) mentioned this in rBb0d9e6797fb8: Fix T87173: wrong Auto-Snap in animation editors.Aug 19 2021, 3:34 PM

Resolved by committing {b0d9e6797fb8}

Germano Cavalcante (mano-wii) mentioned this in D12268: Fix T87173: wrong Auto-Snap in animation editors - For 2.93 LTS.Aug 19 2021, 4:13 PM

Fix T87173: wrong Auto-Snap in animation editors
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 40775

source/blender/editors/transform/transform_mode.c

source/blender/editors/transform/transform_mode_timescale.c

source/blender/editors/transform/transform_mode_timetranslate.c

source/blender/editors/transform/transform_snap.c

Fix T87173: wrong Auto-Snap in animation editorsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 40775

source/blender/editors/transform/transform_mode.c

source/blender/editors/transform/transform_mode_timescale.c

source/blender/editors/transform/transform_mode_timetranslate.c

source/blender/editors/transform/transform_snap.c

Fix T87173: wrong Auto-Snap in animation editors
ClosedPublic
Actions

Revision Contents
Changeset List