Anim: Restrict range of fcurve_to_keylist #114854

Christoph Lendenfeld · 2023-11-14T17:24:16+01:00

2023-11-14 17:24:16 +01:00

Issue

Having a lot of keys in your scene can dramatically slow down the Dope Sheet.
That is because everytime the Dope Sheet is redrawn, the Keylists have to be recomputed.
In the case of the summary channel, that means going through all keyframes of all the FCurves and adding them to the keylist.
That eats up 95% of the time it takes to draw a frame.

This PR

It's not a perfect solution, rather it solves the performance issue for the case when you are not displaying all keys.
Instead of going through all the keys, only add the keys visible in the view to the keylist.
This speeds up the Dope Sheet significantly depending on the zoom level.

This also improves the responsiveness when selecting and transforming keyframes.

Performance changes

The function measured is ED_channel_list_flush which is responsible for building the keylists and drawing them.
The test setup contains 62 bones with all 10 channels keyed (location, rot quaternion, scale) on 6000 frames.
So 3.720.000 keys. The heavier the dataset the bigger the performance impact.

The data was recorded with only the Dope Sheet open, and 3 channels visible

Summary
Object
Action

The more channels are visible, the greater the performance gain. This can be seen in the video.

visible range	before	after
200f	250ms	10ms
400f	250ms	18ms
3000f	250ms	130ms
6000f	250ms	250ms

Future improvements

The keylists really don't need to be rebuilt every frame. It should be possible to only rebuild them when the data changes. That should speed up dragging the viewport regardless of the zoom level.

# Issue Having a lot of keys in your scene can dramatically slow down the Dope Sheet. That is because everytime the Dope Sheet is redrawn, the Keylists have to be recomputed. In the case of the summary channel, that means going through all keyframes of all the `FCurves` and adding them to the keylist. That eats up 95% of the time it takes to draw a frame. # This PR It's not a perfect solution, rather it solves the performance issue for the case when you are not displaying all keys. Instead of going through all the keys, only add the keys visible in the view to the keylist. This speeds up the Dope Sheet significantly depending on the zoom level. This also improves the responsiveness when selecting and transforming keyframes. # Performance changes The function measured is `ED_channel_list_flush` which is responsible for building the keylists and drawing them. The test setup contains 62 bones with all 10 channels keyed (location, rot quaternion, scale) on 6000 frames. So 3.720.000 keys. The heavier the dataset the bigger the performance impact. The data was recorded with only the Dope Sheet open, and 3 channels visible * Summary * Object * Action The more channels are visible, the greater the performance gain. This can be seen in the video. | visible range | before | after | | - | - | - | | 200f | 250ms | 10ms | | 400f | 250ms | 18ms | | 3000f | 250ms | 130ms | | 6000f | 250ms | 250ms | <video src="/attachments/d2e8ec43-e35f-40c6-807e-ebb3981379c1" title="Dope Sheet performance improvement" controls></video> # Future improvements The keylists really don't need to be rebuilt every frame. It should be possible to only rebuild them when the data changes. That should speed up dragging the viewport regardless of the zoom level.

faster_dopesheet.mp4

3.8 MiB

❤️ 3 🎉 1

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2023-11-14 17:24:16 +01:00

Christoph Lendenfeld added 1 commit 2023-11-14 17:24:31 +01:00

57d93d6c5a restrict range of fcurve to keylist

Brad Clark commented

2023-11-14 17:52:40 +01:00

Love it!!

Christoph Lendenfeld added 2 commits 2023-11-16 09:48:00 +01:00

63e17386da expand index range by 1 in each direction

buildbot/vexp-code-patch-coordinator Build done. Details

7b69c28511 also restrict range on scene and action group

Christoph Lendenfeld changed title from ~~WIP: Anim: Restrict range of fcurve_to_keylist~~ to Anim: Restrict range of fcurve_to_keylist

2023-11-16 11:10:20 +01:00

Christoph Lendenfeld commented

2023-11-16 11:12:59 +01:00

@blender-bot build

Christoph Lendenfeld added this to the 4.1 milestone 2023-11-16 11:22:37 +01:00

Christoph Lendenfeld requested review from Nathan Vegdahl 2023-11-16 13:46:56 +01:00

