Page MenuHome

VSE Python UI template rework
Needs RevisionPublic

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

Details

Summary

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

Questions for authors:

  • Should this patch be applied without any changes to "video editing" startup blend file?
  • Can you please provide modified designated keyboard mapping file (in [release\]scripts\presets\keyconfig\keymap_data) or list changes from default?

There was whitespace noise left over so I cleaned that.
please replace(should be latest version) following files in your repo by files below

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

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
158

Should this be s.use_reverse_frames = not s.use_reverse_frames?

159

I guess a lot of the existing operators have this problem, but strictly speaking operators should not return 'FINISHED' unless they actually changed something, to avoid unnecessary undo pushes.

It's not a big issue though.

176

Same comment as above.

And also, why only movie strips and not images?

194

Same comment as above.

213

Code style: don't use () here. Also can be simplified to strip.show_waveform = not strip.show_waveform.

These operators also seem to be compensating for missing easy editing of all selected strips. In the properties editor we have alt + click to edit all selected objects rather than just the active one, ideally the sequencer would support something similar rather than having these kinds of operators.

221

Operator class name does not match label.

225

Label is not clear, should say that it selects (strips) at the current frame.

The idname says time cursor, label says current frame, I see no reason for that inconsistency.

228

extent -> extend

It also should be a boolean rather than enum with true/false.

238

Add space around == , leave out is not None.

241

Code style: don't use camel case.

250

Using try / except here seems dodgy, maybe hasattr is better. It's not unclear from a quick glance at the code which exception it's supposed to be catching.

274

Don't capitalize like this in a description

277

Name is unclear

289

Why sort, and what does frame_final_start have to do with this?

For efficiency I guess it should make a set of the channels that selected sequences are in.

296

strip.channel == s.channel here is always True.

304

No need for "All" in the name here I think.

370–373

Why not always apply to the selection only? The menu item affects unselected strips, which is unexpected.

As with the solo view operator, I'm not sure this is an entirely good idea.

376–378

It's not hide/show, it's mute/unmute.

403

There seems to be no reason for this to be an operator, if it's just toggling on property which could be in the menu directly.

431–432

Why use "in" and "out" instead of "start" and "end"?

1020

This description does not explain what the operator does, and it's not clear to me from the name.

Further, the description should not include information about the meaning of shortcuts. That is to be displayed in the status bar when the operator is running.

1042

I see no reason to store a mouse path array, or any mouse location at all.

1064

This operator seems like it works like local view in the 3D viewport, where you can just look at an individual strip and then go back.

But trying to use it in a more complex setup can break things if strips were intended to muted more permanently. So I'm not sure if it's really a good idea to have an operator that works this way. And if it is useful, it should not be exposed as if it was just a View manipulation.

I wonder if changing the Channel in the preview header is a better solution for this use case.

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

Don't leave commented out code like this.

174

The term "Viewport" does not really make sense here? I guess "Preview" is makes more sense in this context.

178

This doesn't seem to belong here. Why have final audio rendering here along with preview render? Just leave it to the Render menu at the top.

191

Please make this consistent with other editors like the image editor?

Pull zoom in/out/border out of the submenu into, and only have a Fractional Zoom submenu.

324

Don't leave commented out code.

328

Use prop_menu_enum here, this looks weird now.

Also convention would be to grey out the menu rather than hide it if show_seconds is false.

503

Please look at similar Select menus in other editors, like the 3D viewport, and the same item order and naming.

518

This name is a bit confusing because it's not selecting the playhead, whereas other menu items indicate what is being selected.

522

I'm not sure this menu needs to exist at all, we don't have similar ones in other editors and it doesn't actually work since the mouse will not be in the right location when clicking the menu item. The keymap hints in the status bar are intended for this.

527

Just name this "Muted"

791

Don't think this needs to be a submenu, the top level menu is not that long.

862

It should just be "Mute", a single feature does not need two names.

"Lock/Mute" only made sense since the submenu had two different operations in it.

1313–1314

For consistency don't put the text outside operator buttons, rather make them use the full width and put it inside the button.

