Page MenuHome

VSE: changes to the strips drawing
ClosedPublic

Authored by Alessio Monti di Sopra (a.monti) on Feb 18 2020, 6:22 PM.
Tokens
"100" token, awarded by gobb_blend."Like" token, awarded by slumber."Love" token, awarded by antoniov."Pterodactyl" token, awarded by Pipeliner."Love" token, awarded by Draise."Love" token, awarded by Andrea_Monzini.

Details

Summary

This patch is derived from D6846 by @Peter Fog (tintwotin), and keeps mostly the same general look regarding handles and outlines.
Discussion between us and on the #vse channel on blender.chat then expanded significantly the scope of the patch.

Before and after:


The changes in the patch are the following:

  • Selection indication changed towards an outline based system, this in order to easily spot the selected strips and make the video sequencer more consistent with other parts of Blender:
    • Selected strips have a 2px wide outline, white for the active strip, orange for the other ones (added to the theme). Before this was indicated by darkening the strip, and use white text.
    • Unselected strips have a regular 1px outline, of a darker shade of the background color.
  • Active strip is indicated by it having a white text and, when selected, a white outline.

  • Handles:
    • Handles are now smaller and drawn as semi-transparent boxes (their active zone is still the same as before, allowing for some margin of error during mouse selection).
    • When selected they are highlighted with the same color scheme of the outlines (white for active strip, orange for selected strips).
    • They are not drawn when the strip is locked, since they have no function in that state.

  • Muted strips:
    • Increased transparency to better indicate the state.
    • For the same reason text color gets dimmered (slightly brighter for regular strips, slightly darker for the active one).
  • Trim numbers:
    • They are now drawn inside the strip, in the lower part next to the handles, in order to make it easier to understand which label belong to which handle.
    • They are now only drawn when the strip is selected, to have more focus on the translating strips.
    • They get hidden when there is not enough horizontal space, to avoid overlappings.

  • Text drawing changes:
    • Drawn on the top-left part of the strip.
    • Drawn in the center under a certain zoom level threshold.
    • Not drawn when there is not enough space (useful for a general view of the timeline).

  • Color strips: A generic color is added to the theme, while the actual color data of the strip is drawn on the bottom half, when there is enough vertical space. This in order to better distinguish them on the timeline and making so that the strip text is always visible, no matter what color the strip is.

  • Sound strips:
    • When waveforms drawing is active, both text and waveforms are shown, since this has been requested by users.
    • If there is not enough vertical space, waveforms (if enabled) are preferred over text.

  • Transition strips:
    • Removed theme color, transitions are now composed of two triangles and use their input strips colors, since this will make them easier to recognize.
    • If one of the inputs is itself a transition, a fallback grey color is used.
    • If the two input strips are of the same type, the right triangle is drawn slightly darker.

  • Effect strips:
    • Changed theme colors to better indicate categories within the effects.
    • Their input strips are now highlighted when the effect strip is the active one and selected, this makes it much easier to quickly see which strips are the inputs of effect strips.

  • When Multicam strips are the active one and selected, its source channel is highlighted, since it was hard to locate it otherwise.

  • Strip offsets and hold still regions:
    • Hold still regions are now always drawn if existent, with a darker shade of the strip’s background color. This makes it easier to understand that the strip is till affecting the timeline, and to read the strip's text.
    • As a consequence, “Show offsets” and “Special preview” now only show the offsets.
    • Offsets are highlighted when the strip is selected (previously they were darkened).

  • Meta strips:
    • The insides of the meta range are no longer highlighted.
    • A checkerboard pattern is now drawn on the outsides of the meta range, to indicate that the content only the content inside is currently visible.

  • Missing media state is now indicated by a red line drawn on the top part of the strip, still very visible but less aggressive than the previous diagonal strips pattern.

Diff Detail

Repository
rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Alessio Monti di Sopra (a.monti) retitled this revision from VSE: changes to the drawing of handles, selected and active strips to VSE: changes to the strips drawing.
Alessio Monti di Sopra (a.monti) edited the summary of this revision. (Show Details)
  • quite big update, changed summary accordingly

