Details

Reviewers

Richard Antalik (ISS)
Hans Goudey (HooglyBoogly)
Pablo Vazquez (pablovazquez)

Commits

rBfad80a95fd08: VSE: Add Overlay popover panels

Summary

Many editors have already had overlay popover implemented, but not yet the Sequencer. This patch adds overlay popovers for the 3 sequencer view modes, in consistency with the other editors.

Changes:
Overlay popovers for Preview + Sequencer/Preview + Sequencer.

Entries from the View menus moved to the popovers, which will simplify the cluttered View menus.

Additional options have been added: Show Strip Name, Show Source(ex. path), Show Duration, so users can now select what info they need to see on the strips, and get rid of the clutter.

A main switch to turn all overlays on/off has been added in consistency with ex. overlays in the 3D View.

Use full height for drawing waveforms when no info texts are displayed.

Removal of identical entries will make the code much more complicated, so it has been removed, since entries are now selectable.

Set Source text to ON and Name and Duration as OFF, as default settings, to avoid clutter and redundant Info of strips.

In the View menus, move Zoom to Fit up together with the rest of the Preview Zoom functions, to make the menus more user friendly now a lot of entries in the menus have been moved.

Waveform Displaying renamed to Waveform Display.

Diff Detail

Event Timeline

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

This revision now requires changes to proceed.Dec 5 2020, 12:49 AM

The design aspect seems fine to me. +1

jorge (jorsh) awarded a token.Dec 5 2020, 2:40 AM

Daniel Salazar (zanqdo) awarded a token.Dec 5 2020, 2:51 AM

Daniel Salazar (zanqdo) rescinded a token.

Daniel Salazar (zanqdo) awarded a token.

@Hans Goudey (HooglyBoogly)

Updated the versioning code.

Fixed Effect Strip.

This looks very nice. I would maybe say that even showing waveforms could be on/off switch.

Versioning code belongs to versioning_290.c since we are at 2.9x. The draw function looked quite branched even before so I guess I can get over that. Will check code in detail tomorrow.

Antonio Vazquez (antoniov) added a subscriber: Antonio Vazquez (antoniov).Dec 5 2020, 10:55 AM

Antonio Vazquez (antoniov) added inline comments.

