Details

Reviewers

Campbell Barton (campbellbarton)
Richard Antalik (ISS)

Group Reviewers

Video Sequencer

Commits

rB5cebcb415e76: VSE: Add color tags to strips

Summary

This patch adds color tags to VSE strips, an overlay option to toggle the colors on and off, a section in the theme settings to define the 8 possible colors and two ways of changing the color tag through the UI.

The current colors still need to be changed.

You can change the color through the right-click context menu:

Or in the strip side panel next to the strip name:

Overlay setting:

Theme settings:

It also covers versioning, so strips in older files should get the default color tag assigned.

Diff Detail

Repository

rB Blender

Branch

arcpatch-D12405_1 (branched from master)

Build Status

Buildable 17417
Build 17417: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@Falk David (filedescriptor) Yes that's what I was thinking, then I didn't understand the reply from Francesco at all :)

Remove default value
Merge branch 'master' into temp-vse-colored-strips
Add strip_color userpref, property and icons
Versioning and default values
Add operator to change strip color
Add operator to context menu
Merge branch 'master' into temp-vse-colored-strips

Harbormaster completed remote builds in B16950: Diff 41726.Sep 12 2021, 10:53 PM

Updated the diff to use a palette system instead of custom colors. The code is based on how this is handled with collections.

I am using the default colors from the collections, but this would have to be revisited by the UI team or by through a community feedback.
Also, I tried to add the color picker to the Strip side panel, but it doesn't work really well. You end up with buttons and icons that don't scale with the UI... It's a big mess.

Now I am not sure if the overlay option "Display Color Mode" makes sense any more. It might be better to remove that with this system.

Falk David (filedescriptor) edited the summary of this revision. (Show Details)Sep 12 2021, 11:03 PM

From the images, it looks great, however I think you should give exposing the palette options in the sidebar one more thought. The strip color is a strip property and strip properties belongs in the strip sidebar. Have you tried to add a color coding button at the end of then name before the mute button, and clicking it should open a drop down containing the palette? Something like this?

WIP: Add color tag picker in strip panel

Harbormaster completed remote builds in B16951: Diff 41728.Sep 13 2021, 8:24 AM

@Peter Fog (tintwotin) Ok I liked the idea with the subpanel and tried it out. It works better than expected, but there are still a few issues like the space at the left. I'm not an expert when it comes to python UI related stuff so I'm guessing that you can do better than what I have right now.

Falk David (filedescriptor) edited the summary of this revision. (Show Details)Sep 13 2021, 8:28 AM

Icon when no color is selected + small UI tweaks

Harbormaster completed remote builds in B16956: Diff 41733.Sep 13 2021, 11:54 AM

The theme panel with the colors should be moved into the "Video Sequencer" as a subsection.

Merge branch 'master' into temp-vse-colored-strips
Fix merge issue
Replace color mode with color tag boolean toggle
Make the color picker panel prettier

Harbormaster completed remote builds in B17025: Diff 41844.Sep 15 2021, 10:37 AM

Falk David (filedescriptor) retitled this revision from VSE: Add option to color vse strips individually to VSE: Add color tags to strips.Sep 15 2021, 10:42 AM

Falk David (filedescriptor) edited the summary of this revision. (Show Details)

Falk David (filedescriptor) edited the summary of this revision. (Show Details)Sep 15 2021, 10:53 AM

Falk David (filedescriptor) edited the summary of this revision. (Show Details)Sep 15 2021, 10:56 AM

I am pretty happy with what I got now. Feedback is very welcome!
Note that the color palette is still the same as the "collection color tag" one. I am sure that we will need better colors down the line. (Maybe @Pablo Vazquez (pablovazquez) can help out?)

@Richard Antalik (ISS) If there is still a possibility to get this into 3.0 I will try my best to work on this as much as you need me to.

Falk David (filedescriptor) edited the summary of this revision. (Show Details)Sep 15 2021, 11:05 AM

Falk David (filedescriptor) added a reviewer: Video Sequencer.

Campbell Barton (campbellbarton) requested changes to this revision.Sep 15 2021, 11:17 AM

Campbell Barton (campbellbarton) added a subscriber: Campbell Barton (campbellbarton).

Campbell Barton (campbellbarton) added inline comments.

release/scripts/startup/bl_ui/space_sequencer.py
1022–1023	Prefer percentage formatting over f-strings.
1095	single quotes for enum values.
1102	trailing space.
source/blender/sequencer/intern/utils.c
611–612	scene and seq arguments can be const.
616	Color tag should be bounds checked (files from the future may have more colors). `(uint)seq->color_tag < SEQUENCE_COLOR_TOT` is sufficient and accounts for -1 too.

This revision now requires changes to proceed.Sep 15 2021, 11:17 AM

Merge branch 'master' into temp-vse-colored-strips
Code review comments

