Details

Reviewers

Commits

rBe1f3996d740c: VSE: Refactor meta operators

Summary

Move low level logic to module code and versioning logic to versioning code.

Metas strip position was handled in diffrent way compared to other strips.
This was introduced in c8b0d25794be as bugfix for T28158.
I disagree with such design. Meta strips should be handled in same way as
any other strips.

I have tested this change and haven't found any problems.
No problems after checking T28158 as well.

Diff Detail

Repository: rB Blender

Event Timeline

Richard Antalik (ISS) requested review of this revision.Jan 2 2021, 3:42 AM

Richard Antalik (ISS) created this revision.

Harbormaster completed remote builds in B11944: Diff 32372.Jan 2 2021, 3:42 AM

Further testing revealed that more meta handling code had to be changed.

Harbormaster completed remote builds in B11945: Diff 32376.Jan 2 2021, 5:31 AM

Richard Antalik (ISS) retitled this revision from VSE: Refactor meta toggle operator to VSE: Refactor meta operators.Jan 2 2021, 5:42 AM

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

clean up sequencer_meta_separate_exec as well

Harbormaster completed remote builds in B11948: Diff 32379.Jan 2 2021, 8:44 PM

Peter Fog (tintwotin) added projects: Video Sequencer, VFX & Video.Jan 3 2021, 8:53 AM

Richard Antalik (ISS) retitled this revision from VSE: Refactor meta operators to VSE: Rewrite meta operators.Jan 21 2021, 6:52 AM

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

Rebase, add docs to public functions

Harbormaster completed remote builds in B12548: Diff 33437.Feb 2 2021, 4:14 PM

add return values to docs

Harbormaster completed remote builds in B12552: Diff 33443.Feb 2 2021, 4:45 PM

Generally seems fine, two main issues:

Updating the sequence strip time (SEQ_time_update_sequence) from versioning code make indirect calls to AUD and depsgraph functions. Even though this wont run as long as seq->scene_sound == NULL.

This seems like something that could easily back-fire when making changes that don't take into account this function being called on data which hasn't been fully loaded yet.

Comments to these functions noting that they run on load would help avoid this.
This patch makes extend fail on meta-strips. Even on the most simple case (a single color strip made into a meta). Pressing E-key no longer extends.

source/blender/blenloader/intern/versioning_290.c
778	This function isn't used anywhere.
source/blender/editors/transform/transform_convert_sequencer.c
186	Tsk (this wasn't working as intended), define happened below (not an issue with this patch though).
source/blender/sequencer/SEQ_sequencer.h
65	This reads like a function that sets active sequence (at least at a glance). Where this is really a very specialized function which is not used often. This could be called `SEQ_editing_seqbase_active_set` or `SEQ_editing_seqbase_set` ?
69	Word ordering isn't following https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Naming `SEQ_active_meta_stack_get` could be `SEQ_meta_stack_active_get`. Which matches `SEQ_editing_seqbase_active_set`

This revision now requires changes to proceed.Feb 2 2021, 10:58 PM

Address inlines

Harbormaster completed remote builds in B12569: Diff 33474.Feb 3 2021, 12:08 PM

In D9972#258176, @Campbell Barton (campbellbarton) wrote:

Generally seems fine, two main issues:

Updating the sequence strip time (SEQ_time_update_sequence) from versioning code make indirect calls to AUD and depsgraph functions. Even though this wont run as long as seq->scene_sound == NULL.

This seems like something that could easily back-fire when making changes that don't take into account this function being called on data which hasn't been fully loaded yet.

Good catch, I have used only necessary logic there instead.

This patch makes extend fail on meta-strips. Even on the most simple case (a single color strip made into a meta). Pressing E-key no longer extends.

I have looked for special cases when handling meta but missed this one. I have removed special handling, now meta extends as any other strip

Found another issue when loading old file, ed->seqbasep was incorrect, so I have fixed that. I am certain I have checked this issue before so not sure how I haven't found this. Now I am testing files created with 2.70, perhaps I have tested different version before.

source/blender/sequencer/SEQ_sequencer.h
65	I have never read this guideline this way, but I can see how `object` could be any C struct and `property` could be it's member. I would rather do `SEQ_seqbase_active_set` even though it technically operates on `Editing`. Because otherwise in lower comment it should be `SEQ_editing_meta_stack_active_get`.