VSE: Fix assigning effect strip inputs
ClosedPublic
Actions

Authored by Richard Antalik (ISS) on Feb 17 2020, 2:51 AM.

Details

Reviewers

Campbell Barton (campbellbarton)

Maniphest Tasks

T73828: VSE: using python to remove a scene strip with effect applied causes... weirdness

Commits

rB47f98a38d0f8: VSE: Fix assigning effect strip inputs

Summary

Partialy fixes T73828

Currently all 3 effect inputs were assigned even if not all 3 were used. This causes problems with reassigning effects in python, because 3rd input is not accessible.

This patch will only assign inputs that are necessary for effect to work properly.

Diff Detail

Repository

rB Blender

Branch

D6868 (branched from master)

Build Status

Buildable 8349
Build 8349: arc lint + arc unit

Event Timeline

Richard Antalik (ISS) created this revision.Feb 17 2020, 2:51 AM

Harbormaster completed remote builds in B6648: Diff 21782.Feb 17 2020, 2:51 AM

Richard Antalik (ISS) added a reviewer: Campbell Barton (campbellbarton).Feb 19 2020, 6:57 PM

Richard Antalik (ISS) added a project: Video Sequencer.

Richard Antalik (ISS) mentioned this in D7201: Fix T74958: Infinite loop on using strip as modifier mask.Mar 21 2020, 1:00 AM

Richard Antalik (ISS) edited the summary of this revision. (Show Details)Mar 21 2020, 1:16 AM

I'm not sure why this patch makes so many changes, if the issue is values being assigned that don't make sense, can't this be handled by a few lines at the end of seq_effect_find_selected ?

The logic here is difficult to follow (before this patch too), it looks like there are functional changes, re: make sequence selection a little bit more intuitive.

I'd be inclined to leave this code mostly as-is, and simply check BKE_sequence_effect_get_num_inputs and NULL any extra extra members set, after calling seq_effect_find_selected (or make that part of the function, and do this at the very end, with note that the logic needs a review).

Or, the function could be rewritten to make the logic easier to follow, suggest to pass in an array which is filled in (replacing selseq1/2/3), making it possible to generalize some of this logic.

source/blender/editors/space_sequencer/sequencer_edit.c
573	`*_mode` is normally use for enums, in this case the name isn't meaningful, it could be negated and called `use_active_strip`.

This revision now requires changes to proceed.Jun 2 2020, 5:54 AM

In D6868#190909, @Campbell Barton (campbellbarton) wrote:

I'm not sure why this patch makes so many changes, if the issue is values being assigned that don't make sense, can't this be handled by a few lines at the end of seq_effect_find_selected ?

The logic here is difficult to follow (before this patch too), it looks like there are functional changes, re: make sequence selection a little bit more intuitive.

I'd be inclined to leave this code mostly as-is, and simply check BKE_sequence_effect_get_num_inputs and NULL any extra extra members set, after calling seq_effect_find_selected (or make that part of the function, and do this at the very end, with note that the logic needs a review).

Or, the function could be rewritten to make the logic easier to follow, suggest to pass in an array which is filled in (replacing selseq1/2/3), making it possible to generalize some of this logic.

In general I agree with you. Main issue there is that seq_effect_find_selected() is useed all over the place. I think that I have mostly removed code that either was doing nothing or was too obscure. Now I don't find it really hard to read, but it's definitely still ugly.

I will give it one more go and see if I can make it nicer.

Address inlines.

Harbormaster completed remote builds in B8349: Diff 25436.Jun 3 2020, 6:17 AM

Changed my mind, I want to get bugs number now primarily, I can beautify code later.

Campbell Barton (campbellbarton) accepted this revision.Jun 3 2020, 6:32 AM

Campbell Barton (campbellbarton) added inline comments.

source/blender/blenkernel/intern/sequencer.c
2983	The check for 1 seems odd, why not check the number of non-null ibuf's matches `BKE_sequence_effect_get_num_inputs` result?

This revision is now accepted and ready to land.Jun 3 2020, 6:32 AM

Richard Antalik (ISS) added inline comments.Jun 3 2020, 6:47 AM

source/blender/blenkernel/intern/sequencer.c
2983	I mean it would work, but there are some issues: No versioning, so that would not work with older files Speed strip having 2 possible input images with 1 input strip I could do `if (i >= BKE_sequence_effect_get_num_inputs(seq->type))`