Initial review of changes (not full code-review).


Tested the patch and find it makes it harder to know when you will select a handle or not, as noted in the patch submission.

Handles are now smaller and drawn as semi-transparent boxes (their active zone is still the same as before, allowing for some margin of error during mouse selection).

Previously, you can tell if the cursor is inside the region that you will select the handle, especially since this margin changes based on zoom-level.

I found it ended up being guesswork to know if I was going to select a handle or not.


Also noticed this shows white text over white selection handle at times.

@Peter Fog (tintwotin) it's policy not to post screenshots of other software I can't find the link to this just this moment.

I would suggest drawing handles in their clickable sizes on mouseover.

On mouse-not-over they could be like they are or perhaps not drawn at all.

I would suggest drawing handles in their clickable sizes on mouseover.
On mouse-not-over they could be like they are or perhaps not drawn at all.

That would be nice, need to find a different way to draw them in that case though, otherwise you'd have a lot of empty space before the text. Notice also that when the strip is too short, the selection size goes up to 1/3 of the strip (always been like that).

We could maybe draw them like they are now (in this patch), but highlight when the mouse enters the active zone.

Also noticed this shows white text over white selection handle at times.

I can't reproduce this, could be a pixel size problem? What we've done there is guessing how much space a number occupies, and move the label to the right accordingly.
Ok, fixed using U.dpi_fac

I was trying to use BLF_width() for another thing, but I got a lot of jumps in the text, and since those labels worked for both me and @Peter Fog (tintwotin) I thought that was working.

What about adjusting the cursor slightly when it's over the handles?

Note, changing handles on mouse over is something Blender has been avoiding, see: https://wiki.blender.org/wiki/Reference/AntiFeatures#Pre_Selection_Highlighting

What about adjusting the cursor slightly when it's over the handles?

That would be reverse of pre-highlight :)

Note, changing handles on mouse over is something Blender has been avoiding, see: https://wiki.blender.org/wiki/Reference/AntiFeatures#Pre_Selection_Highlighting

I always read this in 3D view context or in context of disambiguation of selection. I don't want to do that, I want to highlight a control element so user can "aim" better.
So I take back "not draw at all" comment

Nearly all UI controls are highlighted in similar way

Is changing the mouse cursor in the active zone also not allowed?
(This is another NLE industry standard to use the mouse cursor to show what will happen if the user click/drag at that position)

So, if I wanted to experiment a bit with highlights or different mouse cursors, what would be the correct way to get the mouse position during a draw function?

So, if I wanted to experiment a bit with highlights or different mouse cursors, what would be the correct way to get the mouse position during a draw function?

Good point... To implement this handle would have to be a widget. I don't think it would be a good idea to implement in this patch.

Otherwise VSE would have to have special event handler for this? I guess, that no other editor does this, but I am really unsure here.

So I take again my comment back...

Changing the cursor on rollover is not an anti-feature. We already do this in Blender in many places. This is highly encouraged.

Changing the cursor on rollover is not an anti-feature. We already do this in Blender in many places. This is highly encouraged.

+1

Please compare the selection of handles in this patch with the Edit mode in the 3D View when selecting a vertex, where there are no action zone markings, no hover mouse cursor highlights, no mouse cursor snapping or changes in mouse cursor look to help users guess where to click:


All of which has been suggested above for handle selection, but compared to the size of a vertex, the handle indicators in this patch are huge, and like the vertex, the patch handle has an action zone, which is much larger than the selection element. So in other words: this patch offers a much more clear selection indicator compared to vertex selection, and besides that, is in consistency with the general and current selection paradigm in Blender.

Introducing a new selection paradigm should not be within the scope of this patch, but belongs to a patch and a discussion of its own.

Please compare the selection of handles in this patch with the Edit mode in the 3D View when selecting a vertex, where there are no action zone markings, no hover mouse cursor highlights, no mouse cursor snapping or changes in mouse cursor look to help users guess where to click:

This is just a bad limitation that might be improved in the future. See:
https://devtalk.blender.org/t/preselection-highlighting/4267/11?u=thinkingpolygons

All 3d apps have it but blender.

For now I found only one not so significant issue, will resume review tomorrow.

source/blender/editors/space_sequencer/sequencer_draw.c
530

This doesn't really work when you scale interface.

I would consider using approach like in draw_horizontal_scale_indicators()
So you will know what is width of the text and after that you can draw it where you want.

Same applies for drawing text in strip - with UI scaled up height goes beyond strip boundary

source/blender/editors/space_sequencer/sequencer_draw.c
540–567

This could be separate function.

599–600

It seems to me that this isn't really doing anything.

You can use seq->startdisp and seq->enddisp directly

606–613

You could use code below, but it would be scaled with UI, Not sure if that is really desirable here.
Also doesn't seem to work that well with sharp edges.

GPU_line_width(2.0f);
imm_draw_box_wire_2d(pos, x1, y1, x2, y2);
733

Why 20? it could be as well 0.

737

I don't have problem with ? operator but I am not sure if I like it in function arguments.

There is no guideline for this though.

768–774

Put precondition at start of function, it's more readable.
This could be put in function immediately after pixely is calculated, so in caller func not here.

I am not so sure this can even happen - I fuzzed UI for a while and pixely was never 0. But to be on safe side I guess we could keep this, I don't know UI code so well to tell.

809

I can not see difference of drawing muted strip extension with just MUTE_ALPHA Or is -30 offset consistent with something?

989

This function was quite big already, I would suggest to split

if (something_special){...} to if (something_special){draw_something_special()}

1010–1011

This could be separate function as well that would give you background_col (with aplha)

1048

Once this is separate function, make precondition:

if (seq->startdisp <= seq->enddisp){
  return;
}
1087–1091
immBegin(GPU_PRIM_TRIS, 3);
float vert_pos[3][2];
copy_v2_fl2(vert_pos[0], x1, y2);
copy_v2_fl2(vert_pos[1], x2, y2);
copy_v2_fl2(vert_pos[2], x2, y1);
immVertex2fv(pos, vert_pos[0]);
immVertex2fv(pos, vert_pos[1]);
immVertex2fv(pos, vert_pos[2]);
immEnd();

and remove v1[2], v2[2], v3[2]

1853–1854

Move to own function

2242

I am lost here, what does this do?

  • added changes to light theme
  • handles selection area fixed to the double of their draw size, should be more predictable since not related to zoom level or strip size any more
  • split various elements of draw_seq_strip() into multiple functions
  • used BLF_width() to calculate space occupied by trim numbers
Alessio Monti di Sopra (a.monti) added inline comments.
source/blender/editors/space_sequencer/sequencer_draw.c
606–613

This was done for compatibility issues with Macs, since it doesn't support glLines wider than 1px

733

Was meant to make the text less harsh, I tried with a value of 10, darker but still not so a punch in the face as 0.
We could link these to theme values, but I wasn't sure what could have been used @William Reynish (billreynish)

2242

Not sure why that is needed, but it solved a draw issue for the frame overlay (was looking around at other examples of that shader).

After a box selection the two colors were becoming two shades of gray.

Richard Antalik (ISS) requested changes to this revision.Thu, Mar 12, 11:43 AM

Forgot to set state previously...

source/blender/editors/space_sequencer/sequencer_draw.c
606–613

Than leave comment ideally with bug report number why this is done in such way

733

I guess you must have better eyesight, I can not see difference on 1px thick line.
That was my only objection, nitpick anyway...

2242

Ah this seems to be correct fix then

This revision now requires changes to proceed.Thu, Mar 12, 11:43 AM
  • commented outline code regarding macOS issues with gpu lines wider than 1px
  • (forgot to mention in previous update) now threshold for text and elements draw/alignment also works based on UI scale

