Animation: Graph Editor - Don't draw curve points if they are too close to each other #110788

Christoph Lendenfeld · 2023-08-04T10:58:17+02:00

2023-08-04 10:58:17 +02:00

When viewing a curve with a lot of keys totally zoomed out,
the keys might be closer to each other than a pixel.
The code would still draw a line between them,
costing time in the draw code.

This PR skips any keys that are too close to each other in screen space.

Performance

The setup: 62 bones with 6000f of animation keyed on every frame. No keys displayed, just the curves.

-	before	after
zoomed in	11μs	17μs
zoomed out	55μs	30μs

So a small performance penalty when zoomed in in exchange for a big gain when zoomed out.

Visuals

There is a small change in visuals when zoomed out a lot. Because this change averages keys that are too close together, it results in a small loss of high frequency detail. If you look closely in the video you can see the detail appearing when the view is zoomed in.

before	after

The logic

If prevbezt and bezt are too close it will not draw, but extend a bounding box with prevbezt and bezt including handles if needed.
Eventually the keys will be far enough apart to draw. At this point, draw the center of the bounding box and reset it.
Now because there could be the case where bezt in the current loop is super far out (e.g 5 keys with 1f spacing and the 6th key is at f1000) we need to recalculate the bounding box again. In case the keys are far away, just draw normally. If it is close still, the same process repeats.

When viewing a curve with a lot of keys totally zoomed out, the keys might be closer to each other than a pixel. The code would still draw a line between them, costing time in the draw code. This PR skips any keys that are too close to each other in screen space. ## Performance The setup: 62 bones with 6000f of animation keyed on every frame. No keys displayed, just the curves. | - | before | after | | - | - | - | | zoomed in | 11μs | 17μs | | zoomed out | 55μs | 30μs | So a small performance penalty when zoomed in in exchange for a big gain when zoomed out. ## Visuals There is a small change in visuals when zoomed out a lot. Because this change averages keys that are too close together, it results in a small loss of high frequency detail. If you look closely in the video you can see the detail appearing when the view is zoomed in. | before | after | | - | - | | ![image](/attachments/5d457bc0-a29f-494e-bf92-63de8b1c5661) |![image](/attachments/508b481b-b9b1-4498-811a-8329cf2dd3e6) | <video src="/attachments/778c43e9-da08-49d2-a209-dceb3f54f2c1" controls></video> ## The logic If `prevbezt` and `bezt` are too close it will not draw, but extend a bounding box with `prevbezt` and `bezt` including handles if needed. Eventually the keys will be far enough apart to draw. At this point, draw the center of the bounding box and reset it. Now because there could be the case where `bezt` in the current loop is super far out (e.g 5 keys with 1f spacing and the 6th key is at f1000) we need to recalculate the bounding box again. In case the keys are far away, just draw normally. If it is close still, the same process repeats.

image.png

163 KiB

image.png

169 KiB

image.png

172 KiB

curve_drawing_lod 2023-08-11 12-37-21.mp4

4.5 MiB

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2023-08-04 10:58:17 +02:00

Christoph Lendenfeld added 3 commits 2023-08-04 10:58:28 +02:00

9bfc06fba4 skip keys based on pixel distance

dd7ca2d669 remove time print

42bbb733f1 Merge branch 'main' into graph_reduced_key_drawing

Christoph Lendenfeld added 1 commit 2023-08-04 11:02:20 +02:00

a942f6f0f5 undo changes to calculate_bezt_draw_resolution

Christoph Lendenfeld added 1 commit 2023-08-04 11:03:29 +02:00

5b6677af8f rename to points_per_unit

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

sanek122005 commented

2023-08-07 11:53:39 +02:00

First-time contributor

I think that such an optimization should be carried out and for dopesheet. it seems to me that in the simplest test by baking keys up to 4000 frames, it will help