Brecht Van Lommel (brecht) requested changes to this revision.Sun, Jan 13, 12:51 AM
This revision now requires changes to proceed.Sun, Jan 13, 12:51 AM

Thank you for this, @Richard Antalik (ISS)
The changes should work without changing the default VSE layout.

I've downloaded your files, cleaned them up some more and uploaded them here: https://github.com/tin2tin/blender_vse_reworked

There is also a shortcut key file: https://github.com/tin2tin/blender_vse_reworked/blob/master/vse_keymap.blend.py
(I'm not too familiar with the general Blender shortcut keys, so I hope someone can run through them to check if they're breaking some consistency stuff. NB. there are a few disabled default shortcut-keys because they are replaced by new operators(in sequencer.py) which will either let the function work on the entire selection or add more advanced features(like the split function), and then re-mapped to the original shortcut-key )

There are several changes to evaluate in this patch:

  1. The menus are now shorter and work as popup-menus at the mouse cursor point. The original design doc on ex. why devide the "strip" menu? https://docs.google.com/document/d/1lVPxEgR5bnMr4jV-OkLifeU4v9Qp9x-eoCOWADn8u7I/edit - not implemented: insert & overwrite
  2. Are the naming of the functions ok and in consistency with Blender lingo?
  3. Buttons added to header.
  4. Preview > Zoom menu changed.
  5. 15 "hidden" functions are made visible in the menus.
  6. 24 additional operators/functions have been added.
  7. The sidebar panel has been right aligned(2.80 style)
  8. The colorwheels in the sidebar panel will align themselves side by side if the panel is wide enough.
  9. The sidebar > strip data is new and contains values in both frames and in smpte. Width alignment will change depending on width of values/stings.

A discussion about the design and implementation of the VSE REworked project can be found here: https://devtalk.blender.org/t/the-video-sequence-editor-reworked-a-ready-to-patch-suggestion/3264

Running through the code to clean it up, I found a few things which needs fixing. I'm not a coder and these things I'm not able to fix myself:

In "Space_sequencer.py":

  • Select > "Locked" & "Muted/Hidden" should be moved into the Grouped menu(which isn't coded in python? And which doesn't need shortcut for each entry).
  • Select > Mouse Cursor "Handles" & "Time Linked" & "Extended" - info about these functions should be moved out of the menus and into the Staturbar.
  • Strip > Mode... doesn't work when called from from Alt + B (popup) menu
  • Transform > Move (G) doesn't work if called from Alt + T (popup) menu.
  • Transform > Move/Extent from frame doesn't work if called from Alt + T (popup) menu.

In "sequencer.py":

  • Line 665 the duration of a transition can't be ripple deleted, because the transition duration can't be found.

@Brecht Van Lommel (brecht) I will review and modify this. It was submitted as-is, because I wanted to comunicate stated questions with authors.

Having said that, keymap question is quite invalid, as I can overwrite original key mapping as a whole, not just append/modify it. However I should probably do a diff to be clear what was changed. Also should changes in key mapping be listed in release notes or is just a mention about change enough?

Also should changes in key mapping be listed in release notes or is just a mention about change enough?

For 2.80 the release notes link to T55194: Shortcut Keys Changed in 2.8x, where you can add changes.
https://wiki.blender.org/wiki/Reference/Release_Notes/2.80/UI

