Page MenuHome
Differential D9760

VSE: Refactor VSE strip loading code
ClosedPublic
Actions

Authored by Richard Antalik (ISS) on Dec 6 2020, 1:48 PM.

Details

Reviewers
Sergey Sharybin (sergey)
Commits
rBbbb1936411a5: VSE: Refactor VSE strip loading code
Summary

Isolate RNA and operator logic from functions that create strips.

  • Operator specific code was removed from SeqLoadInfo structure and SEQ_add_* functions.
  • Strip loading code was removed from RNA and operator functions.
  • SEQ_add_* API was unified to work on SeqLoadData struct. Only exception is image strip, which require files to be loaded separately to strip creation itself. This is not ideal, but I think it's acceptable.
  • Some functions and variables were refactored so the code reads better.

There are minor functional changes (coincidental bugfixes):

  • Operator errors are reported per-strip. Previously they were not reported at all?
  • new_sound() RNA API function now create sound with length of 1 if source file does not exist. Previously it created strip with length of 0.
  • Replace selection operator property wasn't working correctly. Fixed in this patch.

Diff Detail

Repository
rB Blender
Branch
arcpatch-D9760 (branched from master)
Build Status
Buildable 11735
Build 11735: arc lint + arc unit

Event Timeline

Richard Antalik (ISS) created this revision.Dec 6 2020, 1:48 PM
Harbormaster completed remote builds in B11578: Diff 31650.Dec 6 2020, 1:48 PM
Richard Antalik (ISS) requested review of this revision.Dec 6 2020, 1:48 PM
Richard Antalik (ISS) updated this revision to Diff 31651.Dec 6 2020, 3:09 PM
  • Resolve todos and cleanup
Harbormaster completed remote builds in B11579: Diff 31651.Dec 6 2020, 3:09 PM
Richard Antalik (ISS) retitled this revision from De-duplicate RNA API and operator strip load functions to VSE: Rewrite strip loading code.Dec 6 2020, 4:14 PM
Richard Antalik (ISS) edited the summary of this revision. (Show Details)
Richard Antalik (ISS) updated this revision to Diff 31964.Dec 14 2020, 3:14 PM
  • Fix issues
Harbormaster completed remote builds in B11735: Diff 31964.Dec 14 2020, 3:14 PM
Richard Antalik (ISS) edited the summary of this revision. (Show Details)Dec 14 2020, 3:23 PM
Richard Antalik (ISS) updated this revision to Diff 31966.Dec 14 2020, 3:44 PM
Richard Antalik (ISS) edited the summary of this revision. (Show Details)
  • Correct error reporting, so it is more consistent with previous behavior
Harbormaster completed remote builds in B11737: Diff 31966.Dec 14 2020, 3:44 PM
Peter Fog (tintwotin) added projects: Video Sequencer, VFX & Video.Dec 19 2020, 12:45 AM
Richard Antalik (ISS) edited the summary of this revision. (Show Details)Jan 3 2021, 1:14 AM
Richard Antalik (ISS) mentioned this in T77580: Sequencer: restructuring the code layout.Jan 11 2021, 9:04 AM
Richard Antalik (ISS) updated this revision to Diff 33383.Feb 1 2021, 1:53 PM
  • rebase
Harbormaster completed remote builds in B12509: Diff 33383.Feb 1 2021, 1:53 PM
Richard Antalik (ISS) updated this revision to Diff 33456.Feb 2 2021, 8:25 PM
  • add docs
Harbormaster completed remote builds in B12558: Diff 33456.Feb 2 2021, 8:25 PM
Richard Antalik (ISS) edited the summary of this revision. (Show Details)Feb 4 2021, 4:07 PM
Richard Antalik (ISS) added a reviewer: Sergey Sharybin (sergey).Feb 4 2021, 5:18 PM
Richard Antalik (ISS) edited the summary of this revision. (Show Details)Feb 5 2021, 11:28 AM
Sergey Sharybin (sergey) added a comment.Feb 15 2021, 10:09 AM

There are some suggestions in the code.

Would also think that the commit message needs to be clarified. Think it will be more clear to title it as Refactor VSE strip loading code. Makes it explicit that it's a house-keeping change which does not really affect user land.

source/blender/editors/space_sequencer/sequencer_add.c
690

const

692

I would personally split those branches into sequencer_add_sound_single_strip and sequencer_add_sound_multiple_strips. Reduces vertical space of the operator and limits function to a more dedicated functionality.

source/blender/sequencer/SEQ_add.h
48–56 ↗(On Diff #33456)

For clarity you can use nested structs, to separate the fields in a way that it is clear from reading just code without seeing the comment.

For example,

struct {
  int type;
  int end_frame;
  struct Sequence *seq1;
  struct Sequence *seq2;
  struct Sequence *seq3;
} effect;
Richard Antalik (ISS) updated this revision to Diff 34581.Mar 1 2021, 9:24 AM
Richard Antalik (ISS) marked 3 inline comments as done.
  • Address inlines
Harbormaster completed remote builds in B13165: Diff 34581.Mar 1 2021, 9:24 AM
Richard Antalik (ISS) retitled this revision from VSE: Rewrite strip loading code to VSE: Refactor VSE strip loading code.Mar 1 2021, 9:24 AM
Sergey Sharybin (sergey) accepted this revision.Mar 1 2021, 4:35 PM

To me sounds fine.

This revision is now accepted and ready to land.Mar 1 2021, 4:35 PM
Closed by commit rBbbb1936411a5: VSE: Refactor VSE strip loading code (authored by Richard Antalik (ISS)). · Explain WhyMar 2 2021, 12:23 PM
This revision was automatically updated to reflect the committed changes.
Richard Antalik (ISS) added a commit: rBbbb1936411a5: VSE: Refactor VSE strip loading code.

Revision Contents
Changeset List

PathSize
source/
blender/
editors/
space_sequencer/
518 lines
makesrna/
intern/
192 lines
sequencer/
104 lines
intern/
28 lines
387 lines

Diff 31964

View Options

source/blender/editors/space_sequencer/sequencer_add.c

Loading...
View Options

source/blender/makesrna/intern/rna_sequencer_api.c

Loading...
View Options

source/blender/sequencer/SEQ_sequencer.h

Loading...
View Options

source/blender/sequencer/intern/render.c

Loading...
View Options

source/blender/sequencer/intern/strip_add.c

Loading...