I think that such an optimization should be carried out and for dopesheet. it seems to me that in the simplest test by baking keys up to 4000 frames, it will help <video src="/attachments/38011090-c43e-46c9-a25a-103e76f2ec8c" title="2023-08-07 12-51-01.mp4" controls></video>

2023-08-07 12-51-01.mp4

3.3 MiB

Christoph Lendenfeld commented

2023-08-10 11:01:08 +02:00

@sanek122005 yes the dope sheet needs to be optimized, but it's not the drawing that is the issue. I ran a profiler on it and the bottleneck (95% the time spent) is the building of keylists. That is the datastructure that is used to draw and select keys. That is a bit harder to optimize though.

Christoph Lendenfeld added 1 commit 2023-08-11 11:08:40 +02:00

25591a0d67 Merge branch 'main' into graph_reduced_key_drawing

Christoph Lendenfeld added 1 commit 2023-08-11 12:32:08 +02:00

8a7bcbd364 fix wrong calculation of pixel distance

Christoph Lendenfeld added 2 commits 2023-08-11 12:35:34 +02:00

d17df3f75d always add last point

6964529fab remove timing code

Christoph Lendenfeld added 1 commit 2023-08-11 12:54:00 +02:00

684a563224 add comments

Christoph Lendenfeld changed title from ~~WIP: Animation: Graph Editor - Don't draw keys if they are too close to each other~~ to Animation: Graph Editor - Don't draw keys if they are too close to each other

2023-08-11 12:55:44 +02:00

Christoph Lendenfeld requested review from Sybren A. Stüvel 2023-08-11 12:55:59 +02:00

Christoph Lendenfeld requested review from Nathan Vegdahl 2023-08-11 12:56:07 +02:00

Christoph Lendenfeld referenced this pull request

2023-08-11 14:44:55 +02:00

Refactor: resolution_scale in graph_draw.cc #111037

Christoph Lendenfeld referenced this pull request

2023-08-11 15:01:38 +02:00

Refactor: resolution_scale in graph_draw.cc #111037

Nathan Vegdahl requested changes 2023-08-14 15:10:23 +02:00

Nathan Vegdahl left a comment

The idea here is great, but I think to make it reliable it needs to work a little differently. Details in my comment below.

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

													
				@ -1023,0 +1024,4 @@

				static float calculate_pixel_distance(BezTriple *a, BezTriple *b, blender::float2 pixels_per_unit)

				{

				  return ((b->vec[1][0] - a->vec[1][0]) * pixels_per_unit[0]) +

Nathan Vegdahl commented

2023-08-14 15:08:01 +02:00

I think this is another case where we're going to want to take into account the handles. Because although the keys may be within a pixel, their handles can potentially extend high up or high down, making the curve between those two keys something we would want to make sure to draw.

Or in other words, the units of data we should be working with here aren't keys, but the curve segments formed by two keys:

Key A -> Key A right handle -> Key B left handle -> Key B

I think the over-all algorithm we want here is to maintain a bounding box that gets grown with each segment, and then just before the bounding box exceeds the size threshold in either dimension, we draw a curve formed from the first and last keys that went into that bounding box. And then we start again, beginning with the segment that was going to make the bounding box exceed the threshold.

I think this is another case where we're going to want to take into account the handles. Because although the keys may be within a pixel, their handles can potentially extend high up or high down, making the curve between those two keys something we would want to make sure to draw. Or in other words, the units of data we should be working with here aren't keys, but the curve segments formed by two keys: ``` Key A -> Key A right handle -> Key B left handle -> Key B ``` I think the over-all algorithm we want here is to maintain a bounding box that gets grown with each segment, and then *just before* the bounding box exceeds the size threshold in either dimension, we draw a curve formed from the first and last keys that went into that bounding box. And then we start again, beginning with the segment that was going to make the bounding box exceed the threshold.

nathanvegdahl marked this conversation as resolved

Christoph Lendenfeld added 1 commit 2023-08-17 10:51:19 +02:00

77f9f9067a Merge branch 'main' into graph_reduced_key_drawing

Christoph Lendenfeld added 1 commit 2023-08-17 11:51:25 +02:00

c10db7fa1d include handles in key distance calculations

Christoph Lendenfeld commented

2023-08-17 11:53:34 +02:00

@nathanvegdahl I changed the code to include the handles of bezier keys
I need to rerun the performance test though since this added a few calculations that need to run on every key.

@nathanvegdahl I changed the code to include the handles of bezier keys I need to rerun the performance test though since this added a few calculations that need to run on every key.

👍 1

Christoph Lendenfeld requested review from Nathan Vegdahl 2023-08-17 11:53:40 +02:00

Christoph Lendenfeld added 1 commit 2023-08-17 14:45:41 +02:00

4fb21aa6eb Merge branch 'main' into graph_reduced_key_drawing

Christoph Lendenfeld commented

2023-08-17 15:01:48 +02:00

good news
the change only cost about 2μs

good news the change only cost about 2μs

Nathan Vegdahl reviewed 2023-08-21 15:53:23 +02:00

source/blender/editors/space_graph/graph_draw.cc

													
				@ -1065,0 +1089,4 @@

				      first_key->vec[1][0], first_key->vec[1][1], first_key->vec[1][0], first_key->vec[1][1]};

				  /* Used when skipping keys. */

				  bool has_skipped_keys = false;

				  const float min_pixel_distance = 3.0f;

