Page MenuHome

VSE Python UI template rework
AbandonedPublic

Authored by Richard Antalik (ISS) on Jan 12 2019, 8:58 PM.

Details

Summary

Patch origin: https://github.com/tin2tin/blender_vse_reworked
authors: @Peter Fog (tintwotin) and @hudson barkley (snuq)

I have set up notifications on tintwotin and snuq repository, but if I forget to include some change please do warn me.


  • SEQUENCER_OT_CrossfadeSounds
    • When I was at it, I rewrote this one to support crossfading of chains of strips
    • As a user I could imagine some visual flagging of non-conforming element rather then only error message (just an idea)
    • In case of complete overlap should we do fade in and out on overlapped strip?
    • TODO: fades should be logarithmic with audio
    • TODO: crash on ceated keyframe remove in curve editor
  • SEQUENCER_OT_CutMulticam
    • not tested
  • SEQUENCER_OT_SelectStripsUnderPlayhead
    • Removed lock check - must be handled (idealy only) in C
  • SEQUENCER_OT_SelectChannelStrips
    • Not good approach IMO.
    • Can be implemented as suggested by select "groupped" or trigger by keyboard + mouse cursor position.
    • Much better would be some little area on side with mute and solo buttons possibly collapse feature, drag'n'drop reorganisation and stuff like that (future implementation?)
  • SEQUENCER_OT_SetPreviewRange
    • There is already operator for this. I would rather modify keymap / existing operator(s)
    • Deleted
  • SEQUENCER_OT_DeleteLift
    • Identical to delete operator(not counting honoring lock) - what is it supposed to do?
  • SEQUENCER_OT_CutAndDelete
    • Depending on function of SEQUENCER_OT_DeleteLift - modify / remove lift functionality
    • It is "compatible upgrade" of cut operator.
  • SEQUENCER_OT_RippleDelete
    • TODO: Collision handling
  • SEQUENCER_OT_ZoomVertical
    • This should be possible to do natively with scroll + KB modifier... For now I will keep this and look at preferences / v2d settings if I can set this up.
  • SEQUENCER_OT_Move
    • What about moving to nearest snap points(another strip start, end, markers, channels etc) in desired direcion? I got this coded, can move large blocks, but need better collision handling(need this anyway, so why not...)
  • SEQUENCER_OT_MatchFrame
    • Collision detection could be more intelligent to save space(may throw something high up and user may forget about it)
    • TODO: Collision handling?
    • Needs better name
  • SEQUENCER_OT_Concatenate
    • Must work only on selection
    • TODO: Collision handling
    • I broke this, because at last moment I decided to add align param
  • SEQUENCER_OT_SplitMode
    • TODO: Allow to scroll / zoom while cutting
  • TODO: lint pep8

Up to discussion

  • How to handle collisions?
  • What should be behavior of strip lock?
  • C Vs Python

Diff Detail

Repository
rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
release/scripts/startup/bl_operators/sequencer.py
370–373

If the purpose is to have faster playback speed, perhaps disabling them should be a setting for that instead. Backing up all the values and then restoring them is not safe either if the user then makes changes in the meantime.

Changing settings that affect the final render in order to get a faster preview is problematic in general.

376–378

If you want the rename the modifier setting from mute to something else, that can be considered. But it needs to be done everywhere then, not halfway.

Hide/show have a specific meaning in Blender, which is to temporarily hide something for the purpose of editing, not something that affects final renders. "Enable/Disable", "Display", "Render" would be more consistent for this kind of property.

431–432

Don't name menu items after available shortcut keys, use the name of the property they affect.

1042

It's appending the mouse location to the end of a list, and then immediately reading that last item in execute. The mouse coordinate can just be passed in directly instead

The purpose of execute is to run the operator without user interaction. It could be left out here entirely. If it's intended to be there, there should be an operator property to specify the frame. This is can then be set by invoke.

1064

Same as explained before, I don't think backing up the previous state is reliable either.

It's better to change the underlying code than to try to hack around it in Python. For addons it may be ok but for official Blender functionality we need to do better.

release/scripts/startup/bl_ui/space_sequencer.py
503

It is a bit, though consistency is important. If we change this it should be all over.

518

Maybe "At Current Frame" or "Under Playhead".

550

The default video editing workspace has a timeline editor open, I don't think it's hard to find that.

I think this should only have sequencer specific operators.

708

For some reason this:

def sel_sequences(context):
    return len(getattr(context, "selected_sequences", ())) 

selected_seq = sel_sequences(context)

Got replaced with:

if context.selected_sequences:
    selected_seq = len(context.selected_sequences)
else:
    selected_seq = 0

I'm saying the new code is likely wrong.

release/scripts/startup/bl_operators/sequencer.py
213

Originally there was an operator for adding deinterlacing to the selection. I simply copied that concept to the rest of the strip settings, so I did not initiate it.

As I can't code in C, I can't solve the problem of the visibility of the active strip. And I do not have the power to redesign editing concepts of the VSE. So do you want me to remove those (IMOH very useful) functions, or should we let them stay until someone has the time to do a proper design/implementation?

370–373

Should the function stay or go?

1064

Should the function stay or go?

release/scripts/startup/bl_operators/sequencer.py
213