source/blender/blenloader/intern/versioning_280.c
5117 ↗	(On Diff #31630)	Better use the `LISTBASE_FOREACH`macro..for example: `LISTBASE_FOREACH(bScreen *, screen, &bmain->screens){`

Hugo Schouppe (HUSCH) added a subscriber: Hugo Schouppe (HUSCH).Dec 5 2020, 12:20 PM

Moved version to 2.90 and using listbase.

Get rid of the 280 version file.

Remove string trimming when identical entries, since they are selectable now.
Major code clean up.

Test file:

filename2.blend683 KBDownload

Updated to use full height for drawing waveforms when no info texts are displayed.

In D9751#242410, @Richard Antalik (ISS) wrote:

This looks very nice. I would maybe say that even showing waveforms could be on/off switch.

Yes, it's a bit odd with a checkbox for on and off, but it's because these two and the third the strip option are working as radio buttons, so either radio button icons should be used instead or some other solution should be found like a checkbox for on/off and then two settings in a dropdown: 'All' or 'Strip Setting' , but this should properly be left for another patch.

I have tested patch, when I chose combined view, overlay menu is in middle of header.
It may be worth implementing overlays also for preview - metadata, safe areas and annotations. Not sure if "Frame overlay" should be considered an overlay.

By default strip name for effect, scene, mask and movieclip strips will match "source". This will display name like Mask | Mask | 1234 which is quite redundant. Previously it seems that only name or source is displayed if these two match. Also I personally like previous format more Name: Source | Length Perhaps it would better hide redundancy?

The whole draw_seq_text() could be improved significantly I think

Have 2 functions that will set name and source for seq. There is no need to have separators in these strings.
Have function that defines format
Construct final string in one line

It would look like this

static void draw_seq_text_get_name(Sequence *seq, char *r_name)
{
  /* conditions, switches and whatnot... */
  BLI_strncpy(r_name, BKE_sequence_give_name(seq), sizeof(r_name));
}

static void draw_seq_text_get_source(Sequence *seq, char *r_source)
{
  /* conditions, switches and whatnot... */
  sprintf(r_source, "%s%s", seq->strip->dir, seq->strip->stripdata->name);
}

static char *draw_seq_text_get_format(void)
{
  /* conditions, switches and whatnot... */
  return "%s: %s | %d";
}

static void draw_seq_text(...)
{
  char overlay_string[32 + FILE_MAX];
  char name[64];
  char source[FILE_MAX];
  char *format = draw_seq_text_get_format();;
  draw_seq_text_get_name(seq, name);
  draw_seq_text_get_source(seq, source);
  int duration = seq->enddisp - seq->startdisp;
  size_t overlay_string_length = BLI_snprintf(overlay_string, sizeof(overlay_string), format, name, source, duration);

  ...BLF_stuff(overlay_string);
}

source/blender/editors/space_sequencer/sequencer_draw.c
26	This include is not needed
635	`stripdata` is not guaranteed to exist and this can crash. So you have to check for stripdata first. `if (seq->strip->stripdata && seq->strip->stripdata->name ) {`
1214	This would be better as precondition inside `draw_sequence_extensions()` function
1230	Same as above

This revision now requires changes to proceed.Dec 6 2020, 6:24 PM

In D9751#242567, @Peter Fog (tintwotin) wrote:

In D9751#242410, @Richard Antalik (ISS) wrote:

This looks very nice. I would maybe say that even showing waveforms could be on/off switch.

Yes, it's a bit odd with a checkbox for on and off, but it's because these two and the third the strip option are working as radio buttons, so either radio button icons should be used instead or some other solution should be found like a checkbox for on/off and then two settings in a dropdown: 'All' or 'Strip Setting' , but this should properly be let for another patch.

Seems reasonable.

Hugo Schouppe (HUSCH) awarded a token.Dec 6 2020, 10:34 PM

In combined view, moved buttons to the right.
Included Preview Overlay + Sequencer/Preview Overlay dropdowns(Frame Overlay included):

Adding removal of identical entries will make the code much more complicated. Personally, I think the default setting should be just 'Source' checked and 'Name' and 'Duration' disabled because it'll cover 99% of users needs, and it will sort of solve the identical doubles problem. Also, it seems to be an industry standard only to draw source as text info on strips.

It seems to me you're suggesting a refactor of the text generation code, I'm not sure I'll be able to pull this off with my limited coding abilities. Maybe this refactor could be done after this patch has been committed? Maybe it'll also make it simpler to do the remove doubles, if needed?

Removed the include.

Stripdata, fixed as suggested.

Check changed to precondition, both places.

In D9751#242611, @Peter Fog (tintwotin) wrote:

Adding removal of identical entries will make the code much more complicated. Personally, I think the default setting should be just 'Source' checked and 'Name' and 'Duration' disabled because it'll cover 99% of users needs, and it will sort of solve the identical doubles problem. Also, it seems to be an industry standard only to draw source as text info on strips.

That sounds reasonable I think

It seems to me you're suggesting a refactor of the text generation code, I'm not sure I'll be able to pull this off with my limited coding abilities. Maybe this refactor could be done after this patch has been committed? Maybe it'll also make it simpler to do the remove doubles, if needed?

I can help you with this, especially if I would have to clean it up later :) That example code was actually working code, just with single format. it's just matter of moving conditions from all over the place to particular function.

Optimized the strip type specific code by a lot.
Set Source text to ON and Name and Duration as OFF, to avoid clutter and redundant Info of strips.

Remove Frame Overlay from View menu - since it is now in the Overlay dropdown.
In the View menus, move Zoom to Fit up together with the rest of the Preview Zoom functions.

Yay for consistency!

I think you can get rid of the Show... at the beginning of the labels. And (outside of the scope of this patch) we should rename Waveform Displaying to Waveform Display, since we don't use Displaying anywhere else.

Other than that, it looks great!
Thanks Peter!

Peter Fog (tintwotin) edited the summary of this revision. (Show Details)Dec 7 2020, 12:41 PM

Peter Fog (tintwotin) edited the summary of this revision. (Show Details)

A bit of housekeeping:
Waveform Displaying renamed to Waveform Display.

Remove "Show" in overlay entries.

In the View menus, move Zoom to Fit up together with the rest of the Preview Zoom functions, to make the menus more user friendly now a lot of entries in the menus have been moved.

Peter Fog (tintwotin) added a project: VFX & Video.Dec 7 2020, 1:02 PM

Peter Fog (tintwotin) edited the summary of this revision. (Show Details)Dec 7 2020, 1:17 PM