Nathan Vegdahl commented

2023-08-21 15:53:23 +02:00

What is the rationale for this constant? Intuitively I would expect it to be something like 1.0 or 1.5, to only merge sub-pixel details.

nathanvegdahl marked this conversation as resolved

Nathan Vegdahl commented

2023-08-21 15:54:27 +02:00

This basically looks ready to land to me. Just curious about that one constant.

Christoph Lendenfeld commented

2023-08-24 10:57:27 +02:00

This basically looks ready to land to me. Just curious about that one constant.

I found this to be a reasonable value during testing. It's a tradeoff between accuracy and performance, but if you look at the video and compare the pictures there is almost no difference at a value of 3

In a future patch I plan to move a few constants to user preferences, this is one of them. It gives the user control over performance/fidelity

> This basically looks ready to land to me. Just curious about that one constant. I found this to be a reasonable value during testing. It's a tradeoff between accuracy and performance, but if you look at the video and compare the pictures there is almost no difference at a value of 3 In a future patch I plan to move a few constants to user preferences, this is one of them. It gives the user control over performance/fidelity

👍 1

Nathan Vegdahl approved these changes 2023-08-24 13:40:51 +02:00

Nathan Vegdahl left a comment

Looks good to me!

Christoph Lendenfeld changed title from ~~Animation: Graph Editor - Don't draw keys if they are too close to each other~~ to Animation: Graph Editor - Don't draw curve points if they are too close to each other

2023-08-25 10:13:13 +02:00

Christoph Lendenfeld commented

2023-08-25 10:13:45 +02:00

renamed the PR because I realized it is a bit confusing
it's not about keyframes but curve points

renamed the PR because I realized it is a bit confusing it's not about keyframes but curve points

👍 1

Sybren A. Stüvel approved these changes 2023-09-05 14:01:18 +02:00

Sybren A. Stüvel left a comment

LGTM, just tiny remarks that can be handled when landing.

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

													
				@ -1022,6 +1022,28 @@ static blender::float2 calculate_pixels_per_unit(View2D *v2d)

				  return pixels_per_unit;

				}

				static float calculate_pixel_distance(rctf &bounds, blender::float2 pixels_per_unit)

Sybren A. Stüvel commented

2023-09-05 13:46:58 +02:00

The parameters aren't modified, so can be const.

The parameters aren't modified, so can be `const`.

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

													
				@ -1025,0 +1028,4 @@

				         BLI_rctf_size_y(&bounds) * pixels_per_unit[1];

				}

				static void expand_key_bounds(BezTriple *left_key, BezTriple *right_key, rctf &bounds)