@Brecht Van Lommel (brecht) & @Richard Antalik (ISS)
Thank you for spending your time on this.
As I have already stated, I'm not a coder.
Most of the operators are reworked code from ex. KinoRaw. That's why the code-style differs.
I have made the first round of changes from your(Brecht's) remarks(mostly the cosmetic ones), and uploaded the files here:
https://github.com/tin2tin/blender_vse_reworked
(I don't know how to submit patches, sorry)

Some of the remarks probably need a "grand-design" discussion, and some of the more serious issues I do not know how to solve myself, but when I get a bit more time I'll try to add more specific comments)

@Brecht Van Lommel (brecht) @Richard Antalik (ISS)
Another round of fixes uploaded here: https://github.com/tin2tin/blender_vse_reworked
And now also added some comments below.

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

The first part has been fixed.

These functions are partly to compensate and partly because of the concept of an active strip is not very well implemented in the VSE. Ex. it is very hard to see what strip is actually the active one. Where is it here?


And on top of this, working with an active strip in a NLE is something which only exists in Blender, as far as I know, so people will be puzzeled why strip changes aren't added to the entire selection as default behaviour.

Anyway, feel free to remove these functions or any other functions you do not feel fits into the consistency of Blender in general.

250

Not sure how to do that?

277

How would I make it more clear?

The function selects the channel. Should I add "all" or "entire"? There are functions too to select channel left or right of the current frame.

By default, it extends the selection, maybe the extending function should be added. But this would mean changing the hotkeys too. Currently they are "C" for "all", shift + C for "left" and alt + C for "right". Shift + C would then be for select extended and Ctrl + C is "copy". So the "C" shouldn't be used for the channel operations anymore.

289
  1. part: I removed the sorting.
  1. part: I do not understand. Feel free to do as you like.
304

The word "all" is added to stress that it is not selection dependent.

370–373

The purpose of this function is like a "main" switch to turn on/off all modifiers for improved playback speed. Like the "all audio on/off" toggle icon button on the right-hand side of the VSE Sequencer header. Maybe this function should be placed there too. This function and also the View Solo Channel would need a global array to store the individual settings before doing the switch so all strips can be switched back to their original settings.

The "selection-only" bool is not used currently, so maybe should be removed?

376–378

Are you sure you want to use the word "mute" about visuals?

431–432

Because "S" and "E" hotkeys are already used in various combinations. "I" and "O" aren't. But can be changed if you like.

1026–1030

Not sure how to solve this?

It doesn't work either if called from a popup menu(Alt+B).

1042

This is how the operator is working:


The playhead is auto-following the mouse cursor for instant preview of the frame to make a split at(left mouse).
I can't claim I entirely understand this code, so it is very likely it can be improved.

1064

Unfortunately changing the channel # in the header doesn't solo the audio of the current strip in the selected channel.

Example of usage:

This function needs to store the current locked/muted strips in a global array before doing the work, but I do not know how to do this.

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

Most VSE users don't know about this function, but yes it is a duplicate.

328

This was the only way I could get it to work. I don't know how to grey out the SMPTE menu?

503

I've changed the order, but using the word "select" inside the "Select" menu, seems redundant: https://devtalk.blender.org/uploads/default/original/2X/6/6711c9e12d0210b4e3702696db8f6bf65658c801.png

518

Good point, but I can't come up with anything better?

Maybe it is like box select - you're not selecting the box, but you're selecting inside the box. Eg. not selecting the playhead, but under, left or right of the playhead?

522

You're right this shouldn't be in a menu, but in the status bar, however, I do not know how to do this(with python in this file).

The main thing is that these very powerful functions shouldn't be hidden for the users.

Alt:

Ctrl:

Ctrl:

550

VSE users spend a lot of time trying to figure out the navigation controls out. Why hide it?

Eventually, I hope more functions can be added to this menu, like speed control etc.

708

I do not understand?

791

Maybe I'm a traditionalist, but I like cut, copy and paste next to each other and this way you'll understand that "lift" and "extract" both are cutting eg. removing strips.

This is another menu("Edit") I hoped to add more functions to, but I am limited by time and skills... Nathan who has the Power Sequencer project doesn't have the time currently to add functions from that project to this one.

1313–1314

Ok, I did, but the icon shows up left in the button and that fees inconsistent.

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

This is indeed a problem, but I don't think it makes sense to work around it for a few specific properties. It affects all properties really, so I suggest to wait for or work on a more general solution rather than adding this work around.

250

I thought it was maybe because some properties only exist on some kinds of strips, in which case hasattr would help.

But reading the code it's not clear to me what the try / except is even for at all.

277

It's not selecting the channel, but rather all strips in the channels. I think that should be clarified.

304

It's an operator for making a selection, no need to specify that is not selection dependent.

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.