Thanks! Looking good from the UI side of things.

Juan Gabriel (Juangra_Membata) awarded a token.Dec 8 2020, 9:15 AM

Getting there, more stuff to look at though.

/home/hans/Documents/Blender-Git/blender/source/blender/editors/space_sequencer/sequencer_draw.c: In function ‘draw_seq_text’:
/home/hans/Documents/Blender-Git/blender/source/blender/editors/space_sequencer/sequencer_draw.c:686:31: warning: ‘%s’ directive writing up to 1055 bytes into a region of size 1053 [-Wformat-overflow=]
  686 |     sprintf(source_print, " | %s", source);
      |                               ^~   ~~~~~~
/home/hans/Documents/Blender-Git/blender/source/blender/editors/space_sequencer/sequencer_draw.c:686:5: note: ‘sprintf’ output between 4 and 1059 bytes into a destination of size 1056
  686 |     sprintf(source_print, " | %s", source);
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

release/scripts/startup/bl_ui/space_sequencer.py
192	Just use `pass`
199	Should be plural
217–218	I think using multiple sub-panels here instead of a single sub-panel with an if statement would make more sense. Here's what I would suggest: class SEQUENCER_PT_overlay(Panel): bl_space_type = 'SEQUENCE_EDITOR' bl_region_type = 'HEADER' bl_label = "Overlays" bl_ui_units_x = 7 def draw(self, _context): pass class SEQUENCER_PT_preview_overlay(Panel): bl_space_type = 'SEQUENCE_EDITOR' bl_region_type = 'HEADER' bl_parent_id = 'SEQUENCER_PT_overlay' bl_label = "Preview Overlays" @classmethod def poll(cls, context): st = context.space_data return st.view_type in {'PREVIEW', 'SEQUENCER_PREVIEW'} and st.display_mode == 'IMAGE' def draw(self, context): ed = context.scene.sequence_editor st = context.space_data layout = self.layout layout.active = st.show_strip_overlay layout.prop(ed, "show_overlay", text="Frame Overlay") layout.prop(st, "show_safe_areas", text="Safe Areas") layout.prop(st, "show_metadata", text="Metadata") layout.prop(st, "show_annotation", text="Annotations") class SEQUENCER_PT_sequencer_overlay(Panel): bl_space_type = 'SEQUENCE_EDITOR' bl_region_type = 'HEADER' bl_parent_id = 'SEQUENCER_PT_overlay' bl_label = "Sequencer Overlays" @classmethod def poll(cls, context): st = context.space_data return st.view_type in {'SEQUENCER', 'SEQUENCER_PREVIEW'} def draw(self, context): st = context.space_data layout = self.layout layout.active = st.show_strip_overlay layout.prop(st, "show_strip_name", text="Name") layout.prop(st, "show_strip_source", text="Source") layout.prop(st, "show_strip_duration", text="Duration") layout.separator() layout.prop(st, "show_strip_offset", text="Offsets") layout.prop(st, "show_fcurves", text="F-Curves") layout.separator() layout.prop_menu_enum(st, "waveform_display_type")
source/blender/blenloader/intern/versioning_290.c
1249–1252	sseq->flag \|= (SEQ_SHOW_STRIP_OVERLAY \| SEQ_SHOW_STRIP_NAME \| SEQ_SHOW_STRIP_SOURCE \| SEQ_SHOW_STRIP_DURATION);
source/blender/editors/space_sequencer/sequencer_draw.c
241–243	It's better to do these checks before calling the function, not inside of it. The function's purpose is to draw something, not check if it should draw it. This applies everywhere you've added these checks, but especially when you added a `SpaceSequencer` argument to the function.
630–632	Some more thought should go into the size of these string buffers. 1024 + 32 is pretty huge, especially for duration, which is just a number. And "source" shouldn't need to be nearly that big, look at the size of the buffers the strings come from. Also, I think `BLI_snprintf` is preferred here.
654	Run clang format so you don't add spacing like this by mistake
700	Shouldn't this be done where `name` is initialized rather than afterwards?
source/blender/makesrna/intern/rna_space.c
5053	I would just remove the description in these cases where it doesn't add anything to the name.

This revision now requires changes to proceed.Dec 8 2020, 5:51 PM

Peter Fog (tintwotin) retitled this revision from VSE: Add Overlay popover to toggle Texts, F-curves, Offsets, Waveforms on/off. to VSE: Add Overlay Popovers..Dec 10 2020, 9:57 AM

