Page MenuHome
Differential D10116

PoC fixing regression introduced in 2.90.0 where snapping handles of a strip incorrectly changes related transform strip keyframes
ClosedPublic
Actions

Authored by Olivier Jolly (zeograd) on Jan 14 2021, 10:54 PM.

Details

Reviewers
Richard Antalik (ISS)
Maniphest Tasks
T84705: VSE: snapping source strip right handle will incorrectly offset attached transform strip keyframes
Commits
rB5501a5de32db: Fix T84705: Snapping strip handle offset animation
Summary

As asked in T84705, this is the PoC patch fixing the bug for my own use case. It's meant to be improved or rewritten.

Since 2.90.0, in the VSE, when a strip with attached transform strip is extended/shrunk using snapping on either handle, the keyframes of the related strip are also moving.
This patch uses the flag of the source strip to determine whether either handle is selected, and if so, skips offsetting of animdata.

Diff Detail

Repository
rB Blender

Event Timeline

Olivier Jolly (zeograd) requested review of this revision.Jan 14 2021, 10:54 PM
Olivier Jolly (zeograd) created this revision.
Olivier Jolly (zeograd) mentioned this in T84705: VSE: snapping source strip right handle will incorrectly offset attached transform strip keyframes.
Olivier Jolly (zeograd) updated this revision to Diff 32807.Jan 15 2021, 9:57 AM

Based on code style in the same file, factorized the condition to determine whether transform keyframes needs to be updated.

Germano Cavalcante (mano-wii) added a task: T84705: VSE: snapping source strip right handle will incorrectly offset attached transform strip keyframes.Jan 15 2021, 2:04 PM
Germano Cavalcante (mano-wii) added subscribers: Philipp Oeser (lichtwerk), Germano Cavalcante (mano-wii).Jan 15 2021, 2:18 PM

@Philipp Oeser (lichtwerk), you worked on this part of the code (rBe36c05b3d191)
Did that commit introduce the stated regression?

Philipp Oeser (lichtwerk) added a subscriber: Richard Antalik (ISS).Jan 15 2021, 2:44 PM
In D10116#252918, @Germano Cavalcante (mano-wii) wrote:

@Philipp Oeser (lichtwerk), you worked on this part of the code (rBe36c05b3d191)
Did that commit introduce the stated regression?

Yep, it did.

In T84705#1093746, @Germano Cavalcante (mano-wii) wrote:

... in fact I don't think the snap on just one side of the strip should affect the f-curve.

Agree. It seems like a sane default to not shift animation if we are only snapping handles.
(of course, one could think of a usecase where it would make sense -- more so if you are snapping the left/start handle to the left side -- but again: not doing it for snapping handles sounds good, only other thing I can think of is making the whole anim offsetting an operator option, but not sure if it is worth it.)

So to me, the fix seems good, @Richard Antalik (ISS): what do you think?

Richard Antalik (ISS) accepted this revision.Jan 18 2021, 6:06 PM
In T84705#1093746, @Germano Cavalcante (mano-wii) wrote:

... in fact I don't think the snap on just one side of the strip should affect the f-curve.

Agree. It seems like a sane default to not shift animation if we are only snapping handles.
(of course, one could think of a usecase where it would make sense -- more so if you are snapping the left/start handle to the left side -- but again: not doing it for snapping handles sounds good, only other thing I can think of is making the whole anim offsetting an operator option, but not sure if it is worth it.)

There are/can be multiple operators using SEQ_offset_animdata() so instead of operator option this could be tool setting. Moving handles should never move strip start. (that is not true for some strips currently...)

So to me, the fix seems good, @Richard Antalik (ISS): what do you think?

LGTM

This revision is now accepted and ready to land.Jan 18 2021, 6:06 PM
Richard Antalik (ISS) added a comment.Jan 19 2021, 7:40 PM

I have found some issues here, I must have been distracted, because they are quite obvious, Sorry for that.

I can commit with these changes, but I wanted to point this out.

source/blender/editors/space_sequencer/sequencer_edit.c
291

I did not notice this yesterday, this is potentially checking random strip, that is wrong. Ideally this could be handled on per-sequence basis.

Seems to be easy to do by moving either_handle_selected inside following loop:

/* Recalculate bounds of effect strips, offsetting the keyframes if not snapping any handle. */
for (seq = ed->seqbasep->first; seq; seq = seq->next) {
bool either_handle_selected = ((((Sequence *)ed->seqbasep->first)->flag) &
                              (SEQ_LEFTSEL | SEQ_RIGHTSEL)) != 0;
...
}
292

it should be (SEQ_LEFTSEL | SEQ_RIGHTSEL)

Richard Antalik (ISS) updated this revision to Diff 32940.Jan 19 2021, 7:46 PM
Richard Antalik (ISS) edited the summary of this revision. (Show Details)
  • fix issues
Harbormaster completed remote builds in B12282: Diff 32940.Jan 19 2021, 7:46 PM
Ankit Meel (ankitm) added a subscriber: Ankit Meel (ankitm).Jan 21 2021, 6:40 PM
Ankit Meel (ankitm) added inline comments.
source/blender/editors/space_sequencer/sequencer_edit.c
330

const bool ...

1266

It seems you forgot to switch to master before pulling another arc patch.
This is from another diff.
Even then, const int ...

Richard Antalik (ISS) added a comment.Jan 22 2021, 2:58 AM

Also did not change ((((Sequence *)ed->seqbasep->first)->flag) to seq->flag

It is starting to look that I need a therapy or something...

source/blender/editors/space_sequencer/sequencer_edit.c
1266

Oops that was the case, thanks for pointing this out.

Richard Antalik (ISS) updated this revision to Diff 33014.Jan 22 2021, 3:00 AM
  • fix issues
Harbormaster completed remote builds in B12317: Diff 33014.Jan 22 2021, 3:00 AM
Closed by commit rB5501a5de32db: Fix T84705: Snapping strip handle offset animation (authored by Olivier Jolly (zeograd), committed by Richard Antalik (ISS)). · Explain WhyJan 25 2021, 6:00 AM
This revision was automatically updated to reflect the committed changes.
Richard Antalik (ISS) added a commit: rB5501a5de32db: Fix T84705: Snapping strip handle offset animation.

Revision Contents
Changeset List

PathSize
source/
blender/
editors/
space_sequencer/
16 lines

Diff 33091

View Options

source/blender/editors/space_sequencer/sequencer_edit.c

Loading...