Nathan Vegdahl reviewed 2023-11-23 12:27:12 +01:00

Nathan Vegdahl left a comment

The code looks good to me. Still need to test it.

I just have one comment that's basically me hurting inside over C/C++ array semantics. So feel free to ignore if you can't think of anything good to do with it.

The code looks good to me. Still need to test it. I just have one comment that's basically me hurting inside over C/C++ array semantics. So feel free to ignore if you can't think of anything good to do with it.

source/blender/editors/animation/keyframes_keylist.cc Outdated

						
				@ -932,0 +931,4 @@

				void summary_to_keylist(bAnimContext *ac,

				                        AnimKeylist *keylist,

				                        const int saction_flag,

				                        float range[2])

Nathan Vegdahl commented

2023-11-23 11:53:55 +01:00

Using a C-style array to make the range argument optional feels somehow dirty to me here because we're always passing a blender::float2, which is a nice POD type. Having said that, I'm not thrilled about the alternatives I can think of either:

Using std::optional<float2>.
Using float2 *.
Splitting these functions into two: one with and one without that parameter.

Alternative 1 feels very natural to me given the other language I usually work with... but somehow feels awkward and verbose in C++. Alternative 2 semantically feels better to me, but would make actual usage more awkward than what you have right now. Alternative 3 feels like overkill in this case. So... dunno.

Using a C-style array to make the `range` argument optional feels somehow dirty to me here because we're always passing a `blender::float2`, which is a nice POD type. Having said that, I'm not thrilled about the alternatives I can think of either: 1. Using `std::optional<float2>`. 2. Using `float2 *`. 3. Splitting these functions into two: one with and one without that parameter. Alternative 1 feels very natural to me given the other language I usually work with... but somehow feels awkward and verbose in C++. Alternative 2 semantically feels better to me, but would make actual usage more awkward than what you have right now. Alternative 3 feels like overkill in this case. So... dunno.

Nathan Vegdahl commented

2023-11-23 12:44:02 +01:00

When trying to grab keyframes in either the action editor or dopesheet editor, Blender is aborting with the following assert:

BLI_assert failed: /home/guest/Projects/blender/blender/source/blender/editors/transform/transform_convert.cc:1097, create_trans_data(), at '!t->data_container || !t->data_container->data || (t->data_container->data[0].val != t->data_container->data[0].loc)'
  td->val is only for 1D values

Here's a blend file just in case: dopesheet_abort.blend

Just hit g to try to move the keys.

When trying to grab keyframes in either the action editor or dopesheet editor, Blender is aborting with the following assert: ```sh BLI_assert failed: /home/guest/Projects/blender/blender/source/blender/editors/transform/transform_convert.cc:1097, create_trans_data(), at '!t->data_container || !t->data_container->data || (t->data_container->data[0].val != t->data_container->data[0].loc)' td->val is only for 1D values ``` Here's a blend file just in case: [dopesheet_abort.blend](/attachments/c4644c1f-e236-4373-8ac2-bdddab4c3676) Just hit `g` to try to move the keys.

dopesheet_abort.blend

1.0 MiB

Christoph Lendenfeld added 2 commits 2023-11-23 13:06:46 +01:00

d8d37f127b Merge branch 'main' into keylist_limited_range

6bb204c8ac Merge branch 'main' into keylist_limited_range

Christoph Lendenfeld commented

2023-11-23 13:07:40 +01:00

@nathanvegdahl I cannot reproduce that. I merged main just in case this was a different issue. Can you try this again?

Nathan Vegdahl commented

2023-11-23 14:19:57 +01:00

Updated, and that indeed resolved the issue.

👍 1

Nathan Vegdahl commented

2023-11-23 14:53:22 +01:00

The assert was making me nervous, so I looked into it. It was caused by 9dcf73c715 which was committed a few days before your PR and added that assert. Turns out that assert itself was erroneous, and was subsequently removed in e581dde40f.

So I think we're good land!

The assert was making me nervous, so I looked into it. It was caused by 9dcf73c7157f342cb16092b4e47864777a59a4f1 which was committed a few days before your PR and added that assert. Turns out that assert itself was erroneous, and was subsequently removed in e581dde40fad689db7082309c6072ab3806b4f55. So I think we're good land!