I've fixed the active strip drawing to be more distinct from the selected ones now, matching 2.7.

I don't mind too much having this operator if it's particularly useful, we should just avoid doing too much working around issues without addressing the underlying cause.

370–373

In its current state I think it should go.

1064

In its current state it should go, since it's quite misleading to make it seem like this is just a view operation.

@Brecht Van Lommel (brecht) @Richard Antalik (ISS)
More fixes and comments here: https://github.com/tin2tin/blender_vse_reworked
Ex. View Channel Solo/All & Toggle All Modifiers have now been removed.
A lot has been fixed/addressed, but not yet everything.

Can someone help me update the code here? https://developer.blender.org/D4199
Is that possible without deleting the remarks?

release/scripts/startup/bl_operators/sequencer.py
277

Renaming:
SelectChannel > SelectChannelStrips
select_channel > select_channel_strips

370–373

Ok. I'll remove it. I agree that: "Changing settings that affect the final render in order to get a faster preview is problematic in general.". And I don't think this can be solved in a python operator.

But I hope that the function, at some point will be implemented in C like bpy.context.scene.use_audio (ex. bpy.context.scene.use_modifiers).

Or with the reworked cache, it won't be necessary at all.

376–378

"Enable/Disable" would be more simple to use instead of mute/hide/unmute/unhide - however, the Scene Strip and Meta Strip properties both have video and audio and maybe they the video and audio somehow need to be separated? So you can only have the audio or the video from Scene/Meta strips? But I guess both values can have similar looking enable/disable buttons?

Should I rename all mute/hide stuff to enable/disable in the VSE UI?

403

Removed.

431–432

Ok.

1064

Ok. It has been removed. Hope this can be implemented in C at some point, because with this function you can have all your source material stacked in the sequencer and only view/copy from one channel into the edited material in the same sequence. Sort of 3 point editing from and to the same sequence.

release/scripts/startup/bl_ui/space_sequencer.py
178

Remove?

503

What do you want me to do here?

518

The "Playhead" is only the name of the menu. The actual selection is: "Current Frame".

For me, the menu naming becomes a sentence ex. "Select > Playhead > Current Frame."

I feel this is understandable, but what would you prefer?

550

The default 3D Layout also has a View > Play Animation menu entry, even though there is a timeline in that window too.

I guess the main question is if the should be a navigation menu at all, because if yes, there should be a navigation menu, then it would be strange to have the Play menu entry in the View menu.

All of the VSE toolbox add-ons have menu-entries/buttons for prev/next cut/keyframe, so it is something people are looking for. That's one of the reasons to adding a navigation menu.

Btw. both the timeline and the VSE has a marker menu?

Maybe the Timeline should have the navigation menu instead? (With the VSE relevant options added if the window contains a VSE?)

708

Ok, I added the old code.

There was a problem with the "Add" menu not working if a new scene was created. I don't know if this bug is related to this code change?

@hudson barkley (snuq) Do you know about this?

release/scripts/startup/bl_operators/sequencer.py
213

Yes, now the active strip is much more visible, but darkening the selection, makes it feel less "active", than the unselected strips, and it is now confusable with the muted strips.

Current look:

For consistency and clarity, why not use the selection colors of the 3D View:

It would look something like this:

304

"All" has been removed from this and the following classes.

release/scripts/startup/bl_operators/sequencer.py
159

Can this be done more efficiently?

def execute(self, context):
    finished = False
    for s in context.scene.sequence_editor.sequences_all:
        if s.select and s.type == 'MOVIE':
            s.use_reverse_frames = True
            finished = True
    if finished:
         return {'FINISHED'}
    else:
         return {'CANCELLED'}
250

I have removed all try/except instances in the script.

Hello, sorry for lack of communication.

Thank you Peter for collaboration, Plan was (at least mine :), that I will capture current state and continue from that point by myself.

I will update files for this patch. As for my progress - I was reformatting keymap file, still have to compare 2 versions and remove items that has not changed.
I was planning to continue this work during the weekend as it can turn to manual labor, so during workdays I an happy when I can do few "one line" patches.

I will update files for this patch. As for my progress - I was reformatting keymap file, still have to compare 2 versions and remove items that has not changed.
I was planning to continue this work during the weekend as it can turn to manual labor, so during workdays I an happy when I can do few "one line" patches.

I think it is better to wait with the keymap until sequencer.py and space_sequencer.py are good to go, as the keymap may change with changes in the other two files. NB. though @Brecht Van Lommel (brecht) has invested a lot of time in evaluating the code, there still seems to be a good deal which hasn't been commented(and I fear that it isn't because the code is perfect). Maybe Brecht can let you know what still needs a first evaluation? It is not all remarks I'm skilled to solve myself. And as mentioned in another comment here, there are several open design questions, which someone with the right authority needs to decide on.

So the current status is that I've tried to fix all of the remarks I could, the rest needs either design decisions or better coding skills than mine. Could you help me uploading my fixed files here, so the fixed things can be checked as done? https://github.com/tin2tin/blender_vse_reworked

Though all keymaps seems to be or about to be converted to .py there are a lot of C coded key settings and mouse functions coded in this file: https://github.com/dfelinto/blender/blob/master/source/blender/editors/space_sequencer/sequencer_ops.c