Addressed the issues mentioned by @@HooglyBoogly except moving the disable-checks out of the drawing functions, since @Richard Antalik (ISS) asked me move them in there, so this seems undecided, and I'll leave it as it is for now. Also I could use an advise how big the variables should be defined, if the current sizes aren't correct.

release/scripts/startup/bl_ui/space_sequencer.py
217–218	I actually tried this way, but I couldn't get rid of the labels, in the states where the rest of that panel was used, but you found a way. Thank you.

Requesting review.

@Richard Antalik (ISS) Obviously the stakes aren't so high here, but since we should come to some conclusion, I think it makes quite more sense for the checks for the certain cases to be in the parent function.
That way you can see all the cases in the same place, rather than spread out throughout the file. And it keep scope of each function smaller.
Instead of worrying about the state of things, the function will have one job-- draw the strip's name (or something else).

In D9751#244868, @Hans Goudey (HooglyBoogly) wrote:

@Richard Antalik (ISS) Obviously the stakes aren't so high here, but since we should come to some conclusion, I think it makes quite more sense for the checks for the certain cases to be in the parent function.

Sorry I haven't got time to clarify this really. Using precondition was suggested to me as a way to make functions branch less. I would prefer this method even in this case. However I agree that these functions shouldn't do these checks.

I will intervene here as well and update the code a bit.

add _overlay suffix to draw functions controlled by overlays. Make draw_seq_text_overlay() bit less complicated. Move checks for SEQ_SHOW_STRIP_OVERLAY flag outside of individual overlay draw functions

Harbormaster completed remote builds in B11746: Diff 31987.Dec 14 2020, 11:47 PM

Remove unused variables
remove unrelated change

Harbormaster completed remote builds in B11748: Diff 31989.Dec 15 2020, 12:16 AM

@Peter Fog (tintwotin) Please mark inline comments as done when you update code to address them, otherwise it is quite hard to track what is done and what is not.

@Hans Goudey (HooglyBoogly) As for buffer lengths, BLI_snprintf(r_source, source_len, "%s%s", seq->strip->dir, seq->strip->stripdata->name); can produce string 1024 characters long, which is FILE_MAX
Also in draw_seq_text_get_overlay_string() we could execute draw_seq_text_get_xxx() only when SEQ_SHOW_STRIP_XXX is enabled. I am not sure if this can impact performance really, I have never benchmarked this kind of code.

I think @Peter Fog (tintwotin) wanted to get rid of strip name. Now I am thinking we should not do this because some users may consider it very important to have it displayed. I can't really remember exact result of our discussion.

In any case I was hoping to group draw_strip_x_overlay() functions in one draw_strip_overlay() function, but this turned out to be a bit messy and I started changing more and more code. Before I would go too far I reverted these changes. So this really needs separate patch.

The code has been rearranged, so I'm quickly marking Done pr. memory.

"I think @Peter Fog (tintwotin) wanted to get rid of strip name. Now I am thinking we should not do this because some users may consider it very important to have it displayed. I can't really remember exact result of our discussion."

I think users should have the opportunity to switch off all text for ex. using maximum height for waveforms, or editing with non descriptive filenames. In ex. 3D View there is also a toggle button to switch off all overlays, having that in the VSE is both useful and in consistency.

Peter Fog (tintwotin) added a subscriber: • P. Hoffmann (PETER).Dec 15 2020, 7:33 AM

Peter Fog (tintwotin) removed a subscriber: Antonio Vazquez (antoniov).Dec 15 2020, 7:50 AM

@Richard Antalik (ISS) A quick note. When only Source is checked, Adjustment Layer and Effect Strips/Cross doesn't have any text drawn - I think in this case the strip type should be used as source. Try to test with the example file I uploaded above.

filename2.blend683 KBDownload

In D9751#245676, @Peter Fog (tintwotin) wrote:

The code has been rearranged, so I'm quickly marking Done pr. memory.

"I think @Peter Fog (tintwotin) wanted to get rid of strip name. Now I am thinking we should not do this because some users may consider it very important to have it displayed. I can't really remember exact result of our discussion."

I think users should have the opportunity to switch off all text for ex. using maximum height for waveforms, or editing with non descriptive filenames. In ex. 3D View there is also a toggle button to switch off all overlays, having that in the VSE is both useful and in consistency.