Nathan Vegdahl approved these changes 2023-11-23 14:53:51 +01:00

Nathan Vegdahl left a comment

LGTM.

Christoph Lendenfeld added 1 commit 2023-11-23 15:29:20 +01:00

e18eaa52df always pass a float2 range

Christoph Lendenfeld added 1 commit 2023-11-23 15:34:08 +01:00

9f4c34ebd0 fix comment

Christoph Lendenfeld commented

2023-11-23 15:36:13 +01:00

@nathanvegdahl thanks for the review but I need your eyes on this once more.
You mentioned that passing the float2 into a C style float array that can be a nullptr feels dirty and I agree.
When writing this I just wasn't sure what else to do.

So now I changed it to be not optional. Any functions that cannot meaningfully provide a range just pass in {-FLT_MAX, FLT_MAX}
That way it is really obvious what's going on, and which functions could be optimized later on. (motion paths for example)

@nathanvegdahl thanks for the review but I need your eyes on this once more. You mentioned that passing the float2 into a C style float array that can be a `nullptr` feels dirty and I agree. When writing this I just wasn't sure what else to do. So now I changed it to be not optional. Any functions that cannot meaningfully provide a range just pass in `{-FLT_MAX, FLT_MAX}` That way it is really obvious what's going on, and which functions could be optimized later on. (motion paths for example)

Christoph Lendenfeld requested review from Nathan Vegdahl 2023-11-23 15:36:20 +01:00

Christoph Lendenfeld added 1 commit 2023-11-23 15:37:37 +01:00

4e44bdda84 remove limited range from motionpath code

too risky to do in this PR

Nathan Vegdahl commented

2023-11-23 16:15:50 +01:00

Oh yeah, this feels much better!

Is there a reason you went with FLT_MAX instead of INFINITY? It doesn't make a practical difference either way, so I'm happy as-is. Mainly just curious, because I'm used to using infinity for this kind of thing.

Oh yeah, this feels much better! Is there a reason you went with `FLT_MAX` instead of `INFINITY`? It doesn't make a practical difference either way, so I'm happy as-is. Mainly just curious, because I'm used to using infinity for this kind of thing.

Christoph Lendenfeld commented

2023-11-23 16:22:19 +01:00

Is there a reason you went with FLT_MAX instead of INFINITY? It doesn't make a practical difference either way, so I'm happy as-is. Mainly just curious, because I'm used to using infinity for this kind of thing.

It's just that I used FLT_MAX so far.
Also not sure what happens when I plug INFINITY into a BKE_fcurve_bezt_binarysearch_index call

> Is there a reason you went with `FLT_MAX` instead of `INFINITY`? It doesn't make a practical difference either way, so I'm happy as-is. Mainly just curious, because I'm used to using infinity for this kind of thing. It's just that I used `FLT_MAX` so far. Also not sure what happens when I plug `INFINITY` into a `BKE_fcurve_bezt_binarysearch_index` call

Nathan Vegdahl commented

2023-11-23 16:26:52 +01:00

BKE_fcurve_bezt_binarysearch_index is all comparison based, so it should work fine. (And it has special cases to optimize for before/after the first/last frames as well, to avoid the whole binary search, which makes your approach here efficient.)

Anyway, FLT_MAX is totally fine!

`BKE_fcurve_bezt_binarysearch_index` is all comparison based, so it should work fine. (And it has special cases to optimize for before/after the first/last frames as well, to avoid the whole binary search, which makes your approach here efficient.) Anyway, `FLT_MAX` is totally fine!

Nathan Vegdahl approved these changes 2023-11-23 16:27:20 +01:00

Nathan Vegdahl left a comment

Approved!

Christoph Lendenfeld merged commit f06fd85d97 into main

2023-11-23 16:37:44 +01:00

Christoph Lendenfeld referenced this issue from a commit

2023-11-23 16:37:44 +01:00

Anim: Restrict range of fcurve_to_keylist

Christoph Lendenfeld deleted branch keylist_limited_range

2023-11-23 16:37:46 +01:00

Hans Goudey referenced this issue from a commit

2023-11-27 18:01:10 +01:00