Animation: Common curve drawing for FCurves #110764

Christoph Lendenfeld · 2023-08-03T16:32:56+02:00

2023-08-03 16:32:56 +02:00

Before this patch, the drawing code iterated the FCurve to see if there are any
curve interpolation types that can't easily be drawn. (Sinusoidal, Bounce, etc)
If it found one, it would fall back to evaluating the FCurve.

That meant in the worst case scenario it would iterate the whole FCurve
before even starting to draw.

This PR unifies the drawing logic for FCurves no matter their interpolation type.
If it encounters a key type that it can't draw, it falls back to samples, but only
for the current key.

To clarify that it renames draw_fcurve_curve_bezts to draw_fcurve_curve_keys

Curves with modifiers are still drawn with samples.

Performance

Test setup: 6000f of dense data on 62 bones. Only measuring the average draw time per curve. All measurements were done at the same zoom level.

-	before	after
only beziers	28μs	13μs
only elastic	90μs	60μs
mix (~1/2 of the view with elastic)	110μs	24μs

The performance boost with "only elastic" can be explained by the fact that per key I can skip 1 call to evaluate_fcurve. That is because I always start at an existing keyframe so I can use its position.

Before this patch, the drawing code iterated the FCurve to see if there are any curve interpolation types that can't easily be drawn. (Sinusoidal, Bounce, etc) If it found one, it would fall back to evaluating the FCurve. That meant in the worst case scenario it would iterate the whole FCurve before even starting to draw. This PR unifies the drawing logic for FCurves no matter their interpolation type. If it encounters a key type that it can't draw, it falls back to samples, but only for the current key. To clarify that it renames `draw_fcurve_curve_bezts` to `draw_fcurve_curve_keys` Curves with modifiers are still drawn with samples. ## Performance Test setup: 6000f of dense data on 62 bones. Only measuring the average draw time **per curve**. All measurements were done at the same zoom level. | - | before | after | | - | - | - | | only beziers | 28μs | 13μs | | only elastic | 90μs | 60μs | | mix (~1/2 of the view with elastic) | 110μs | 24μs | The performance boost with "only elastic" can be explained by the fact that per key I can skip 1 call to `evaluate_fcurve`. That is because I always start at an existing keyframe so I can use its position.

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2023-08-03 16:32:56 +02:00

Christoph Lendenfeld added 2 commits 2023-08-03 16:33:07 +02:00

remove need to iterate FCurve before drawing 1f3a0bfde4

decrease sample resolution 24095575b7

Christoph Lendenfeld added this to the Module: Animation & Rigging project 2023-08-04 11:39:22 +02:00

Christoph Lendenfeld requested review from Nathan Vegdahl 2023-08-04 11:42:04 +02:00

Nathan Vegdahl requested changes 2023-08-04 17:29:41 +02:00

Nathan Vegdahl left a comment

Love this change. Way clearer and simpler this way.

Just one issue with computing the step size of the fcurve evaluation. (Very much the kind of mistake I would make, too!)

Love this change. Way clearer and simpler this way. Just one issue with computing the step size of the fcurve evaluation. (Very much the kind of mistake I would make, too!)

source/blender/editors/space_graph/graph_draw.cc Outdated

						
				@ -1095,0 +1089,4 @@

				        const float samples_per_pixel = 0.75f;

				        const float samples_per_frame = (window_width * samples_per_pixel) / v2d_frame_range;

				        const float evaluation_step = (bezt->vec[1][0] - prevbezt->vec[1][0]) / samples_per_frame;

Nathan Vegdahl commented

2023-08-04 17:27:45 +02:00

I played around with a build, and this computation of the evaluation_step doesn't seem to be right (it gives very inconsistent drawing resolutions, sometimes extremely low, sometimes very high).

I think the issue is that you're effectively double-accounting for the mapping from screen space to fcurve space by using both v2d_frame_range and (bezt->vec[1][0] - prevbezt->vec[1][0]) in the computation. I believe you only need one of them.

If I replace the const float samples_per_frame and const float evaluation_step lines with the following:

const float pixel_width = v2d_frame_range / window_width;
const float evaluation_step = pixel_width / samples_per_pixel;

Then it seems to behave as expected. For example, setting samples_per_pixel to 0.1 then results in a key every ~10 pixels, regardless of zoom.

I played around with a build, and this computation of the `evaluation_step` doesn't seem to be right (it gives very inconsistent drawing resolutions, sometimes extremely low, sometimes very high). I think the issue is that you're effectively double-accounting for the mapping from screen space to fcurve space by using both `v2d_frame_range` and `(bezt->vec[1][0] - prevbezt->vec[1][0])` in the computation. I believe you only need one of them. If I replace the `const float samples_per_frame` and `const float evaluation_step` lines with the following: ```c const float pixel_width = v2d_frame_range / window_width; const float evaluation_step = pixel_width / samples_per_pixel; ``` Then it seems to behave as expected. For example, setting `samples_per_pixel` to `0.1` then results in a key every ~10 pixels, regardless of zoom.

Christoph Lendenfeld commented

2023-08-10 11:29:37 +02:00

I tested it and your code gives me the exact same evaluation_step
I still changed the code to your suggestion because I think it reads a bit better

I tested it and your code gives me the exact same `evaluation_step` I still changed the code to your suggestion because I think it reads a bit better

👍 1

Nathan Vegdahl commented

2023-08-10 11:35:25 +02:00

Huh, that's odd. Wonder why it behaved differently on my system. Oh well. :-)

nathanvegdahl marked this conversation as resolved

Christoph Lendenfeld added 2 commits 2023-08-10 11:30:47 +02:00

Merge branch 'main' into graph_common_curve_drawing 73519729fc

change evaluation step code 23c7281e48