Richard Antalik (ISS) edited the summary of this revision. (Show Details)Jan 17 2019, 12:24 AM
Richard Antalik (ISS) updated this revision to Diff 13242.

Updated patch based on latest version

Thank you for updating the files. I can see the try/except wasn't removed in my Github, but they are removed now.

Unfortunately, I can see that Brecht's comments haven't moved positions with the line number changes in the updated file, which makes it a bit hard to understand some of the comments. And also there have been changes(new stuff added) to the files outside of this patch(ex. GP stuff), do those changes have to be inserted by hand in my files or is there a way not to accept those changes(or apply them after this patch)?

Campbell Barton (campbellbarton) requested changes to this revision.Jan 17 2019, 3:35 AM
Campbell Barton (campbellbarton) added inline comments.
release/scripts/startup/bl_operators/sequencer.py
299–356

Think these operators could be removed and instead have these properties added to "Select Grouped" operator (Shift-G).

Then you can select all un-locked/un-muted too.

791

This seems to be a wrapper for Cut which handles selection differently?

This is confusing, we shouldn't have multiple tools that do nearly the same thing and give them different names.

1057

Should avoid magic numbers, also this is no longer the maximum frame in Blender.

This revision now requires changes to proceed.Jan 17 2019, 3:35 AM

I think it is better to wait with the keymap until sequencer.py and space_sequencer.py are good to go, as the keymap may change with changes in the other two files. NB. though @Brecht Van Lommel (brecht) has invested a lot of time in evaluating the code, there still seems to be a good deal which hasn't been commented(and I fear that it isn't because the code is perfect). Maybe Brecht can let you know what still needs a first evaluation? It is not all remarks I'm skilled to solve myself. And as mentioned in another comment here, there are several open design questions, which someone with the right authority needs to decide on.

Oh my.. Now I am looking at this thread, 100 inlines...

OK I will read through this and set this as a priority issue, which it actually is to be fair...

Looking further into split operators SEQUENCER_OT_SplitExtract and SEQUENCER_OT_SplitLift duplicates functionality, with only one line difference: sequencer.ripple_delete / sequencer.delete_lift. This could be made into a single operator.

release/scripts/startup/bl_operators/sequencer.py
299–356

Yes, that would be preferred, but the 'Grouped' menu seems not to be coded in python - or maybe I just can't find it? (It's not included in the space_sequencer.py file). Or in other words, I wouldn't know how to do what you're asking.

791

Well, yes. In the menus, I used the word "Cut" in the remove-sense eg. cut/copy/paste.

Split is the most commonly used word in NLEs for this operation. I guess the word for the similar 3D operation is knife - which may explain that the VSE shortcut is 'K'.

Not to be confused with the razor function - which in most NLEs are similar to what I call Split > Mode...
(which my previously linked gif showed the functionality of)

My implementation of the "Split" function differs from the Blender native"Cut" function in these ways:

  • Locked strips can't be split(the native "cut" will cut locked strips).
  • If the playhead is above one or more selected strips only they will be split.
  • If the playhead is above no selected strips and they aren't locked, the unselected strips will be split.

1057

This function is from the old KinoRaw add-on. How should this be done correctly?

Oh my.. Now I am looking at this thread, 100 inlines...
OK I will read through this and set this as a priority issue, which it actually is to be fair...

Most of those are fixed. And there has been some discussion too. :-)

Looking further into split operators SEQUENCER_OT_SplitExtract and SEQUENCER_OT_SplitLift duplicates functionality, with only one line difference: sequencer.ripple_delete / sequencer.delete_lift. This could be made into a single operator.

True. Will have to think about how to do a tooltip which fits in all cases. Would "Splits selected strips and cuts" work?

Looking further into split operators SEQUENCER_OT_SplitExtract and SEQUENCER_OT_SplitLift duplicates functionality, with only one line difference: sequencer.ripple_delete / sequencer.delete_lift. This could be made into a single operator.

This has been done now: https://github.com/tin2tin/blender_vse_reworked

@Peter Fog (tintwotin) I should say, that I am modifying these scripts, so please don't make very big changes.

@Peter Fog (tintwotin) I should say, that I am modifying these scripts, so please don't make very big changes.

Sorry, I didn't know that. My son was jumping around me when I updated the file, so I feel that there might be things unfinished. the main changes are the split-extract/lift/remove,operator, navigation - range-start/end menu. Will not do more for now.

@Richard Antalik (ISS), could you identify any of the changes here that'd be better done in C? (as TODO's in comments for eg).

In the case of extending "Select Grouped" this is something you could probably do even without much experience in C.
We can help you get started compiling Blender.

Otherwise, someone else can add this.

release/scripts/startup/bl_operators/sequencer.py
299–356

Are there other changes in this patch that would be better integrated into existing C operators?

This patch adds some C-operator wrappers in some places where it's probably better to extend the original operators instead.

@Richard Antalik (ISS), could you identify any of the changes here that'd be better done in C? (as TODO's in comments for eg).
In the case of extending "Select Grouped" this is something you could probably do even without much experience in C.
We can help you get started compiling Blender.
Otherwise, someone else can add this.

