Paths

Table of Contentst

VSE: modify select_active_side operator to work with multiple channels
ClosedPublic
Actions

Authored by Alessio Monti di Sopra (a.monti) on Oct 24 2019, 5:11 PM.

Details

Reviewers

Richard Antalik (ISS)
Campbell Barton (campbellbarton)

Summary

The patch changes the behaviour of the select_active_side operator in the video sequencer, in order to allow it to select multiple channels, instead of only one.

The time reference is the most left (or right, depending on the selection side) selected strip in the channel.

Unlike before, the active strip is not automatically added to the selection.

Diff Detail

Repository: rB Blender

Event Timeline

Alessio Monti di Sopra (a.monti) created this revision.Oct 24 2019, 5:11 PM

Would make this a separate operator, or - replace the current operator if using the active item is not useful (would be good to get feedback from users on that).

Logic from this patch to use active as a fallback seems a bit strange.

source/blender/editors/space_sequencer/sequencer_select.c
931	No need to allocate an array, a static array sized `MAXSEQ` will do. The bounds can be stored in that.

This revision now requires changes to proceed.Oct 24 2019, 5:40 PM

Campbell Barton (campbellbarton) added inline comments.Oct 24 2019, 5:43 PM

source/blender/editors/space_sequencer/sequencer_select.c
941	Shouldn't this use the end?

Peter Fog (tintwotin) added a subscriber: Peter Fog (tintwotin).Oct 24 2019, 7:16 PM

This comment was removed by Peter Fog (tintwotin).

In D6127#140896, @Campbell Barton (campbellbarton) wrote:

Would make this a separate operator, or - replace the current operator if using the active item is not useful (would be good to get feedback from users on that).

Logic from this patch to use active as a fallback seems a bit strange.

Was discussing this in #vse with @Peter Fog (tintwotin) maybe he can help finding the best solution. The general idea was to be consistent with the other exposed selection operators, that work with the full selection of strips instead of only the active one.
It would actually need a new name, I didn't think about it. Maybe select_channel_side or select_by_channel ?

As for the fallback, I though it as a way to keep the old behaviour around for those who were used to it. I have not a strong opinion on this, if you think it should be removed I'll do it.

Alessio Monti di Sopra (a.monti) added inline comments.Oct 24 2019, 7:42 PM

source/blender/editors/space_sequencer/sequencer_select.c
941	I did it at the beginning but, since the function that actually applies the selection always uses the start frame of the strip in its logic, if you were to select on the left side you could end up also selecting strips that ends way after the one you originally used as reference. I didn't change the logic there, since that function were also being used in the main selection operator which logic is a bit complex, so I used that compromise and forgot to better look into it. I will look if changing it break anything though in the other operator, and if not I'll do it. Or I can rewrite the switch for this specific use.

As for the fallback, I though it as a way to keep the old behavior around for those who were used to it.

If this is useful in it's current state, it should be kept not hidden-behind non-obvious logic.

Consider documentation for the new behavior - if the documentation for this needs statements to explain how to get certain functionality by selecting one, then removing all selection... this is a hint it might not be good design.

There are only 2 menu items currently, I don't see an issue with having:

Left of Selected
Right of Selected
Left of Active
Right of Active

Peter Fog (tintwotin) added a comment.Oct 25 2019, 1:03 AM

This comment was removed by Peter Fog (tintwotin).

Consider documentation for the new behaviour - if the documentation for this needs statements to explain how to get certain functionality by selecting one, then removing all selection... this is a hint it might not be good design.

Ok, in this perspective I see you're right about this being a bad design.
At this point I would personally completely remove that logic, and cancel the operation if nothing is selected.
I'll wait for @Peter Fog (tintwotin) and @Richard Antalik (ISS) opinion on having it exposed separately. It should also be considered that there is already an accepted patch that exposes the "both sides" option of the operator, to select the whole channel, so the final menu entries would be six, and not four in that case.

In D6127#140932, @Peter Fog (tintwotin) wrote:

I don't know if this is related to this patch, but running a build with this patch, selecting left&right channel, it selects correctly the channels containing selected strips, however I noticed that it always selects extended(thought shift isn't pressed) when first selecting left and then right.

