Details

Reviewers

Campbell Barton (campbellbarton)
Dalai Felinto (dfelinto)

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

Richard Antalik (ISS) created this revision.Aug 11 2019, 10:35 PM

Harbormaster completed remote builds in B4375: Diff 17029.Aug 11 2019, 10:35 PM

Eitan Traurig (EitanSomething) awarded a token.Aug 12 2019, 12:53 AM

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

Harbormaster completed remote builds in B4394: Diff 17061.Aug 13 2019, 1:36 AM

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

Harbormaster completed remote builds in B5069: Diff 18442.Sep 24 2019, 3:06 AM

William Reynish (billreynish) awarded a token.Sep 24 2019, 3:19 AM

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.

Richard Antalik (ISS) added inline comments.Sep 24 2019, 12:25 PM

release/scripts/startup/bl_ui/space_toolsystem_toolbar.py
2217	In my tests it didn't work, but I can try again

Peter Fog (tintwotin) added inline comments.Sep 24 2019, 12:29 PM

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) edited the summary of this revision. (Show Details)Sep 24 2019, 12:42 PM

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

Richard Antalik (ISS) retitled this revision from VSE: tool system integration [WIP] to VSE: tool system integration.Sep 24 2019, 1:37 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

fix typos

Harbormaster completed remote builds in B5476: Diff 19236.Oct 29 2019, 12:56 AM

Richard Antalik (ISS) added inline comments.Oct 29 2019, 12:56 AM

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

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

Harbormaster completed remote builds in B5505: Diff 19291.Oct 31 2019, 1:04 AM

Richard Antalik (ISS) added inline comments.Oct 31 2019, 1:19 AM

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.

Peter Fog (tintwotin) removed a subscriber: Peter Fog (tintwotin).Oct 31 2019, 6:05 AM

rebase, bump subversion

Harbormaster completed remote builds in B5875: Diff 20006.Dec 2 2019, 2:22 AM

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.

Harbormaster completed remote builds in B5877: Diff 20008.Dec 2 2019, 6:24 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

Harbormaster completed remote builds in B5952: Diff 20179.Dec 10 2019, 6:38 AM

Campbell Barton (campbellbarton) retitled this revision from VSE: tool system integration to Sequencer: tool system integration.Dec 16 2019, 3:29 AM

David Friedli (hlorus) added a subscriber: David Friedli (hlorus).Jan 20 2020, 8:09 AM

Closed by 6a49161c8c60cb63d9e3ed8dbe700163ff858436

Sequencer: tool system integration
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 20179

release/scripts/presets/keyconfig/keymap_data/blender_default.py

release/scripts/startup/bl_ui/space_sequencer.py

release/scripts/startup/bl_ui/space_toolsystem_common.py

release/scripts/startup/bl_ui/space_toolsystem_toolbar.py

source/blender/blenkernel/BKE_sequencer.h

source/blender/blenloader/intern/versioning_280.c

source/blender/editors/space_sequencer/sequencer_edit.c

source/blender/editors/space_sequencer/space_sequencer.c

source/blender/makesrna/RNA_enum_types.h

source/blender/makesrna/intern/rna_space.c

source/blender/makesrna/intern/rna_workspace.c

source/blender/windowmanager/WM_toolsystem.h

source/blender/windowmanager/intern/wm_toolsystem.c

Sequencer: tool system integrationAbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 20179

release/scripts/presets/keyconfig/keymap_data/blender_default.py

release/scripts/startup/bl_ui/space_sequencer.py

release/scripts/startup/bl_ui/space_toolsystem_common.py

release/scripts/startup/bl_ui/space_toolsystem_toolbar.py

source/blender/blenkernel/BKE_sequencer.h

source/blender/blenloader/intern/versioning_280.c

source/blender/editors/space_sequencer/sequencer_edit.c

source/blender/editors/space_sequencer/space_sequencer.c

source/blender/makesrna/RNA_enum_types.h

source/blender/makesrna/intern/rna_space.c

source/blender/makesrna/intern/rna_workspace.c

source/blender/windowmanager/WM_toolsystem.h

source/blender/windowmanager/intern/wm_toolsystem.c

Sequencer: tool system integration
AbandonedPublic
Actions

Revision Contents
Changeset List