Move remove gaps operator logic to module code
ClosedPublic
Actions

Authored by Richard Antalik (ISS) on Dec 3 2020, 7:32 AM.

Details

Reviewers

Sergey Sharybin (sergey)

Commits

rB9e4a4c2e996c: VSE: Move remove gaps operator logic to module code

Summary

Logic was broken into finding gaps and ofsetting strips.
Functions were modified so they work on explicitly defined seqbase,
so they can be used as python API functions.

Functional changes:

Improve performance by calculating gap length and offseting strips at once. Previously strips were offset by one frame.
Calculate gap from start frame. Previously gap was considered only inbetween strips.

Diff Detail

Repository

rB Blender

Branch

rmgap (branched from master)

Build Status

Buildable 11537
Build 11537: arc lint + arc unit

Event Timeline

Richard Antalik (ISS) created this revision.Dec 3 2020, 7:32 AM

Harbormaster completed remote builds in B11537: Diff 31559.Dec 3 2020, 7:32 AM

Richard Antalik (ISS) requested review of this revision.Dec 3 2020, 7:32 AM

Calculate gap from start frame. Previously gap was considered only inbetween strips.

What does this mean for users?

Also, something what is applicable in general: there should be explanation of what function do. For example, the meaning and behavior of SEQ_offset_after_frame is not very clear.

source/blender/sequencer/SEQ_sequencer.h
603	We do have comment section covered by the style guide: https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments I would also say instead of having uber-header stating which C file logic is implemented in there should be separate headers like `SEQ_timeline.h`, `SEQ_transform.h` and so on.
source/blender/sequencer/intern/strip_time.h
38–41	Think some explanation is needed here. Point of confusion: Having start, end, and length seems redundant. Having information about other gap in the structure which denotes gap information is also not very straightforward. Could be fine, but explanation is needed.

This revision now requires changes to proceed.Dec 3 2020, 10:46 AM

Address inlines

Harbormaster completed remote builds in B11559: Diff 31600.Dec 4 2020, 10:24 AM

I forgot to press submit button here. Submitting now to address possible questions.

In D9730#241696, @Sergey Sharybin (sergey) wrote:

Calculate gap from start frame. Previously gap was considered only inbetween strips.

What does this mean for users?

Now when you remove content from start of timeline, you have to select all strips that are aligned, move playhead to start of timeline and snap these strips to playhead. It is mildly irritating.
Worst case you don't have aligned strips and you have to move strips manually.

With this change, just move playhead to start of timeline and execute this operator. All strips will shift so there is no gap between timeline start and first strip.

Also, something what is applicable in general: there should be explanation of what function do. For example, the meaning and behavior of SEQ_offset_after_frame is not very clear.

Yes I should add docs to mostl functions that are in external headers.

source/blender/sequencer/SEQ_sequencer.h
603	I know I should split this header, I didn't want to do it yet because I thought that I may end up shuffling some functions around. In any case I will clean this up in separate commit, now I am following style. I will also think about splitting this file now.
source/blender/sequencer/intern/strip_time.h
38–41	`next_gap_start_frame` was meant to be optimization, but it is not used and even a bit dangerous. `gap_length` is more readable in `SEQ_edit_remove_gaps()`, `gap_start_frame` and `gap_end_frame` are used internally in `seq_time_gap_info_get()`