That's because the moment you select to the left you end up with strips selected to the most left one, and that is the one used as reference if you run again the operator to select to the right.
The way to invert the direction is to use the redo-panel after the operator ran the first time.

Use static array for the channels list
Changed logic to select_active_side():
- use the strip's end frame as reference for selecting to the left
- use "less/greater or equal than" logic to allow selecting strips that start (on end) on the same frame (this required a small change in select_exec in the Alt+Click mode to maintain the same behaviour as before)

Alessio Monti di Sopra (a.monti) marked 2 inline comments as done.Oct 25 2019, 11:39 AM

It should also be considered that there is already an accepted patch that exposes the "both sides" option of the operator, to select the whole channel, so the final menu entries would be six, and not four in that case.

Checked this patch and think this isn't a very convenient way to select an entire channel (replied there D6107: VSE: Expose existing Select Channel Both(directions) operator in Menu ).

Campbell Barton (campbellbarton) added inline comments.Oct 29 2019, 10:03 PM

source/blender/editors/space_sequencer/sequencer_select.c
943–953	There was some misunderstanding, you can avoid a search by doing `seq->machine` as an index into an array which updates extents.

Alessio Monti di Sopra (a.monti) added inline comments.Oct 30 2019, 12:24 AM

source/blender/editors/space_sequencer/sequencer_select.c
943–953	Ok, so I'd basically "flag" the array element (something like this): if (seq->flag & SELECT) { ch_list[seq->machine] = -1; } } And then loop till the last index? for (int i = 0; i < MAXSEQ + 1; i++) { if (ch_list[i] == -1) { select_active_side(ed->seqbasep, side, i, side == SEQ_SIDE_LEFT ? r_ref : l_ref); } (This seems to work fine, not sure if it's exactly what you meant.)

This just tracks which channels contain selection.

Exactly how you implement is up to you, I thought it would make sense to store the ranges, after that selecting everything in-front/behind simply needs to check against this array.

In D6127#141624, @Campbell Barton (campbellbarton) wrote:

This just tracks which channels contain selection.

Exactly how you implement is up to you, I thought it would make sense to store the ranges, after that selecting everything in-front/behind simply needs to check against this array.

Yes sorry, was tired and I didn't think it could be confusing, I had simply removed the lines between those ones, to keep the comment focused on the array.
Thanks btw for the quick feedback!

Peter Fog (tintwotin) removed a subscriber: Peter Fog (tintwotin).Oct 30 2019, 12:22 PM

I thought some time about keeping both active and selected strips options for the operator, and in the I think it's better to remove the old behaviour in favour of only applying the selection on the selected strips' channels.
This is both to avoid confusion in the menus and also since it's really easy and fast to get the same old behaviour by Alt clicking on the active strip's handles.

Renamed the operator in Select Side
Updated logic to avoid unnecessary for loops
Cancel the operator if nothing is selected
Added a "Both Sides" option in the menu (I think this is necessary at least until a better solution is found, otherwise one could only access this function in the redo panel)

The min-max are tracked across all channels, this gives not very useful results for this example

untitled.blend395 KBDownload

I would expect this selection to be done per channel.

To be clear, matching the existing functionality if each selected strip was first made active, then operated on.

Campbell Barton (campbellbarton) requested changes to this revision.Oct 30 2019, 6:32 PM

This revision now requires changes to proceed.Oct 30 2019, 6:32 PM

use separate reference frames per channel

Campbell Barton (campbellbarton) mentioned this in rBddf20fae4c0e: Sequencer: use all selected strips for select side operator.Oct 31 2019, 10:53 PM

Thanks, committed rBddf20fae4c0e9552895a5ce097d568dc7a30c869 with some simplification

This revision is now accepted and ready to land.Oct 31 2019, 10:54 PM

Campbell Barton (campbellbarton) closed this revision.Oct 31 2019, 10:54 PM

@Campbell Barton (campbellbarton) Thanks for committing and for taking your time looking at this! I for sure learnt something new :)
Was actually looking for something like copy_vn_i() in BLI_array_utils earlier, with no luck obviously

Two small things I noticed in the commit:

I think that minus in -INT_MIN was by accident? It gives a compiler warning.
the description of the new switch is the same as the other one and says it's used for mouse selection, while it's not.