Page MenuHome

Sequencer: tool system integration
AbandonedPublic

Authored by Richard Antalik (ISS) on Aug 11 2019, 10:35 PM.

Details

Summary

I have changed cut operator, so it works as a tool - that could be
reviewed.

Sseq->view is used as a mode for tools, nut sure if this is abuse.

Custom select operator is used here as default, but I guess I was tired when I decided that, it's unnecessary - will update that

Diff Detail

Repository
rB Blender
Branch
T (branched from master)
Build Status
Buildable 5952
Build 5952: arc lint + arc unit

Event Timeline

Update to defunct version.
Still no idea why this doesn't work.

  • fix mode enum for sequencer
  • fix keymap inconsistency
  • actually select should be default
  • custom OP has to be used for select

When opening a new sequencer workspace this error shows:

And a minor note:

release/scripts/startup/bl_ui/space_toolsystem_toolbar.py
2217

*_tools_annotate, could also be in SEQUENCER_PREVIEW
All other functions seems to be there to support it.

release/scripts/startup/bl_ui/space_toolsystem_toolbar.py
2217

In my tests it didn't work, but I can try again

release/scripts/startup/bl_ui/space_toolsystem_toolbar.py
2217

If you use ex. the layout workspace and change the 3d view to Sequencer-preview then you can draw when pressing "d", however if you do the same in the Video Editing workspace it doesn't seem to work.

Richard Antalik (ISS) retitled this revision from VSE: tool system integration [WIP] to VSE: tool system integration.Sep 24 2019, 1:37 PM
Dalai Felinto (dfelinto) requested changes to this revision.Sep 24 2019, 1:42 PM

Please pay attention to your compiler warnings. They help spot bugs like:

/source/blender/editors/space_sequencer/sequencer_edit.c: In function ‘sequencer_cut_exec’:
/source/blender/editors/space_sequencer/sequencer_edit.c:2206:18: warning: statement with no effect [-Wunused-value]
         cut_side == SEQ_SIDE_BOTH;
         ~~~~~~~~~^~~~~~~~~~~~~~~~
source/blender/editors/space_sequencer/sequencer_edit.c
2211
- cut_side == SEQ_SIDE_BOTH;
+ cut_side = SEQ_SIDE_BOTH;
source/blender/editors/space_sequencer/space_sequencer.c
889

Why the XXX here?

This revision now requires changes to proceed.Sep 24 2019, 1:42 PM
Richard Antalik (ISS) marked 4 inline comments as done.
Richard Antalik (ISS) edited the summary of this revision. (Show Details)
  • fix typos
source/blender/editors/space_sequencer/space_sequencer.c
889

It's in all editors. Campbell will probably know more about plans for this.

Campbell Barton (campbellbarton) requested changes to this revision.Oct 30 2019, 8:14 AM
Campbell Barton (campbellbarton) added inline comments.
release/scripts/startup/bl_ui/space_toolsystem_toolbar.py
1686–1687

This should show soft/hard cut option.

1700–1701