What I meant is having only source and duration to choose from. but I think we should have name as well.

In D9751#245688, @Peter Fog (tintwotin) wrote:

@Richard Antalik (ISS) A quick note. When only Source is checked, Adjustment Layer and Effect Strips/Cross doesn't have any text drawn - I think in this case the strip type should be used as source.

I have noticed this, I think it is OK as it is, but you can change or improve this.

In any case I think I am fine with patch as is.

As we talked about, could/should source be the one option which is on as default setting among the text options, and as such I think there should be an entry in source for all strips.

I homeschooling my son these days, so I have very limited time, so I'm fine with you doing what you think is needed and commit it.

Accepting since these final changes are trivial.

source/blender/blenloader/intern/versioning_290.c
1259	Versioning code should go down here in these brackets.
source/blender/editors/space_sequencer/sequencer_draw.c
1169	`avoid to draw` -> `avoid drawing`
source/blender/makesrna/intern/rna_space.c
5053	Not done. For "Show Source", a better description could be "Display [path/name] of the strip's source file". The others should probably just be removed.

This revision is now accepted and ready to land.Dec 15 2020, 3:11 PM

Closed by commit rBfad80a95fd08: VSE: Add Overlay popover panels (authored by Peter Fog (tintwotin), committed by Richard Antalik (ISS)). · Explain WhyDec 15 2020, 11:54 PM

This revision was automatically updated to reflect the committed changes.

Richard Antalik (ISS) added a commit: rBfad80a95fd08: VSE: Add Overlay popover panels.

For the record: I would prefer that all strips would have a text exposed in Source mode, for ex. Effect strips, Adjustment layer and it should be the type, because that would make Source useable as the only text enabled as default setting.

I know that the name setting will often double with the source text(if type is exposed), but since no name has been typed in, there should be no name, also, my guess is that practically no users will actively type in new names for their strips, making the Name setting less useful than Source. The Source setting is also the only way users can see the contents of Scene strips and Text strips written on the strips. Duration is already exposed in the Time panel, if a user occasionally needs it. For ex. editing image sequences, it can be enabled in the dropdown, but it shouldn't be enabled as default setting.

Cleaning up the default text clutter on the strips, by only having the Source enabled, was my main motivation for starting work this patch.

In D9751#246032, @Peter Fog (tintwotin) wrote:

For the record: I would prefer that all strips would have a text exposed in Source mode, for ex. Effect strips, Adjustment layer and it should be the type, because that would make Source useable as the only text enabled as default setting.

I know that the name setting will often double with the source text(if type is exposed), but since no name has been typed in, there should be no name, also, my guess is that practically no users will actively type in new names for their strips, making the Name setting less useful than Source. The Source setting is also the only way users can see the contents of Scene strips and Text strips written on the strips. Duration is already exposed in the Time panel, if a user occasionally needs it. For ex. editing image sequences, it can be enabled in the dropdown, but it shouldn't be enabled as default setting.

Cleaning up the default text clutter on the strips, by only having the Source enabled, was my main motivation for starting work this patch.

Sorry I haven't noticed changes in patch only when I was checking if versioning works as it should, I have made change to your defaults.
We can change what's displayed in source still and even do this without doubling. I wouldn't mind changing defaults, but with this patch some strips would get no text, which is not good. It is better if adjusting defaults is discrete change I would say.

Blender Defender (Blender_Defender) awarded a token.Dec 22 2020, 12:07 PM

VSE: Add Overlay Popovers.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 31658

release/scripts/startup/bl_ui/space_sequencer.py

source/blender/blenloader/intern/versioning_290.c

source/blender/blenloader/intern/versioning_defaults.c

source/blender/editors/space_sequencer/sequencer_draw.c

source/blender/editors/space_sequencer/space_sequencer.c

source/blender/makesdna/DNA_space_types.h

source/blender/makesrna/intern/rna_space.c

VSE: Add Overlay Popovers.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 31658

release/scripts/startup/bl_ui/space_sequencer.py

source/blender/blenloader/intern/versioning_290.c

source/blender/blenloader/intern/versioning_defaults.c

source/blender/editors/space_sequencer/sequencer_draw.c

source/blender/editors/space_sequencer/space_sequencer.c

source/blender/makesdna/DNA_space_types.h

source/blender/makesrna/intern/rna_space.c

VSE: Add Overlay Popovers.
ClosedPublic
Actions

Revision Contents
Changeset List