Harbormaster completed remote builds in B17028: Diff 41852.Sep 15 2021, 3:19 PM

Fix spaces

Harbormaster completed remote builds in B17029: Diff 41853.Sep 15 2021, 3:22 PM

Falk David (filedescriptor) marked 5 inline comments as done.Sep 15 2021, 3:24 PM

Campbell Barton (campbellbarton) accepted this revision.Sep 15 2021, 3:31 PM

This revision is now accepted and ready to land.Sep 15 2021, 3:31 PM

This is a good improvement!
In the context menu I suggest to try nesting the color swatch in a submenu, to prevent it from being visible all the time.
The full color spectrum is eye-catching and creates visual noise in the navigation. I understand there is a tradeoff between speed of access and visual noise.

Add submenu for color picker
Merge branch 'master' into temp-vse-colored-strips

Harbormaster completed remote builds in B17047: Diff 41892.Sep 16 2021, 11:28 AM

Enable "Show Color Tags" by default

Harbormaster completed remote builds in B17048: Diff 41893.Sep 16 2021, 11:43 AM

Campbell Barton (campbellbarton) added inline comments.Sep 16 2021, 1:31 PM

source/blender/blenloader/intern/versioning_300.c
1359–1361	This isn't setting values for nested strips. Use `SEQ_for_each_callback`

Campbell Barton (campbellbarton) added inline comments.Sep 16 2021, 1:35 PM

source/blender/blenloader/intern/versioning_300.c
1359–1361	NOTE: A recursive function would also be fine (used often in sequence versioning).

Merge branch 'master' into temp-vse-colored-strips
Do version recursively so we also handle meta strips

Harbormaster completed remote builds in B17053: Diff 41899.Sep 16 2021, 3:26 PM

Thanks @Campbell Barton (campbellbarton) !

@Francesco Siddi (fsiddi) I changed it so the picker is now in the "Set Color Tag" submenu. This is also exposed in the "Strip" menu for consistency.

Campbell Barton (campbellbarton) accepted this revision.Sep 16 2021, 5:02 PM

Thank you for taking the suggestions on board @Falk David (filedescriptor)!
I have one final suggestion, which is to add a shade of grey to the palette.

A note for future iterations: in order to use the palette to convey multiple (orthogonal) meanings, such as shot grouping, production stages, duration thresholds, proxy vs online, it will be useful to implement a rule-based color system. This concept has been brought up several time by Ton in other contexts, and it would apply here too. This is a good topic to look into during the Blender 3 series.

@Francesco Siddi (fsiddi) No problem :) Would the shade of grey replace one of the existing colors or would it be a new 9th color?

I suggest to make it a 9th color, after brown.

I can't seem to understand why, but the patch consistently crashes for me, when I choose the "Video Editing" app template. I think it has to do with the fact that there are no strips in the default file. When testing, I loaded a file that had some strips already in the timeline.

@Campbell Barton (campbellbarton) any idea what could be happening?

Merge branch 'master' into temp-vse-colored-strips
Add new color: grey

Harbormaster completed remote builds in B17107: Diff 41997.Sep 17 2021, 4:52 PM

I noticed an issue:

Load Blender with factory settings
New File > Video Editing
Crash

Can you reproduce?

In D12405#327069, @Falk David (filedescriptor) wrote:

I can't seem to understand why, but the patch consistently crashes for me, when I choose the "Video Editing" app template. I think it has to do with the fact that there are no strips in the default file. When testing, I loaded a file that had some strips already in the timeline.

@Campbell Barton (campbellbarton) any idea what could be happening?

@Francesco Siddi (fsiddi) yes, can reproduce, but can't figure out what's wrong. The crash happens whithin the initialization of the editor UI.

In D12405#327385, @Francesco Siddi (fsiddi) wrote:

I noticed an issue:

Load Blender with factory settings

New File > Video Editing

Crash

Can you reproduce?

It's not crashing for me, whats the stack-trace?

Sorry for getting late to this, I have only few requests for change here. Other than that I think this patch works well.

As for crash mentioned earlier, I can't reproduce either, this is after resolving conflicts and rebasing on latest master, because I was getting some build errors at current revision.

source/blender/editors/space_sequencer/sequencer_draw.c
110–233	What is reason for moving this to `SEQ_utils.h`? these functions usually handle sequencer internal workings while this function is only called from `sequencer_draw.c` and could remain static.
source/blender/editors/space_sequencer/sequencer_edit.c
2003	I think, that this is best to set in `SEQ_sequence_alloc()` for all strip types so you can remove this from `strip_add.c` too.

This revision now requires changes to proceed.Sep 21 2021, 3:10 PM

Seems like the overlays in the sequencer have been split into preview overlays and timeline overlays. I will move the overlay to show the color tags to the timeline overlays.

In D12405#328336, @Richard Antalik (ISS) wrote:

Sorry for getting late to this, I have only few requests for change here. Other than that I think this patch works well.

As for crash mentioned earlier, I can't reproduce either, this is after resolving conflicts and rebasing on latest master, because I was getting some build errors at current revision.

Yea I also had to fix some merge issues. It seems like with latest master, the crash is gone.
As for the comments: I moved the function into SEQ_utils because initially I needed to set a custom color for each strip based on the theme. Since that is no longer the case, I can move the function back to the draw code. Just haven't done that yet.

Hi @Falk David (filedescriptor) - will you be able to implement the suggested changes soon?
It would be great to have this feature merged before bcon2!

@Francesco Siddi (fsiddi) I'm on holiday until Friday! I was also hoping to get this done before bcon2 :( There is a chance though that I can finish the changes tomorrow on my trip back.

Peter Fog (tintwotin) removed a subscriber: Peter Fog (tintwotin).Sep 29 2021, 9:45 AM

Rebase

Harbormaster completed remote builds in B17417: Diff 42633.Sep 29 2021, 12:05 PM

Set initial color_tag in SEQ_sequence_alloc
Move function color3ubv_from_seq back to original place.

Harbormaster completed remote builds in B17419: Diff 42635.Sep 29 2021, 12:18 PM

Cleanup unused include, whitespace

Harbormaster completed remote builds in B17421: Diff 42637.Sep 29 2021, 12:30 PM

Fixed my requests, went over patch again, and I don't see any other issues, so for me this is good to go.

This revision is now accepted and ready to land.Sep 29 2021, 12:31 PM

Closed by commit rB5cebcb415e76: VSE: Add color tags to strips (authored by Falk David (filedescriptor), committed by Richard Antalik (ISS)). · Explain WhySep 29 2021, 2:36 PM

This revision was automatically updated to reflect the committed changes.

Richard Antalik (ISS) added a commit: rB5cebcb415e76: VSE: Add color tags to strips.

VSE: Add color tags to strips
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 42633

release/datafiles/userdef/userdef_default_theme.c

release/scripts/startup/bl_ui/space_sequencer.py

release/scripts/startup/bl_ui/space_userpref.py

source/blender/blenkernel/BKE_blender_version.h

source/blender/blenloader/intern/versioning_300.c

source/blender/blenloader/intern/versioning_defaults.c

source/blender/blenloader/intern/versioning_userdef.c

source/blender/editors/include/UI_icons.h

source/blender/editors/interface/interface_icons.c

source/blender/editors/space_sequencer/sequencer_draw.c

source/blender/editors/space_sequencer/sequencer_edit.c

source/blender/editors/space_sequencer/sequencer_intern.h

source/blender/editors/space_sequencer/sequencer_ops.c

source/blender/editors/space_sequencer/space_sequencer.c

source/blender/makesdna/DNA_sequence_types.h

source/blender/makesdna/DNA_space_types.h

source/blender/makesdna/DNA_userdef_types.h

source/blender/makesrna/RNA_enum_items.h

source/blender/makesrna/intern/rna_sequencer.c

source/blender/makesrna/intern/rna_space.c

source/blender/makesrna/intern/rna_userdef.c

source/blender/sequencer/CMakeLists.txt

source/blender/sequencer/SEQ_utils.h

source/blender/sequencer/intern/strip_add.c

source/blender/sequencer/intern/utils.c

VSE: Add color tags to stripsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 42633

release/datafiles/userdef/userdef_default_theme.c

release/scripts/startup/bl_ui/space_sequencer.py

release/scripts/startup/bl_ui/space_userpref.py

source/blender/blenkernel/BKE_blender_version.h

source/blender/blenloader/intern/versioning_300.c

source/blender/blenloader/intern/versioning_defaults.c

source/blender/blenloader/intern/versioning_userdef.c

source/blender/editors/include/UI_icons.h

source/blender/editors/interface/interface_icons.c

source/blender/editors/space_sequencer/sequencer_draw.c

source/blender/editors/space_sequencer/sequencer_edit.c

source/blender/editors/space_sequencer/sequencer_intern.h

source/blender/editors/space_sequencer/sequencer_ops.c

source/blender/editors/space_sequencer/space_sequencer.c

source/blender/makesdna/DNA_sequence_types.h

source/blender/makesdna/DNA_space_types.h

source/blender/makesdna/DNA_userdef_types.h

source/blender/makesrna/RNA_enum_items.h

source/blender/makesrna/intern/rna_sequencer.c

source/blender/makesrna/intern/rna_space.c

source/blender/makesrna/intern/rna_userdef.c

source/blender/sequencer/CMakeLists.txt

source/blender/sequencer/SEQ_utils.h

source/blender/sequencer/intern/strip_add.c

source/blender/sequencer/intern/utils.c

VSE: Add color tags to strips
ClosedPublic
Actions

Revision Contents
Changeset List