Page MenuHome

VSE: draw f-curves for opacity and volume values on the strips
AcceptedPublic

Authored by Alessio Monti di Sopra (a.monti) on Sat, Mar 21, 12:32 PM.

Details

Summary

Initial implementation of f-curve drawing for the video sequencer.

The patch adds a flag in the VSE's View menu (active by default) for drawing opacity and volume f-curves as darker regions of the strips.
A similar solution could also be used for the NLA editor's influence curves. (currently drawn as gray lines).


To get better performance:

  • Only the visible sections of the curve (inside the editor borders) are evaluated.
  • The amount of frames for which the curve is evaluated depends on the zoom level.
  • Avoid adding verts for points next to each others that have the same value

Further performance improvements could probably be achieved in curve lookups.

Diff Detail

Repository
rB Blender

Event Timeline

Alessio Monti di Sopra (a.monti) edited the summary of this revision. (Show Details)
  • moved evaluation steps to integers
  • add an extra evaluation frame left/right of the editor's borders
  • avoid adding verts for points next to each others that have the same value
  • further reduce resolution of the curve at lower zoom level
  • add the last couple of vertices out of the main loop, in order to make sure that the shape is always properly closed

I have done comparison of batch vs immediate drawing, and there is little to no improvement in performance. I did not expect that... I will do one more comparison where all curves will be drawn in one go, to see if that will improve things by some significant margin. Though drawing all curves at one go would require refactoring of strip drawing with multiple strips iterations, which is not quite elegant...

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

Add !BKE_fcurve_is_empty(fcu) to precondition.

1008–1009

I would use eval_start or eval_start_frame
Same for end

It can happen, that start > end, in that case we shouldn't continue

1022

define prev_ev here so and call it prev_y
Or perhaps curve_val would be even better

1025

Shouldn't this be cfra <= end_ev ?

If you want to be pedantic, you can ensure evaluation on end_ev frame.

1030

&& cfra < end_ev - step

Then you won't need

/* Add last two verts to make sure that the shape is closed properly. */
y = evaluate_fcurve(fcu, end_ev);
copy_v2_fl2(vert_pos[0], end_ev, (y * y_height) + y1);
copy_v2_fl2(vert_pos[1], end_ev, y2);

GPU_vertbuf_vert_set(vbo, vert_count, vert_pos[0]);
GPU_vertbuf_vert_set(vbo, vert_count + 1, vert_pos[1]);
vert_count += 2;
Richard Antalik (ISS) requested changes to this revision.Mon, Mar 23, 9:03 AM
This revision now requires changes to proceed.Mon, Mar 23, 9:03 AM

I have done comparison of batch vs immediate drawing, and there is little to no improvement in performance. I did not expect that...

Yes, I've also realized that..
More in general did you notice major performance drops while enabling the curves? It didn't seem the case to me any more now, at least by looking at the playback fps.

source/blender/editors/space_sequencer/sequencer_draw.c
1008–1009

It can happen, that start > end, in that case we shouldn't continue

How could that happen?
I've already checked before that the strip is not inverted.

I have done comparison of batch vs immediate drawing, and there is little to no improvement in performance. I did not expect that...

Yes, I've also realized that..
More in general did you notice major performance drops while enabling the curves? It didn't seem the case to me any more now, at least by looking at the playback fps.

With 10 strips 1M frames long, when extremely zoomed out I got drop from 100 to 80 FPS. I saw UI_view2d_constant_grid_draw using way too much CPU though, but that is just BTW.
With 500 strips 50 frames long there is significant drop from 100 to 35 FPS, fcurve lookup is more significant in this case.

In normal use I don't really see effect on performance here.

Still would be good to optimize things, but overall I wouldn't consider performance to be showstopper in this case.

For lot of small strips (split image sequences) we could add condition, that at least n points have to be drawn. that is only legitimate situation, I can imagine to be most impacted

source/blender/editors/space_sequencer/sequencer_draw.c
1008–1009

While I was panning in timeline it happened. It doesn't seem it should just from looking at code, but I haven't looked too deep.

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

We still need to make sure that the last verts are drawn.

Putting them out of the loop seemed the simpler solution to me, otherwise I think we would need to precisely calculate the steps in order to always include the end frame, and I don't think it's worth it.

  • addressed inline comments
    • added preconditions
    • changed variable names
  • get seq->start/end from the main function
Richard Antalik (ISS) requested changes to this revision.Tue, Mar 24, 11:50 PM
Richard Antalik (ISS) added inline comments.
source/blender/editors/space_sequencer/sequencer_draw.c
999

It must be if (!fcu && BKE_fcurve_is_empty(fcu)) {
otherwise it will crash if fcu is NULL

1030

Hmm if you did step = max_ii(1, pixelx); you can't really make this fail, because last sample would be overwritten by draw_seq_outline(). I guess you evaluated every 2 pixels to make evaluation faster?

Anyway, move this code to small function at least, so it is not repeated 3 times.

This revision now requires changes to proceed.Tue, Mar 24, 11:50 PM
  • moved adding vertices in separate function
  • used floor(pixelx) for step values higher than 1
  • changed max_verts calculation
  • inverted logic for precondition
This revision is now accepted and ready to land.Fri, Mar 27, 3:21 PM