No need for dummy functions, it looks like this wont have any draw logic (the 3D view tool doesn't for eg).

1713

This gives an error, it need to use the real operator name.

RuntimeError: Error: Operator 'builtin.select_box' not found!
source/blender/blenkernel/BKE_sequencer.h
506

The enum is for left/right sides, where this only has meaning in regards to selection.

Why not use SEQ_SIDE_NONE ?, otherwise, this could be a separate variable internally, to avoid extending a generic enum.

source/blender/blenloader/intern/versioning_280.c
4174–4191

This should get it's own versioned section or recent files won't get the toolbar added.

Suggest version bumping & using this kind of check.

if (!MAIN_VERSION_ATLEAST(bmain, 281, 15)) { ... }

source/blender/editors/space_sequencer/sequencer_edit.c
2245–2246

Comment why pass through is needed.

2322

Currently we avoid "as tool" for operators since it's not descriptive for users developers.

This could be exposed with an "only_channel", "use_cursor_init" properties (to use the mouse cursor instead of the current frame).

source/blender/editors/space_sequencer/space_sequencer.c
889

This should be a define, fine to leave in for this commit and change everywhere later.

This revision now requires changes to proceed.Oct 30 2019, 8:14 AM
Richard Antalik (ISS) marked 8 inline comments as done.
  • Fix tool and operator names
  • remove dummy fn
  • Add cut type selection to tool panel (no icons)
  • Rename operator run_as_tool property
  • add comment
  • Reimplement operator enum item as separate bool
release/scripts/startup/bl_ui/space_toolsystem_toolbar.py
1713

I think I fixed it.
What I don't understand fully is what does 'builtin' actually mean and how is it supposed to be used. Now I treat it as just a convention.

source/blender/blenkernel/BKE_sequencer.h
506

Description of the enum is The side that remains selected after cutting
SEQ_SIDE_NONE will deselect everything.
SEQ_SIDE_NO_CHANGE may have some other niche use (using selection 'mask' to cut strips without need to reselect), thats why I opted to add this.

I am not quite sure what you mean by "separate variable internally". I added hidden property with more descriptive name.

source/blender/blenloader/intern/versioning_280.c
4174–4191

I did not understand why to bump version in diff. Is it because it is easy to forget before commiting?

Also this should be included in 2.82, and I see only 281 in versioning.
Since version bumps are merged from stable, I guess it should be included in last available versioning condition and bumped by some (random) merge?

Haven't seen this discussed / clarified.

  • rebase, bump subversion

It would be great if we could get this included in 2.82. Let’s try and get it done. More tools could always be added here, but getting the basics to work would be a nice benefit.

  • Fix active tool panel not updating.
  • Use W-key to cycle selection tools.
  • Added common keymap to toolbar (done in other toolbars).
  • Remove version bump (turns out this isn't needed), moved into utility function.
Campbell Barton (campbellbarton) requested changes to this revision.EditedDec 2 2019, 6:29 AM

Looked into applying this and ran into some more general design issues.

  • The toolbar is hidden by default, yet the active tools tab is visible.

    Shouldn't we either show both by default, or neither?
  • There is no top-bar for tool settings.

    Do we intend to follow the other space types design and add a top-bar to the sequencer?
This revision now requires changes to proceed.Dec 2 2019, 6:29 AM

@Campbell Barton (campbellbarton)

Shouldn't we either show both by default, or neither?

I think the toolbar should be visible by default. And the Sidebar should just show controls related to the selected strip, since the tool properties are in the top bar.

There is no top-bar for tool settings.
Do we intend to follow the other space types design and add a top-bar to the sequencer?

Yes - should add top bar here too.

Few notes:

  • Also - I noticed that the Select and Box Select tools do the same thing - both of them allow dragging clips and box selecting when dragging from outside a clip. Having both seems redundant then.
  • Of course we'll need an icon for Cut/Blade. I can take care of that separately.

Great, it's nicer to handle all versioning at once.

I'd guess we want to automatically add the top-bar to video editor space types unless they are "Preview" type.

These will look redundant to begin with, we could eventually move "Options" into a popover there too.

@Campbell Barton (campbellbarton) we could also hide the top bar by default as we do in some other editors. For now there aren’t really any tool settings yet anyway so it will seem rather unnecessary to keep that visible by default. The toolbar could be visible though.

Other tools we could add to toolbar could perhaps be the offset/roll tool.

I should also add some special/different cursors for these tools I think.

Maybe the feedback on the actual tools should be saved for later to get this committed asap, but I noticed a few things:

  • In Sequencer/Preview mode the Annotation tool isn't working.
  • Shouldn't the Annotations panel in the View panel be moved into the the Tool panel in the Sidebar, instead of the current pop-up?
  • The cut types are called 'Cut' and 'Hold Cut' in the UI - not hard and soft.
  • If the topbar isn't used, maybe selecting a new tool should auto-change tab to Tool, so the Tool options will become visible, even though another tab has been selected?
  • The background color of the VSE toolbars aren't in consistency with the toolbar in the 3D View(needs to be darker).
  • In the current state of the Cut tool, it is only usable for cutting audio waveforms. In order to cut video, the users will need a live video preview of the cut/mouse cursor position - which is how the razor/blade tool is working in most NLEs. The mouse cursor could also change to a razorblade in this mode.
  • Slide could be a quick-to-implement-tool.
  • Add top-bar
  • Update for recent fallback tool addition
Campbell Barton (campbellbarton) retitled this revision from VSE: tool system integration to Sequencer: tool system integration.Dec 16 2019, 3:29 AM

Closed by 6a49161c8c60cb63d9e3ed8dbe700163ff858436