Adds the option to use a custom color for strips in the VSE. This looks like this:
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
This looks very useful.I will review the patch and see if the code needs to be updated.
Comment Actions
I tried it today and it definitely needs an update. I'm trying to rewrite it.
My plan is to also add a preset system like for the node color in the node editor
Comment Actions
Works as advertised. Very nice with the presets, too.
I don't know if the color presets should have a commit of it's own, and then add it to all the color elements in the VSE, like the text color, the color strips, and several of the modifiers too. But maybe @Richard Antalik (ISS) can let you know of it is okay to include it in this patch?
On the UI, maybe the panel could be called "Strip Color" to avoid confusion with other color elements in the Sidebar? Or maybe the color and preset button could be added next to the strip name/string entry box on top, so all the visual strip elements are collected in the same line? What do @William Reynish (billreynish) say?
If you, at some point, want to include some preset colors, maybe it would be best to avoid the colors already used for the various strip types to avoid confusion?
If the argument comes up, that changing the strip color will make it difficult to know what strip type it is, then an option could be to add a checkbox for limiting the vertical drawing to a line in the top or bottom of the strip, so both the original strip color and the new color is visible?
For reference, this is the current UI:
Comment Actions
Works as advertised. Very nice with the presets, too.
I don't know if the color presets should have a commit of it's own, and then add it to all the color elements in the VSE, like the text color, the color strips, and several of the modifiers too. But maybe @Richard Antalik (ISS) can let you know of it is okay to include it in this patch?
On the UI, maybe the panel could be called "Strip Color" to avoid confusion with other color elements in the Sidebar? Or maybe the color and preset button could be added next to the strip name/string entry box on top, so all the visual strip elements are collected in the same line? What do @William Reynish (billreynish) say?
If you, at some point, want to include some preset colors, maybe it would be best to avoid the colors already used for the various strip types to avoid confusion?
If the argument comes up, that changing the strip color will make it difficult to know what strip type it is, then an option could be to add a checkbox for limiting the vertical drawing to a line in the top or bottom of the strip, so both the original strip color and the new color is visible?
For reference, this is the current UI:
Thanks for your feedback.
I forgot to say, that currently, this is a per strip setting (color strips excluded) and does not depend on modifiers etc.
To change the name is only cosmetics and is easily done and I think it is a good idea. In my old path for version 2.79 I already had it besides all the other strip options, but in case of 2.8 I didn't know what the general UI rules are.
At this point in time I did not add any predefined presets (only default one)..
This sounds great to me. I have to dig deaper into the code but I think it is no problem, but I think this belongs to the project owners to decide?
Comment Actions
Most functions in the Strip tab in the Sidebar will change values in the full selection if +Alt is pressed or by using 'Copy to Selected' in the right click menu:
Maybe this would be a useful way to batch change colors on multiple strips?
Comment Actions
Most functions in the Strip tab in the Sidebar will change values in the full selection if +Alt is pressed or by using 'Copy to Selected' in the right click menu:
Maybe this would be a useful way to batch change colors on multiple strips?
Using "Copy to Selected" indeed works also in the VSE.
| release/scripts/presets/strip_color/default.py | ||
|---|---|---|
| 5 ↗ | (On Diff #20939) | Defaults should not be set in python. I believe they should be set here @Richard Antalik (ISS) is that correct |
Comment Actions
I don't want to seem mad.. but I think that there are a lot of discrepancies between what developers say and what is actually in the Blender code... (makes it very hard for new developers to do the smallest things..)
To be correct, yes, the default values when the Strip is created should be set somewhere else, i did not knew exactly where...
But in the presets of tracking marker colors, there is also a default preset and that is why I added it here too...
But okay, without understanding this, I will add the default value of creation and remove the preset (maybe I create some other)
Comment Actions
Is this common functionality in video editors? We can of course add custom colors all over (many other places too), but would avoid unless it's really needed.
If video previews are added this may be less useful too.
| release/scripts/startup/bl_operators/presets.py | ||
|---|---|---|
| 530–545 ↗ | (On Diff #20939) | Would not use preset systems for colors (we don't do this anywhere else) - object / node colors for example. |
| source/blender/makesdna/DNA_sequence_types.h | ||
| 142 | For display viewport display, byte precision colors are enough. | |
Comment Actions
I don't c
I don't care anymore... It was on right-click-select and I thought it would be a good start to get into programming for Blender...
But simply doing it so as it is somewhere else in Blender also does not fullwill the requirements.. (node color also is float)
Comment Actions
It still is!
But simply doing it so as it is somewhere else in Blender also does not fullwill the requirements.. (node color also is float)
Every developer will get feedback on their patches. There are always things you don't think about that others do see, that's what the review process is all about. The original developer of the patch and the reviewers work as a team, and keep going back & forth with improvements and tweaks until the patch is of such quality that it can go into Blender. Nobody is scolding you for doing the wrong thing; nobody is expecting you to do the right thing from the get-go. Any feedback remarks you get are about that particular bit of code, and do not reflect on you personally.
I think what the most important question here is: what is the reason VSE strips need a custom colour? What is the intended workflow? You said you were on RightClickSelect before; I can imagine that this feature was discussed there. It would be useful to have a link to that discussion.
Your patch has been sitting still for quite a while, sorry for that. Recently I made a list of Ingredients of a Patch that might be useful for you to read through. It'll help make future patch review sessions a bit smoother/faster/nicer.
Comment Actions
@Rudolf Ortner (rudolf.ortner) when working on new tasks it's best to work on approved task - see T64682: Video Sequence Editor (Sequencer) Module, if you can't find any that are a starting point you could ask developers in blender.coders chat.
Comment Actions
Color coding of clips is a fairly normal feature in most Video Editors out there.
Avid:
Premiere(gif):
Final Cut Pro:
The right-click-select thread:
https://blender.community/c/rightclickselect/y1bbbc/
Imo, there is a point in the comment by David McSween on RCS: "Hmm after editing video in many NLEs with many other editors, I find that User color coding is preferred by everyone over the standard or default palette. The issue here is mostly to do with the VSE having media agnostic tracks (so a strip could be anything) making it hard to group strip functions by their geography on the screen."
Which could be an argument for using a colored line instead of changing the entire color of the strip?
Comment Actions
I am sorry for leaving a bad impression about myself..
I have no problem with you saying that this patch might not be useful..
But to criticize small code peaces that are everywhere else in Blender is confusing and not very motivating...
Comment Actions
I guess I will finish this patch if you don’t want. Sorry if I was off putting with my comments I should have worded them better.
I thought I submitted those comments ⬇️
| release/scripts/presets/strip_color/default.py | ||
|---|---|---|
| 5 ↗ | (On Diff #20939) | This is unnecessary no defaults need to be set |
| release/scripts/startup/bl_operators/presets.py | ||
| 736 ↗ | (On Diff #20939) | Remove AddPresetStripColor |
| release/scripts/startup/bl_ui/space_sequencer.py | ||
| 1408 | I don’t think this is necessary | |
| 1415 | Rename
| |
| 1435 | Remove def draw_header_preset | |
| 2217 | Remove SEQUENCER_PT_strip_color_presets | |
| source/blender/makesdna/DNA_sequence_types.h | ||
| 142 | Move to the bottom of declaration of Sequencer. | |
Comment Actions
Almost done only a few changes left
| release/scripts/startup/bl_operators/presets.py | ||
|---|---|---|
| 510 ↗ | (On Diff #21032) | Can you fix the white space |
| 528 ↗ | (On Diff #21032) | Can you fix the white space |
| source/blender/makesrna/intern/rna_sequencer.c | ||
| 1738 | I’m not familiar with RNA_def_property_float_array_default so I’m not sure if this is done correctly. | |
| release/scripts/startup/bl_operators/presets.py | ||
|---|---|---|
| 528 ↗ | (On Diff #21032) | I'm very sorry for that... |
| source/blender/makesrna/intern/rna_sequencer.c | ||
| 1738 | I found it here in rna_gpencil.c prop = RNA_def_property(srna, "annotation_onion_before_color", PROP_FLOAT, PROP_COLOR_GAMMA); RNA_def_property_float_sdna(prop, NULL, "gcolor_prev"); RNA_def_property_array(prop, 3); RNA_def_property_range(prop, 0.0f, 1.0f); RNA_def_property_float_array_default(prop, default_onion_color_b); RNA_def_property_clear_flag(prop, PROP_ANIMATABLE); RNA_def_property_ui_text(prop, "Before Color", "Base color for ghosts before the active frame"); RNA_def_property_update(prop, NC_GPENCIL | ND_DATA, "rna_GPencil_update"); | |
Comment Actions
I need to discuss with @Richard Antalik (ISS) variable names but everything else looks done.
| source/blender/blenkernel/intern/sequencer.c | ||
|---|---|---|
| 5456 | I checked other places in blender code and there is no preset for color picker so this is unnecessary | |
| source/blender/blenkernel/intern/sequencer.c | ||
|---|---|---|
| 5456 | The Custom Node color is also done this way: Somebody of you told me this morning to set the start value in BKE_sequence_alloc | |
Comment Actions
Regarding use case of setting colors, it seems other applications use a palette of categories, which seems more useful then manually setting RGB for for every strip
Like what we have for bones.
Linking from UI task.
| source/blender/makesdna/DNA_sequence_types.h | ||
|---|---|---|
| 243–244 | Can you be a little more descriptive of what you want changed I don’t understand exactly what you mean here so I don’t think @Rudolf Ortner (rudolf.ortner) would understand. | |
| source/blender/makesrna/intern/rna_sequencer.c | ||
| 1727 | I would name it ui_custom_strip_color | |
Comment Actions
That's why I wanted to add presets in the first place. So that the user can quickly assign predefined colors..
I am currently not familiar with Bones and what you actually mean?
| source/blender/makesdna/DNA_sequence_types.h | ||
|---|---|---|
| 243–244 | I think he means that I should use char instead of float for the color. | |
Comment Actions
See the preferences -> Theme -> Bone Color Sets,
This wouldn't have to have colors for each state (selected, active), it's just as example of a predefined color palette.
Comment Actions
Just chiming in here after seeing this task mentioned in the UI channel on blender chat:
Premiere Pro uses a fairly flexible system for strip colors. It gives you a set 16 labels with configurable names and colors, and seven strip types that can have default labels assigned to them. From there, you can (re)assign labels as you please, with a global toggle for setting the labels on a per-instance or a per-source-file basis. This lets you setup your labels as:
- Role labels (e.g. assigning a 'B Cam' label that happens to be green)
- Group labels (e.g. assigning a 'Group 3' label that happens to be blue)
- Color labels (e.g. assigning a 'Red' label that happens to be red)
- Some hybridization of the above
We don't need to copy Premiere Pro, but I do think that the general notion of offering visually distinct sets/groups/labels is worth exploring. The selfish part of me would love it if we used a combination of colors and patterns (Trello handles this wonderfully), but I wouldn't delay the core feature for the sake of a niche option.
Comment Actions
I would tend to agree with @Campbell Barton (campbellbarton) that it's probably more useful to have a set of presets. Some apps use the concept of 'roles'. This way, you can affect many strips at a time in a more organized fashion.
Another issue is simply more UI layout. I feel like this feature becomes way too prominent in the Sequencer sidebar, since it's a less important small strip option, but is presented as the main control here:
Ideally we would try and find a less prominent place for it. Possibly it could go in a subpanel to the Time panel, which has other clip-related items, or we'd need to re-think the parent panels a bit to find a sensible home for this.
Alternatively, if we re-think this feature as 'Roles', then it seems more reasonable to have a top-level Roles panel with a UI list of roles here.
@Peter Fog (tintwotin) posted an alternative viable solution in Blender.chat's ui section, which could be to simplify the strip colors feature, so that it just becomes a small color swatch next to the strip name:
I think this could also be ok, but the difficulty then is that the strip would need to be automatically assigned a color when created.
In conclusion, I think it's best if we go with one of the two extremes: Either an even simpler color dropdown next to the strip name, or a more elaborate system of roles/presets in a panel.
Comment Actions
An Attract python patch to color code strips according to their development status:
https://developer.blender.org/rBCA4c84fbf33982e1dcf7db7de92c4649929ccca965
Comment Actions
Just as a meta-question about the patch review process: Why is the code even being looked at, when there is still a discussion about the desired functionality? I don't see the use to ask someone to change strip_color into ui_custom_strip_color when we might even end up not storing the color at all, but rather an index into a list of roles (which could be stored in the blend file as well).
Comment Actions
At the time I wasn't sure if @Richard Antalik (ISS) was looking to commit this, so I commented on the code and raised some concerns.
In retrospect it would have been better to postpone the code review and handle this as a design task.
I'd normally leave this up to the module owner, however I suppose someone in the UI team could do this too.
Comment Actions
Agree here.
I don't think intention here is to implement any kind of sophisticated role system. I don't think we need one right now.
@Peter Fog (tintwotin) posted an alternative viable solution in Blender.chat's ui section, which could be to simplify the strip colors feature, so that it just becomes a small color swatch next to the strip name:
I think this could also be ok, but the difficulty then is that the strip would need to be automatically assigned a color when created.
If there could be checkbox to enable this feature in color picker popup, that would be IMO best solution.
Sorry I am getting to this so late btw.