Checked all the changes - seems ok to me.

We should probably look to make similar change to the NLA too.

Could you consider change the cursor that appear when you drag the handles?

That would make more sense, since you can only drag left or right.. IMHO...

This patch should be split up, so each change can be properly reviewd (some wont need changes).

Did anyone check on the overhead of f-curve drawing? From a quick look I would guess it's possible to make the video editor noticeably slower as every strip draw call does a lookup on all f-curves.

It'd be more efficient to put all f-curves with a sequencer prefix into a hash table when this option is enabled.

source/blender/editors/space_sequencer/sequencer_draw.c
751

Cast no longer needed.

Campbell Barton (campbellbarton) requested changes to this revision.Mon, Mar 16, 7:26 AM
This revision now requires changes to proceed.Mon, Mar 16, 7:26 AM

This patch should be split up, so each change can be properly reviewd (some wont need changes).

I think splitting here is wouldn't be problematic, but I don't mind it as is.

Did anyone check on the overhead of f-curve drawing? From a quick look I would guess it's possible to make the video editor noticeably slower as every strip draw call does a lookup on all f-curves.
It'd be more efficient to put all f-curves with a sequencer prefix into a hash table when this option is enabled.

Performance drop was noticable with 48 strips of only 20s length. So this concern is justified.
After looking at this with profiler, 60% of CPU time was spent on drawing curves. Of that 50% was fcurve lookup or evaluation. Depends on timeline content.

I think that batch drawing would be good start here, same can be done for sound waveforms for waveforms - that is easy stuff

It would be best to also store precomputed data of fcurve graph. Not sure about difficulty of implementing such cache, don't know much about life cycle and other tech details of fcurves myself, so not sure if @Alessio Monti di Sopra (a.monti) would like to have a go on this.

AFAIK this drawing has been taken from NLA editor, So same objection of speed would apply there as well.
Perhaps we just shouldn't enable this overlay by default?

Alessio Monti di Sopra (a.monti) edited the summary of this revision. (Show Details)
  • removed previously added header that wasn't needed
  • removed f-curves drawing function, will try to implement that properly in a separate patch

@Campbell Barton (campbellbarton) Since a lot of changes are related to each other in one way or another, I'd personally prefer to keep everything together as much as possible. Apart from f-curves, are there some other specific parts that you wanted to review separately?

  • forgot to update color of trim numbers after UI_view2d_text_cache_add() changes
  • forgot to remove part of f-curves drawing related changes
  • removed previously added header that wasn't needed
  • removed f-curves drawing function, will try to implement that properly in a separate patch

@Campbell Barton (campbellbarton) Since a lot of changes are related to each other in one way or another, I'd personally prefer to keep everything together as much as possible. Apart from f-curves, are there some other specific parts that you wanted to review separately?

Good choice since this is outside the scope of drawing tweaks.

  • better vertical position for the text, allowing to show more of the strip content (color bands, waveforms) when the strip is wider.
  • color strips theme entry changed to a light gray, in order to avoid confusion with the actual color content.

  • small fix to the text margin, now it considers UI scale
  • use that margin also for missing media indication
  • split code for invalid and locked states in separate functions
  • same for code calculating text x-axis offsets
This revision is now accepted and ready to land.Thu, Mar 19, 12:12 AM
source/blender/editors/space_sequencer/sequencer_draw.c
459

Should be scaled by U.pixelsize so hi-dpi screens don't have small handles.

This revision was automatically updated to reflect the committed changes.
source/blender/editors/space_sequencer/sequencer_draw.c
459

Oops, too late.

I am not sure if U.pixelsize is apropriate here, because it is referred to as Line Width
If anything, we could set this size in milimeters or imperial and use DPI to calculate width? Or use proportion of screen size.

Mixing absolute and relative sizes may result in a mess however. (in webdesign at least in my experience, UI will be likely same)

In any case I suspect we would like to change cursor on mouseover and make handle sizes even thinner...