blender
/
blender
135
420
Fork 601
Code Issues 6.3k Pull Requests 768 Projects 19 Wiki Activity

Animation: Graph Editor curve drawing performance improvement #110301

Merged
Christoph Lendenfeld merged 18 commits from ChrisLend/blender:graph_draw_performance into main 2023-08-03 14:43:29 +02:00
Conversation 1 Commits 18 Files Changed 1 +33 -14
1 changed files with 33 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit d4ccfdf5d9 - Show all commits

47
source/blender/editors/space_graph/graph_draw.cc
View File

@ -865,10 +865,12 @@ static bool fcurve_can_use_simple_bezt_drawing(FCurve *fcu)
static int calculate_bezt_draw_resolution(BezTriple *bezt,
BezTriple *prevbezt,
const int points_per_frame)
const blender::float2 resolution_scale)
{
const int resolution = (int)((bezt->vec[1][0] - prevbezt->vec[1][0]) * points_per_frame);
return resolution;
const int resolution_x = (int)((bezt->vec[1][0] - prevbezt->vec[1][0]) * resolution_scale[0]);
const int resolution_y = (int)(fabs(bezt->vec[1][1] - prevbezt->vec[1][1]) *
nathanvegdahl marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2023-07-24 11:44:47 +02:00
Outdated
Review
Copy Link

What's the idea behind adding the two together? Would be good to have a comment that explains.

What's the idea behind adding the two together? Would be good to have a comment that explains.
Nathan Vegdahl commented 2023-07-24 16:29:01 +02:00
Outdated
Review
Copy Link

I think it might make sense to do this differently anyway. In particular, this doesn't take into account the handles of the f-curve segment. For example, you could have two keys that are very close together, but with handles that extend very far vertically, creating a tall narrow hump (like in this image). Drawing that accurately requires a lot more resolution than just the key frame distance would suggest.

I think the simplest robust thing to do would be to take the bounding box of the segment's four control points (i.e. the keys and inner handles) and base the resolution on the maximum dimension of that bounding box in viewport space.

I also suspect we'll want to keep a maximum resolution, and maybe add a minimum resolution. But they can probably just be hard-coded rather than being a parameter.

The maximum resolution is useful because in practice beyond a certain number of samples you're very unlikely to visually approximate the cubic curve any better at any "reasonable" scale. And it would help prevent unexpected performance drops when a really long/tall segment just barely peeks into the viewport range. Blender's 3d bezier curves cap the resolution at a maximum of 1024, so maybe use the same here. Although probably just 256 or even 128 would be reasonable.

The minimum resolution could be as low as 2 points, just to avoid accidentally doing something degenerate. But I think 4 points is a good default, since that way it can always at least reflect the possible points of inflection of the cubic curve.

I think it might make sense to do this differently anyway. In particular, this doesn't take into account the handles of the f-curve segment. For example, you could have two keys that are very close together, but with handles that extend very far vertically, creating a tall narrow hump (like in [this image](https://perm.cessen.com/2023/blender_dev/tall_curve.jpg)). Drawing that accurately requires a lot more resolution than just the key frame distance would suggest. I think the simplest robust thing to do would be to take the bounding box of the segment's four control points (i.e. the keys and inner handles) and base the resolution on the maximum dimension of that bounding box in viewport space. ~~I also suspect we'll want to keep a maximum resolution, and maybe add a minimum resolution. But they can probably just be hard-coded rather than being a parameter.~~ ~~The maximum resolution is useful because in practice beyond a certain number of samples you're very unlikely to visually approximate the cubic curve any better at any "reasonable" scale. And it would help prevent unexpected performance drops when a *really* long/tall segment just barely peeks into the viewport range. Blender's 3d bezier curves cap the resolution at a maximum of 1024, so maybe use the same here. Although probably just 256 or even 128 would be reasonable.~~ ~~The minimum resolution could be as low as 2 points, just to avoid accidentally doing something degenerate. But I think 4 points is a good default, since that way it can always at least reflect the possible points of inflection of the cubic curve.~~
Nathan Vegdahl commented 2023-07-24 17:13:50 +02:00
Outdated
Review
Copy Link

Re: maximums and minimums, never mind. It looks like those are in add_bezt_vertices(), which probably makes more sense.

Re: maximums and minimums, never mind. It looks like those are in `add_bezt_vertices()`, which probably makes more sense.
Christoph Lendenfeld commented 2023-07-24 20:46:35 +02:00
Outdated
Review
Copy Link

how about I move the call to calculate_bezt_draw_resolution into add_bezt_vertices
that way it is in the same place as the clamping happens. I would then just pass the float2 resolution_scale to add_bezt_vertices
regarding the bounding box including the handles, good idea. Will add that

how about I move the call to `calculate_bezt_draw_resolution` into `add_bezt_vertices` that way it is in the same place as the clamping happens. I would then just pass the `float2 resolution_scale` to `add_bezt_vertices` regarding the bounding box including the handles, good idea. Will add that
Nathan Vegdahl commented 2023-07-25 11:33:32 +02:00
Outdated
Review
Copy Link

I think I slightly prefer the call being in draw_fcurve_curve_bezts() as it is now. It keeps things a little "flatter" (you aren't chasing functions that call functions that call functions).

But I don't think it matters much either way. In this case, my mis-comment was just me being a goof ball and not reading the whole PR before submitting my comments.

I think I slightly prefer the call being in `draw_fcurve_curve_bezts()` as it is now. It keeps things a little "flatter" (you aren't chasing functions that call functions that call functions). But I don't think it matters much either way. In this case, my mis-comment was just me being a goof ball and not reading the whole PR before submitting my comments.
Christoph Lendenfeld commented 2023-07-27 11:37:43 +02:00
Outdated
Review
Copy Link

added a comment and included the handles in the calculations

added a comment and included the handles in the calculations
resolution_scale[1]);
return resolution_x + resolution_y;
}
/**
nathanvegdahl marked this conversation as resolved Outdated
Nathan Vegdahl commented 2023-07-24 20:40:13 +02:00
Outdated
Review
Copy Link

I believe this note about immBeginAtMost should now be moved to draw_fcurve_curve_bezts(), since this function no longer makes any graphics calls.

(And just generally this doc comment can be updated to reflect what the function actually does now, but Sybren mentioned that already.)

I believe this note about `immBeginAtMost` should now be moved to `draw_fcurve_curve_bezts()`, since this function no longer makes any graphics calls. (And just generally this doc comment can be updated to reflect what the function actually does now, but Sybren mentioned that already.)
Christoph Lendenfeld commented 2023-07-27 11:32:34 +02:00
Outdated
Review
Copy Link

good point. updated the comment

good point. updated the comment
@ -1003,9 +1005,29 @@ static void get_extrapolation_point_right(FCurve *fcu,
curve_vertices.append(vertex_position);
}
static blender::float2 calculate_resolution_scale(View2D *v2d)
{
/* The resolution for bezier forward diff in frame/value space. This ensures a constant
* resolution in screenspace. */
const int window_width = BLI_rcti_size_x(&v2d->mask);
const int window_height = BLI_rcti_size_y(&v2d->mask);
const float points_per_pixel = 0.25f;
const float v2d_frame_range = BLI_rctf_size_x(&v2d->cur);
const float v2d_value_range = BLI_rctf_size_y(&v2d->cur);
const blender::float2 resolution_scale = {(window_width * points_per_pixel) / v2d_frame_range,
(window_height * points_per_pixel) / v2d_value_range};
return resolution_scale;
}
/* Helper function - draw one repeat of an F-Curve (using Bezier curve approximations). */
static void draw_fcurve_curve_bezts(
bAnimContext *ac, ID *id, FCurve *fcu, View2D *v2d, uint pos, const bool draw_extrapolation)
static void draw_fcurve_curve_bezts(bAnimContext *ac,
ID *id,
FCurve *fcu,
View2D *v2d,
uint pos,
const blender::float2 resolution_scale,
const bool draw_extrapolation)
{
using namespace blender;
if (!draw_extrapolation && fcu->totvert == 1) {
@ -1020,13 +1042,6 @@ static void draw_fcurve_curve_bezts(
GPU_matrix_scale_2f(1.0f, unit_scale);
GPU_matrix_translate_2f(0.0f, offset);
const int window_width = BLI_rcti_size_x(&v2d->mask);
const int points_per_pixel = 1;
const int max_points = window_width * points_per_pixel;
const float v2d_frame_range = BLI_rctf_size_x(&v2d->cur);
const float points_per_frame = max_points / v2d_frame_range;
Vector<float2> curve_vertices;
/* Extrapolate to the left? */
@ -1047,6 +1062,7 @@ static void draw_fcurve_curve_bezts(
curve_vertices.append(
nathanvegdahl marked this conversation as resolved Outdated
Nathan Vegdahl commented 2023-07-24 20:29:05 +02:00
Outdated
Review
Copy Link

It took me a bit of thinking to realize the conditions that this handles:

  1. There is only a single vertex in the fcurve.
  2. The curve is completely outside the view (either to the left or right).

Might be worth adding a brief comment mentioning that, so people don't need to puzzle it out.

It took me a bit of thinking to realize the conditions that this handles: 1. There is only a single vertex in the fcurve. 2. The curve is completely outside the view (either to the left or right). Might be worth adding a brief comment mentioning that, so people don't need to puzzle it out.
Christoph Lendenfeld commented 2023-07-27 11:33:39 +02:00
Outdated
Review
Copy Link

good idea. done that

good idea. done that
{fcu->bezt[first_bezt_index].vec[1][0], fcu->bezt[first_bezt_index].vec[1][0]});
}
/* Draw curve between first and last keyframe (if there are enough to do so). */
for (int i = first_bezt_index + 1; i <= last_bezt_index; i++) {
BezTriple *prevbezt = &fcu->bezt[i - 1];
@ -1063,7 +1079,7 @@ static void draw_fcurve_curve_bezts(
curve_vertices.append({prevbezt->vec[1][0], prevbezt->vec[1][1]});
}
else if (prevbezt->ipo == BEZT_IPO_BEZ) {
const int resolution = calculate_bezt_draw_resolution(bezt, prevbezt, points_per_frame);
const int resolution = calculate_bezt_draw_resolution(bezt, prevbezt, resolution_scale);
add_bezt_vertices(bezt, prevbezt, resolution, curve_vertices);
}
@ -1109,6 +1125,8 @@ static void draw_fcurve(bAnimContext *ac, SpaceGraph *sipo, ARegion *region, bAn
if (((fcu->modifiers.first) || (fcu->flag & FCURVE_INT_VALUES)) ||
(((fcu->bezt) || (fcu->fpt)) && (fcu->totvert)))
{
const blender::float2 resolution_scale = calculate_resolution_scale(&region->v2d);
std::cout << "ppf: " << resolution_scale << std::endl;
/* set color/drawing style for curve itself */
/* draw active F-Curve thicker than the rest to make it stand out */
if (fcu->flag & FCURVE_ACTIVE && !BKE_fcurve_is_protected(fcu)) {
@ -1182,7 +1200,8 @@ static void draw_fcurve(bAnimContext *ac, SpaceGraph *sipo, ARegion *region, bAn
/* just draw curve based on defined data (i.e. no modifiers) */
if (fcu->bezt) {
if (fcurve_can_use_simple_bezt_drawing(fcu)) {
draw_fcurve_curve_bezts(ac, ale->id, fcu, &region->v2d, shdr_pos, draw_extrapolation);
draw_fcurve_curve_bezts(
ac, ale->id, fcu, &region->v2d, shdr_pos, resolution_scale, draw_extrapolation);
}
else {
draw_fcurve_curve(ac, ale->id, fcu, &region->v2d, shdr_pos, false, draw_extrapolation);