Details

Reviewers

Sybren A. Stüvel (sybren)
Alexander Gavrilov (angavrilov)

Maniphest Tasks

T78995: Poor performance in Timeline, Dope Sheet, etc when at the top of the editor.

Commits

rB6f50969406a7: Cleanup: Hide implementation details for ED_keyframe_keylist.

Summary

For T78995 we want to change the data structure of keylists to
improve performance. (Probably a Vector with bin-search capabilities).

This patch hides the internal structure of the keylists behind AnimKeylist
structure. This allows us to change the internals without 'breaking' where it is
being used.

The change adds functions to create, free, find and walk over the
keylist.

Diff Detail

Repository: rB Blender

Event Timeline

Jeroen Bakker (jbakker) created this revision.Jul 20 2021, 2:51 PM

Harbormaster completed remote builds in B15917: Diff 39735.Jul 20 2021, 2:51 PM

Jeroen Bakker (jbakker) requested review of this revision.Jul 20 2021, 2:51 PM

Send in for review as this the change effects areas I am not familiar with.

Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Jul 20 2021, 2:57 PM

Jeroen Bakker (jbakker) added inline comments.Jul 20 2021, 3:03 PM

source/blender/editors/animation/keyframes_keylist.c
91	NOTE: Implementation was kept as this is how the original code worked. Not sure as it could be intentional that the current code was doing it this way.
511	Revert change.

Revert unwanted change in comment.

Harbormaster completed remote builds in B15919: Diff 39737.Jul 20 2021, 3:25 PM

Sybren A. Stüvel (sybren) requested changes to this revision.Jul 22 2021, 12:47 PM

Sybren A. Stüvel (sybren) added inline comments.

source/blender/editors/animation/keyframes_draw.c
231	Why isn't this using `keys`, but calling `ED_keylist_listbase()` again?
source/blender/editors/animation/keyframes_keylist.c
91	I think it's a good choice to keep the implementation the same for this patch. And yes, in the future it would definitely be clearer to use `ED_keylist_find_next(keys, min_fra)` and only check the boundary of `max_fra`.
92	Maybe `ED_keylist_find_first_between` would be a better name, since this function does not return all frames between min/max.
103	I'd change the name to `ED_keylist_is_empty()`, to make it extra clear that it's a query and not an imperative.
110	This cast is weird. It's the `keylist->keys` itself that has the `first` and `last` pointers, so that is actually the `ListBase` here. The `first` pointer points to the first element. If that is a `ListBase` as well, and this fact is being used here, I feel that this is making far too many assumptions about the internal structure of the wrapped `DLRBT_Tree` to go without heavy and industrial-strength documentation.
source/blender/editors/include/ED_keyframes_keylist.h
148	Add a comment that explains what the `ListBase` contains. I tend to use `ListBase /ItemType/ *` notation for this.
source/blender/editors/space_nla/nla_draw.c
131–132	I know this already existed, but it's a really ugly coupling between the NLA drawing code and the internals of the new keylist structure. Wouldn't it be better to have some `ED_keylist_frame_range(keylist, r_frame_start, r_frame_end)` function?

This revision now requires changes to proceed.Jul 22 2021, 12:47 PM

Jeroen Bakker (jbakker) added inline comments.Jul 22 2021, 5:34 PM

source/blender/editors/animation/keyframes_keylist.c
92	First isn’t correct as the implementation uses a bin search . Perhaps column?

Reuse local data. keys already contained the needed data.
Renamed ED_keylist_find_between -> ED_keylist_find_any_between.
Renamed ED_keylist_empty -> ED_keylist_is_empty.
Removed unclear cast in ED_keylist_listbase.
Introduced Range2f/ED_keylist_frame_range.
Clarified result type of ED_keylist_listbase.

Harbormaster completed remote builds in B15978: Diff 39851.Jul 23 2021, 8:12 AM

Moved code variable declaration closer to where it is used.

Harbormaster completed remote builds in B15979: Diff 39852.Jul 23 2021, 8:16 AM

Jeroen Bakker (jbakker) added inline comments.Jul 23 2021, 8:20 AM

source/blender/editors/space_nla/nla_draw.c
131–132	I considered `Range2f frame_range = ED_keylist_frame_range(keylist)` as it uses the same stackspace and is much nicer. But it would hide that the function couldn't find a valid result. In the case above we don't check as we know that we always have a valid result (keylist isn't empty).