I have "no problem" with C code.

Also depends on if and what has to be done in C.
If it was up to me, I would rather keep C side as simple as possible(expose props, sanity checks, object factory + low level glue basically) and do "complex" operations in python.

  • As OOP, python is more suited to work with objects
  • Contribution base is larger
  • Maintenance is easier

You can have lot of data even in VSE but I can hardly imagine hitting performance limit due to opertators written in python (have seen https://github.com/mikeycal/vse-stopwatch-timer-for-blender IMO could be done by animating visibility + transform + speed or number of other possibilities)
If python operators are OK to create, I implementation of some feature-rich container for strips may quite simplify code in general.
As an example, soon after my first Blender(and python, so don't laugh :) experience I started to work on some "unified handling API" for bpy.
https://github.com/RichardAntalik/VideoTools/blob/master/__init__.py

OK now some rant about patch itself

Some operators were renamed / deleted
All were refactored.
labels / description fields updated
added default values to operator RNA props / params
modified poll methods to prevent execution, if requirements for execution are not met

SEQUENCER_OT_CrossfadeSounds

  • When I was at it, I rewrote this one to support crossfading of chains of strips
  • As a user I could imagine some visual flagging of non-conforming element rather then only error message (just an idea)
  • In case of complete overlap should we do fade in and out on overlapped strip?
  • TODO: fades should be logarithmic with audio
  • TODO: crash on ceated keyframe remove in curve editor

SEQUENCER_OT_CutMulticam

  • not tested

SEQUENCER_OT_SelectStripsUnderPlayhead

  • Removed lock check - must be handled (idealy only) in C

SEQUENCER_OT_SelectChannelStrips

  • Not good approach IMO.
  • Can be implemented as suggested by select "groupped" or trigger by keyboard + mouse cursor position.
  • Much better would be some little area on side with mute and solo buttons possibly collapse feature, drag'n'drop reorganisation and stuff like that (future implementation?)

SEQUENCER_OT_SetPreviewRange

  • There is already operator for this. I would rather modify keymap / existing operator(s)
  • Deleted

SEQUENCER_OT_DeleteLift

  • Identical to delete operator(not counting honoring lock) - what is it supposed to do?

SEQUENCER_OT_CutAndDelete

  • Depending on function of SEQUENCER_OT_DeleteLift - modify / remove lift functionality
  • It is "compatible upgrade" of cut operator.

SEQUENCER_OT_RippleDelete

  • TODO: Collision handling

SEQUENCER_OT_ZoomVertical

  • This should be possible to do natively with scroll + KB modifier... For now I will keep this and look at preferences / v2d settings if I can set this up.

SEQUENCER_OT_Move

  • What about moving to nearest snap points(another strip start, end, markers, channels etc) in desired direcion? I got this coded, can move large blocks, but need better collision handling(need this anyway, so why not...)

SEQUENCER_OT_MatchFrame

  • Collision detection could be more intelligent to save space(may throw something high up and user may forget about it)
  • TODO: Collision handling?
  • Needs better name

SEQUENCER_OT_Concatenate

  • Must work only on selection
  • TODO: Collision handling
  • I broke this, because at last moment I decided to add align param

SEQUENCER_OT_SplitMode

  • TODO: Allow to scroll / zoom while cutting

TODO: lint pep8

Up to discussion

  • How to handle collisions?
  • What should be behavior of strip lock?
  • C Vs Python

@Peter Fog (tintwotin) I will work on this a bit more, just a heads up, that there was commit to space_sequencer.py, so it was updated in Blender.

Sorry if I missed some inline and/or did modify anything in a way you don't want...

@Richard Antalik (ISS) Thank you for investing so much time in this. The "space_sequencer.py" doesn't seem to be updated to reflect the changes in "sequencer.py"? When I replace these two files in a fresh build of Blender the entire UI is broken. So I can't test your changes.

Looking at the code I was surprised to see that the split function was removed?

  • It did so much more than just mirroring the built-in cut function. Please see the inline comments and gif for more.

NB. moving strips adjoined to effect-strips(ex cross-fade) will cause errors, so this will have to be taken into account before doing the action.
(Effect-strips not having write-able start/end values and the detection of effect-strips(and adjoined strips), causes a lot of problems when trying to move/copy/duplicate. Actually, "duplicate" workes better than "copy" when only having part of a strip/cross-fade/strip combo selected).

The troublesome copy function in combination with ex. partly selected cross-fades is one of the reasons why I didn't implement insert + overwrite paste functions, though IMHO those are must have NLE functions. https://docs.google.com/document/d/1lVPxEgR5bnMr4jV-OkLifeU4v9Qp9x-eoCOWADn8u7I/edit

On the 'select all strips in channel' - I agree that this function would be better suited to be placed in a channel header, but as long the ex. NLA UI track headers aren't copied into the VSE - I can only think of this very basic implementation. If channel headers where implemented the playhead would mark the X point and the active channels would mark the Y point for operations, which would simplify where operations would be done. Currently, it is quite confusing for the users with operations only working on ex. active strip or selection or a very specific selection(ex. two movie strips) or mouse point without letting the user know what operation is depending on what selection. And having channel headers would simplify it. So you wouldn't have to always select stuff before an operation(which is unnecessary step in most NLEs). Ex. if you paste strips, only the playhead marks an X value, and the strips are inserted in the first empty channels upwards - if you could select active channels you could insert/overwrite into the selected channels pasting the strips from the buffer exactly into the place you want them.

@Richard Antalik (ISS) Thank you for investing so much time in this. The "space_sequencer.py" doesn't seem to be updated to reflect the changes in "sequencer.py"? When I replace these two files in a fresh build of Blender the entire UI is broken. So I can't test your changes.

I haven't got to space_sequencer.py yet. I will review that now.
you can use find (F3) or space in 2.79 and type in operator name to test operators

Looking at the code I was surprised to see that the split function was removed?

  • It did so much more than just mirroring the built-in cut function. Please see the inline comments and gif for more.

I removed this, because I don't agree with this behavior. What it implemented was some kind of indirect inverse selection mask, which is strange concept I have not seen anywhere.
Possible solutions to this:

  • build a selection mask, and use cut tool with Side: Both setting.
  • implement settings, that would allow user to configure cut to do exactly what you wanted originally.

Having said that, building such masks with default keymap is not very easy.
I use LMB + tweak for box select, no extend, Shift enables extend, Ctrl enables Deselect, which is I think most widely used layout.

@Peter Fog (tintwotin) can you clarify these also?

  • SEQUENCER_OT_Move
    • What about moving to nearest snap points(another strip start, end, markers, channels etc) in desired direcion? I got this coded, can move large blocks, but need better collision handling(need this anyway, so why not...)
  • How to handle collisions? What should happen when you do ripple delete and attached effect in upper channel would collide into some strip?
    • prevent execution and throw error?
    • move as far as possible?
    • configurable behavior?
  • What should be behavior of strip lock?
    • prevent any modification other then changing lock state?

Another thing, that I wanted to tackle was displaying and modifying properties of selection.

  • I was thinking about concept of active strip - my brain refuses to accept it's usefulness. Maybe because active strip is completely independent of selection.
  • Alt + tweak value and alt + enter seems to work pretty well so nothing to do here I guess

Displaying properties of selection could be done better IMO.

  • Ideally you want to display all properties of all types. Logical step would be group properties based on strip type
    • use tab per type, so you can immediately see what types are selected and switch between them?
    • Not sure, if we can show only values of fields that would be same for all selected strips of the same type, otherwise they could be blank or display some label like "varies".
  • when I change active strip to strip of different type(from movie to sound), panel organisation is quite inconsistent.
    • Don't know what we can do about this...

@Richard Antalik (ISS), could you identify any of the changes here that'd be better done in C? (as TODO's in comments for eg).
In the case of extending "Select Grouped" this is something you could probably do even without much experience in C.
We can help you get started compiling Blender.
Otherwise, someone else can add this.

I have "no problem" with C code.
Also depends on if and what has to be done in C.
If it was up to me, I would rather keep C side as simple as possible(expose props, sanity checks, object factory + low level glue basically) and do "complex" operations in python.

  • As OOP, python is more suited to work with objects
  • Contribution base is larger
  • Maintenance is easier

You can have lot of data even in VSE but I can hardly imagine hitting performance limit due to opertators written in python (have seen https://github.com/mikeycal/vse-stopwatch-timer-for-blender IMO could be done by animating visibility + transform + speed or number of other possibilities)
If python operators are OK to create, I implementation of some feature-rich container for strips may quite simplify code in general.

Nevertheless, adding new operators isn't ideal - when a few options can be added to existing operators.

Could you submit a separate patch that adds options to select grouped?

There are probably other cases where you can extend existing operators, although I couldn't say for sure - we should at least look into doing this for operators you're adding that only change functionality.

If you want to choose a new direction for how the sequencer is developed, this should be done separately from reworking the UI.

As an example, soon after my first Blender(and python, so don't laugh :) experience I started to work on some "unified handling API" for bpy.
https://github.com/RichardAntalik/VideoTools/blob/master/__init__.py
OK now some rant about patch itself
Some operators were renamed / deleted
All were refactored.
labels / description fields updated
added default values to operator RNA props / params
modified poll methods to prevent execution, if requirements for execution are not met
SEQUENCER_OT_CrossfadeSounds

  • When I was at it, I rewrote this one to support crossfading of chains of strips
  • As a user I could imagine some visual flagging of non-conforming element rather then only error message (just an idea)
  • In case of complete overlap should we do fade in and out on overlapped strip?
  • TODO: fades should be logarithmic with audio
  • TODO: crash on ceated keyframe remove in curve editor

SEQUENCER_OT_CutMulticam

  • not tested

SEQUENCER_OT_SelectStripsUnderPlayhead

  • Removed lock check - must be handled (idealy only) in C

SEQUENCER_OT_SelectChannelStrips

  • Not good approach IMO.
  • Can be implemented as suggested by select "groupped" or trigger by keyboard + mouse cursor position.
  • Much better would be some little area on side with mute and solo buttons possibly collapse feature, drag'n'drop reorganisation and stuff like that (future implementation?)

SEQUENCER_OT_SetPreviewRange

  • There is already operator for this. I would rather modify keymap / existing operator(s)
  • Deleted

SEQUENCER_OT_DeleteLift

  • Identical to delete operator(not counting honoring lock) - what is it supposed to do?

SEQUENCER_OT_CutAndDelete

  • Depending on function of SEQUENCER_OT_DeleteLift - modify / remove lift functionality
  • It is "compatible upgrade" of cut operator.

SEQUENCER_OT_RippleDelete

  • TODO: Collision handling

SEQUENCER_OT_ZoomVertical

  • This should be possible to do natively with scroll + KB modifier... For now I will keep this and look at preferences / v2d settings if I can set this up.

SEQUENCER_OT_Move

  • What about moving to nearest snap points(another strip start, end, markers, channels etc) in desired direcion? I got this coded, can move large blocks, but need better collision handling(need this anyway, so why not...)

SEQUENCER_OT_MatchFrame

  • Collision detection could be more intelligent to save space(may throw something high up and user may forget about it)
  • TODO: Collision handling?
  • Needs better name

SEQUENCER_OT_Concatenate

  • Must work only on selection
  • TODO: Collision handling
  • I broke this, because at last moment I decided to add align param

SEQUENCER_OT_SplitMode

  • TODO: Allow to scroll / zoom while cutting

TODO: lint pep8
Up to discussion

  • How to handle collisions?
  • What should be behavior of strip lock?
  • C Vs Python
  • TODO's relating to operators could be moved into the code, they are more likely to get forgotten about if left as comments here.

    Use convention: # TODO(yourname): What needs doing.
  • I've added this list into the main task, so you can tick items off once done and they're not forgotten about.

This patch is getting to big to be able to review usefully.

Suggest splitting it up, eg: obvious improvements to the UI can go in separately (doesn't need much review).

More opinionated changes about how the sequencer works can then be a smaller patch with fewer changes.

Campbell Barton (campbellbarton) requested changes to this revision.Jan 20 2019, 10:39 PM

Poll should never be accessing collections/selection, it's called on drawing and will slowdown playback if it needs to build a list of many items only to check if a button should be greyed out.

Access the active strip is enough in most cases.

Also, there are operators that just change toggles, these can probably be removed since you can right click and copy-to-selected or hold Alt while clicking.

Noted inline.

release/scripts/startup/bl_operators/sequencer.py
43

Just check if the active strip is a sound in this case.

122

Not sure why this is needed, you can change a property for all selected sequences at once.

132

Accessing selection.

141

Again, not sure why this is needed.

171

Accessing selected sequences for poll.

191

Accessing selection.

201

Not sure why this is needed.

211

Accessing selected sequences for poll.

285

Accessing selected sequences for poll.

308

Accessing all sequences for poll.

330

Accessing all sequences for poll.

482

Accessing selected sequences.

572

Accessing selected sequences for poll.

624

Accessing selected sequences for poll.

679

Accessing selected sequences for poll.

728

Accessing selected sequences for poll.

820

Accessing all sequences for poll.

This revision now requires changes to proceed.Jan 20 2019, 10:39 PM
  • SEQUENCER_OT_Move
    • What about moving to nearest snap points(another strip start, end, markers, channels etc) in desired direcion? I got this coded, can move large blocks, but need better collision handling(need this anyway, so why not...)

Ctrl+drag playhead/strip will snap to next cut, so snapping with Ctrl pressed would be in consistency, but the default move should be without snapping.

  • How to handle collisions? What should happen when you do ripple delete and attached effect in upper channel would collide into some strip?
    • prevent execution and throw error?
    • move as far as possible?
    • configurable behavior?

When pasting, stips will be inserted into the first free channels, so maybe when strips are colliding they should be moved upwards into the first free channel(but only if it is effect-strips).

  • What should be behavior of strip lock?
    • prevent any modification other then changing lock state?

Yes, I think so too.

Another thing, that I wanted to tackle was displaying and modifying properties of selection.

  • I was thinking about concept of active strip - my brain refuses to accept it's usefulness. Maybe because active strip is completely independent of selection.

Maybe all select-operators should ensure that the active strip is always the last selected strip, which it is when you select manually. The question is if you deselect all also should deselect the active strip(and close the strip properties panel?). That would be an understandable behavior, but I don't know if it will be annoying to have the panel open and close all the time?

  • Alt + tweak value and alt + enter seems to work pretty well so nothing to do here I guess

I feel that info about the shift/ctrl/alt functions should be in the status bar and context-sensitive, so ex. when hovering over a checkbox is should say +alt will add values to entire selection(if that is not the default behavior).

Displaying properties of selection could be done better IMO.

  • Ideally you want to display all properties of all types. Logical step would be group properties based on strip type
    • use tab per type, so you can immediately see what types are selected and switch between them?
    • Not sure, if we can show only values of fields that would be same for all selected strips of the same type, otherwise they could be blank or display some label like "varies".

I don't know if this is too much trouble for now? How does the 3D View deal with selections containing different types?

  • when I change active strip to strip of different type(from movie to sound), panel organization is quite inconsistent.
    • Don't know what we can do about this...

Yeah, the strips have quite individual settings, which makes it hard to avoid panels jumping around when changing type.

Regarding the C core and Py shell idea: if we can contribute operators in Python, we'd work directly on Blender instead of our add-on Power Sequencer with my teammate Razvan. I'm in the middle of a Kickstarter campaign now so too busy to help, unfortunately, but we do all of our work with the VSE so we'd gladly contribute directly to Blender itself. 🙂
Thanks a lot for your work everyone!

  • SEQUENCER_OT_Move
    • What about moving to nearest snap points(another strip start, end, markers, channels etc) in desired direcion? I got this coded, can move large blocks, but need better collision handling(need this anyway, so why not...)

Ctrl+drag playhead/strip will snap to next cut, so snapping with Ctrl pressed would be in consistency, but the default move should be without snapping.

Problem is setting arbitrary step - 1 would be OK because you will always achieve desired result, but slow. 25 is problematic, because what if you want to move by 15?

  • How to handle collisions? What should happen when you do ripple delete and attached effect in upper channel would collide into some strip?
    • prevent execution and throw error?
    • move as far as possible?
    • configurable behavior?

When pasting, stips will be inserted into the first free channels, so maybe when strips are colliding they should be moved upwards into the first free channel(but only if it is effect-strips).

Ok we can always undo if not happy with the result

  • What should be behavior of strip lock?
    • prevent any modification other then changing lock state?

Yes, I think so too.

Ok, but this will require modifying a bunch of C stuff, so this will take a while. If this is not priority issue I could see time better spent on other stuff

Another thing, that I wanted to tackle was displaying and modifying properties of selection.

  • I was thinking about concept of active strip - my brain refuses to accept it's usefulness. Maybe because active strip is completely independent of selection.

Maybe all select-operators should ensure that the active strip is always the last selected strip, which it is when you select manually. The question is if you deselect all also should deselect the active strip(and close the strip properties panel?). That would be an understandable behavior, but I don't know if it will be annoying to have the panel open and close all the time?

Now I know why this bugs me so much - it violates select & modify workflow with box select
First you have to select(activate) type, that has property you want to modify, then make actual selection and finally modify.

I have looked at 3D viewport and default action on LMB+tweak is box select,
in edit mode clicking on background / box select will clear original selection,
in object mode there is concept of active element so that behaves as VSE

Also we can still use panel, when nothing is selected. Not sure for what, but we can :)

Displaying properties of selection could be done better IMO.

  • Ideally you want to display all properties of all types. Logical step would be group properties based on strip type
    • use tab per type, so you can immediately see what types are selected and switch between them?
    • Not sure, if we can show only values of fields that would be same for all selected strips of the same type, otherwise they could be blank or display some label like "varies".

I don't know if this is too much trouble for now? How does the 3D View deal with selections containing different types?

Not too much, but same issue - it violates select & modify workflow

  • Alt + tweak value and alt + enter seems to work pretty well so nothing to do here I guess

I feel that info about the shift/ctrl/alt functions should be in the status bar and context-sensitive, so ex. when hovering over a checkbox is should say +alt will add values to entire selection(if that is not the default behavior).

Not sure - this information would how on every editable element, so it would be kinda redundant.
On the other hand it is quite essential information - perhaps it should be shown in status bar, where LMB icon is

@Campbell Barton (campbellbarton)
OK I will create new diff for operators.

@Nathan Lovato (gdquest)
It is possible to add power sequencer as official addon, not sure how a review is done in such case.
For complete integration, development strategy should be discussed. There are some key playback issues in the core, so I want to resolve those first.
Good luck with your KS :)

I was looking at menu items,
Biggest problem are operators - many of them can be run with properties, that would contradict their description, or depends on selected sequences, etc...

I am of opinion, that unless you can access feature from default menu / panel, it's as if it doesn't exist...
Only way I currently see _all_ operators presented to user is by making list in side panel where you can set all available properties. First I will have to check if this is possible.

In menu there should be only minimal set of reasonable functions, that a new user would look at (only to see assigned shortcut anyway).

Also I would agree with Brecht, that things like move cursor forward/backward is basically a waste of space. We don't want to teach users, how to use computer :)

I feel that info about the shift/ctrl/alt functions should be in the status bar and context-sensitive, so ex. when hovering over a checkbox is should say +alt will add values to entire selection(if that is not the default behavior).

Not sure - this information would how on every editable element, so it would be kinda redundant.
On the other hand it is quite essential information - perhaps it should be shown in status bar, where LMB icon is

On Windows default behavior +Shift selects between all elements between first and last selection. +Ctrl extends selection pr. selected element. +Alt for batch adding values to all selection had I never heard about before Blender. In other words, none of these functions are the same inside and outside of Blender, so I think the users should be made aware of them.

Shift/ctrl/alt + select do have several functions (hidden) functions in the sequencer, and info about these should be added to the status-bar(which can't be added through python?) Click to enlarge the gifs:

Alt:

Ctrl(inside strips):

Ctrl(outside strips):

I was looking at menu items,
Biggest problem are operators - many of them can be run with properties, that would contradict their description, or depends on selected sequences, etc...

Yes, the inability to specify tooltips/descriptions outside of the operators are limiting the UX.

I am of opinion, that unless you can access feature from default menu / panel, it's as if it doesn't exist...

Agree.

Only way I currently see _all_ operators presented to user is by making list in side panel where you can set all available properties. First I will have to check if this is possible.
In menu there should be only minimal set of reasonable functions, that a new user would look at (only to see assigned shortcut anyway).

Well, I tried to do the opposite. To add as many as possible to the menus, but trying to avoid clutter(like in the old Strip menu) with the long term purpose to clean up the really, really cluttered panels(which I haven't). It seems (to me) that this is also how menu vs. panel are divided in the 3D View. However, instead of making a few very long menus(like the 3D View), I've made them short and applied a shortcut to each of them so they work as popups at mousepointer helper-menus(previously it was only the add menu which had a shortcut). And when you're in the process of editing in the VSE you should not have to have the panel open at all, so all operators manipulating strips should be accessible from the menus & shortcuts.

Also I would agree with Brecht, that things like move cursor forward/backward is basically a waste of space. We don't want to teach users, how to use computer :)

Well, the view menu in the 3d view also has a 'play animation' entry - so it is in consistency to have a play menu entry. And as you stated yourself menus are for learning the shortcuts. And on top of this I personally think that there should be many more navigation options - like playback speed etc. @hudson barkley (snuq) implemented some of them in his VSE add-on: https://www.facebook.com/groups/1981104845289453/permalink/2034552476611356/
And a long term wish is to have the navigation functions mapped to ex. grap/trim-mode functions, so you could ex. select a handle play in various speeds(through navigation (shortcut) controls) with the strip extending until you find the frame you want to end the cut and okay it. These navigation functions could also influence ex. the move-mode or slide-mode.

I could not resist to do a little experiment of jumpy panel: https://photos.app.goo.gl/dXQyg23Z5sTzJUVH6

This patch has turned into devtalk.
Sorry, I wasn't there, when you were working on this.

I have decided to continue with operators for now - some of them are almost essential
Delay work on panels and menus for later - UI is not unusable now. To finish this I would have to rewrite basically a whole patch. Ideally read thread where this was discussed to justify layout and other inconsistencies between other editors.

I would close/archive this unless anybody strongly disagree with my decision

I'm must confess that I'm not ready to give up on this yet... I've updated the space_sequencer.py according to the renaming in your version of the operator file:
https://github.com/tin2tin/blender_vse_reworked/blob/master/UI_fix_space_sequencer.py
If you want to edit the space_sequencer.py file from the Blender text-editor, several additional files needs to be loaded into the text editor to avoid errors. They are opened in this .blend:
https://we.tl/t-uWQSc01EbV

This is how the menus are looking now(click to open the gif):

NB. the panels are also converted to 2.80 right-aligned style in this space_sequencer.py(so this file does not only contain changes in the menus).

I haven't touched the sequencer.py so there are still the mentioned problems with the polls.

I'm must confess that I'm not ready to give up on this yet...

Nobody is giving up, What I am saying is that this will require some more back and forth

I expected to resolve few formalities at most and commit to master. However there is quite some work to be done.

You can look at https://developer.blender.org/project/28/item/view/311/
There are not that many bugs(lot of them kinda invalid) - these should be fixed in 2.80
Also a lot of nice features - some are ready more than others, some are more important than others. I can not integrate all of them in this release unfortunately.

Testing the various operators:

SEQUENCER_OT_SelectStripsUnderPlayhead:

if method == 'STRIPS' or method == 'BOTH':

Must be:

if self.method == 'STRIPS' or self.method == 'BOTH':

The select handles options are in a select > handle menu and maybe not needed inside this operator too?

(Selecting adjoined handles, couldn't I find a way to do in python, but this would be a very useful feature)

SEQUENCER_OT_SelectChannelStrips:
Selects now only strips in the channel containing the active strip - I would prefer that it selected strips in all channels containing selected strips(as it was).

SEQUENCER_OT_ExtendToFill:
Is broken now.

SEQUENCER_OT_PreviewAndCutMode:
Doesn't limit cuts to the selected strips, if there are selected strips under the playhead.

SEQUENCER_OT_CutAndDelete:

bpy.ops.sequencer.cut(frame=scene.frame_current, type=self.type, side=self.direction)
if self.ripple_delete:

Should now be:

bpy.ops.sequencer.cut(frame=scene.frame_current, type=self.type, side=self.**side**)
if self.**method**:

SEQUENCER_OT_Concatenate:
Is now broken.

Currently this patch is getting too big to properly review.

It seems this is a dump of a branch that makes opinionated changes, that should be reviewed and applied separately.

  • Prevent locked strips from being edited.

    If this is desired behavior, this should be a change made at a lower level in C.
  • Change behavior of cutting.
  • Operators to toggle options (which seem redundant).
  • Operators to select grouped (which can done by extending existing operator).

Suggest to close this patch and submit as smaller changes.

  • Basic UI layout changes, these can likely be committed easily.
  • Change behavior of locked option to prevent edits (currently only prevents transform).

    Although we should check with users if this is even desirable.
  • New operators - could be separate patches, grouped by type - all split operators, all selection operators... etc.

Currently this patch is getting too big to properly review.

True.

@Peter Fog (tintwotin) Thanks, for tests.

I am preparing as harmless version as possible. Please be patient :)
I will get increasingly radical...