Details

Reviewers

Sergey Sharybin (sergey)
Germano Cavalcante (mano-wii)

Commits

rBe7003bc9654b: VSE: Reduce transform code complexity

Summary

Reduce complexity of sequencer transform code by removing recursivity.
This is possible by treating meta strips (mostly) as any other strip and
containing all transform code within SEQ_ functions.

Unfortunately internally meta strips still require special treatment,
but all complexity from code all over transform code seems to be
possible to contain within one function.

Functional change:
Previously adjusting handle of single image strip moved animation.
Now animation is not moved, which is behavior for all other strips.

Diff Detail

Repository

rB Blender

Branch

simplify_transform2 (branched from master)

Build Status

Buildable 14991
Build 14991: arc lint + arc unit

Event Timeline

Richard Antalik (ISS) requested review of this revision.Jun 4 2021, 1:49 AM

Richard Antalik (ISS) created this revision.

Harbormaster completed remote builds in B14943: Diff 37835.Jun 4 2021, 1:49 AM

Rebase on master

Harbormaster completed remote builds in B14991: Diff 37920.Jun 6 2021, 3:56 AM

Remove more code, TODO: now strips can overlap

Harbormaster completed remote builds in B15230: Diff 38369.Jun 16 2021, 2:10 AM

Richard Antalik (ISS) retitled this revision from [WIP] VSE: Reduce transform code complexity to VSE: Reduce transform code complexity.Jun 16 2021, 3:44 AM

Richard Antalik (ISS) edited the summary of this revision. (Show Details)

Fix missing overlap correction
Resolve outstanding issues

Harbormaster completed remote builds in B15233: Diff 38372.Jun 16 2021, 3:50 AM

Clear remaining seq->depth usage. unrelated, but it was finally cleared by this patch.

Harbormaster completed remote builds in B15234: Diff 38373.Jun 16 2021, 3:57 AM

Richard Antalik (ISS) added a reviewer: Sergey Sharybin (sergey).Jun 16 2021, 4:03 AM

Richard Antalik (ISS) edited the summary of this revision. (Show Details)

Richard Antalik (ISS) mentioned this in D11618: VSE: Refactor transform operator code.Jun 16 2021, 8:52 AM

Think its mainly fine. Some little suggestion about the comment.

The transform code isn't my area, so maybe include Germano into review?

source/blender/makesdna/DNA_sequence_types.h
172 ↗	(On Diff #38373)	Remove, or change it to `pad` for the future remove. Don't leave fields which are supposed to be unused to avoid their accidental use.
source/blender/sequencer/intern/strip_transform.c
236	Not sure this comment adds information. You can see that metas need special handling from the if statement. Some suggested wording: Meta strips requires special handling: their content is to be translated, and then frame range of the meta is to be updated for the updated content.

Use nicer comment, change unused DNA field to pad

Harbormaster completed remote builds in B15246: Diff 38390.Jun 16 2021, 10:36 AM

In D11493#298309, @Sergey Sharybin (sergey) wrote:

Think its mainly fine. Some little suggestion about the comment.

The transform code isn't my area, so maybe include Germano into review?

Will check with Germano, if he wants to have a look

It's always nice to see the transform code being simplified and it makes sense to move some updates to the internals of the sequencer.
The structure of the transform code hasn't changed and I confess that I don't touch the Sequencer's transform much (Is it just the "Translate" and "Sequence Slide" that affects this area?)

(It's a little strange that the after-transform behavior of the sequencer (use_sync_markers and T_ALT_TRANSFORM) is in the freeSeqData function. It would be expected to be in the special_aftertrans_update__sequencer but that is not the fault of the patch).

So, in an overview, if everything is tested well, it looks good.

This revision is now accepted and ready to land.Jun 16 2021, 2:16 PM

Closed by commit rBe7003bc9654b: VSE: Reduce transform code complexity (authored by Richard Antalik (ISS)). · Explain WhyJun 17 2021, 3:47 AM

This revision was automatically updated to reflect the committed changes.

Richard Antalik (ISS) added a commit: rBe7003bc9654b: VSE: Reduce transform code complexity.

In D11493#298433, @Germano Cavalcante (mano-wii) wrote:

It's always nice to see the transform code being simplified and it makes sense to move some updates to the internals of the sequencer.
The structure of the transform code hasn't changed and I confess that I don't touch the Sequencer's transform much (Is it just the "Translate" and "Sequence Slide" that affects this area?)

I think so. There are operators like SEQUENCER_OT_snap but these use sequencer internal transform functions.

(It's a little strange that the after-transform behavior of the sequencer (use_sync_markers and T_ALT_TRANSFORM) is in the freeSeqData function. It would be expected to be in the special_aftertrans_update__sequencer but that is not the fault of the patch).

I think it should be possible to move these if current state is not following convention. This seems to be good idea to do for markers, but not sure about T_ALT_TRANSFORM. I will first remove tagging with seq->tmp from implementation, then it may be easier to freely move around.

Edit: I see now that makers are mainly handled outside of special_aftertrans_update__sequencer due to overlap which affects unselected strips. So it may not be possible to easily decouple these.

VSE: Reduce transform code complexity
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 37920

source/blender/editors/transform/transform_convert_sequencer.c

source/blender/sequencer/SEQ_time.h

source/blender/sequencer/intern/strip_time.c

source/blender/sequencer/intern/strip_transform.c

VSE: Reduce transform code complexityClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 37920

source/blender/editors/transform/transform_convert_sequencer.c

source/blender/sequencer/SEQ_time.h

source/blender/sequencer/intern/strip_time.c

source/blender/sequencer/intern/strip_transform.c

VSE: Reduce transform code complexity
ClosedPublic
Actions

Revision Contents
Changeset List