Page MenuHome
Differential D7205

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

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

Details

Reviewers
Richard Antalik (ISS)
Commits
rBd0d20de183f1: VSE: Draw f-curves for opacity and volume values on the strips
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) created this revision.Mar 21 2020, 12:32 PM
Alessio Monti di Sopra (a.monti) updated this revision to Diff 22996.Mar 21 2020, 4:06 PM
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
Alessio Monti di Sopra (a.monti) updated this revision to Diff 23013.Mar 22 2020, 4:21 PM
  • 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
Richard Antalik (ISS) added a comment.Mar 23 2020, 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... 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.Mar 23 2020, 9:03 AM
This revision now requires changes to proceed.Mar 23 2020, 9:03 AM
Alessio Monti di Sopra (a.monti) added a comment.Mar 23 2020, 9:40 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.

Alessio Monti di Sopra (a.monti) added inline comments.Mar 23 2020, 9:57 AM
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.

Richard Antalik (ISS) added a comment.Mar 23 2020, 11:32 AM
In D7205#172378, @Alessio Monti di Sopra (a.monti) wrote:

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.

Alessio Monti di Sopra (a.monti) added inline comments.Mar 23 2020, 12:07 PM
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.

Alessio Monti di Sopra (a.monti) updated this revision to Diff 23061.EditedMar 24 2020, 3:24 PM
  • addressed inline comments
    • added preconditions
    • changed variable names
  • get seq->start/end from the main function
Alessio Monti di Sopra (a.monti) marked 3 inline comments as done.Mar 24 2020, 3:25 PM
Richard Antalik (ISS) requested changes to this revision.Mar 24 2020, 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.Mar 24 2020, 11:50 PM
noki paike (amonpaike) awarded a token.Mar 26 2020, 8:34 AM
Alessio Monti di Sopra (a.monti) updated this revision to Diff 23139.Mar 26 2020, 4:34 PM
  • moved adding vertices in separate function
  • used floor(pixelx) for step values higher than 1
  • changed max_verts calculation
  • inverted logic for precondition
Richard Antalik (ISS) accepted this revision.Mar 27 2020, 3:21 PM
This revision is now accepted and ready to land.Mar 27 2020, 3:21 PM
Closed by commit rBd0d20de183f1: VSE: Draw f-curves for opacity and volume values on the strips (authored by Richard Antalik (ISS)). · Explain WhyApr 6 2020, 1:04 AM
This revision was automatically updated to reflect the committed changes.
Richard Antalik (ISS) added a commit: rBd0d20de183f1: VSE: Draw f-curves for opacity and volume values on the strips.

Revision Contents
Changeset List

PathSize
release/
scripts/
startup/
bl_ui/
1 line
source/
blender/
blenloader/
intern/
11 lines
2 lines
editors/
space_sequencer/
100 lines
2 lines
makesdna/
2 lines
makesrna/
intern/
5 lines

Diff 23013

View Options

release/scripts/startup/bl_ui/space_sequencer.py

Loading...
View Options

source/blender/blenloader/intern/versioning_280.c

Loading...
View Options

source/blender/blenloader/intern/versioning_defaults.c

Loading...
View Options

source/blender/editors/space_sequencer/sequencer_draw.c

Loading...
View Options

source/blender/editors/space_sequencer/space_sequencer.c

Loading...
View Options

source/blender/makesdna/DNA_space_types.h

Loading...
View Options

source/blender/makesrna/intern/rna_space.c

Loading...