- Amsterdam, Netherlands
- https://cessen.com
-
Animator, rigger, and software developer. Currently working at the Blender Institute as a developer on Blender's animation system.
Been using Blender since 1998, and worked on Big Buck Bunny and Sintel (two of Blender's open movie projects).
- Joined on
2003-03-21 -
-
While that's true, this is also accidentally quadratic, because on every removal (BKE_fcurve_delete_key) it has to shift all the keys after the removed key back. If you iterate forward and keep track of the gap as it's created, you can shift keys over the gap as you go for linear runtime.
Oops, I think a change got pushed while I was reviewing, so these last two comments got detached. I'll resubmit them.
This is also accidentally quadratic (actually slightly worse, because it additionally does a binary search to find where to insert each key), and does an allocation and full copy of the f-curve's entire bezt data on every insert.
This description feels non-specific to me. There are a lot of operators that create key frames.
While that's true, this is also accidentally quadratic, because on every removal (BKE_fcurve_delete_key) it has to shift all the keys after the removed key back. If you iterate forward and keep track of the gap as it's created, you can shift keys over the gap as you go for linear runtime.
I'd prefer new constants for this enum, rather than awkwardly having just one that's new and defined arbitrarily high to avoid conflicting with the others. It's not as convenient in other code, because then you have to map between the two sets of constants later. But it feels cleaner to me that way.
If we do keep these options as-are, I think REMOVE_IN_RANGE is a better default. I don't think people usually want to leave behind the old keys in the range being baked.
Found a couple of bugs while testing:
- If you set the start range > end range, Blender crashes.
- If you set the frame step <= 0.0, Blender freezes.
And I have a couple of questions about…
Looks good to me. And works as I would expect in testing.
Landed #111143, which adds an operator for changing the handle selection of selected keys.
Ah, fair point. I basically just copy-pasted this from elsewhere. I agree that just using the index directly makes more sense here. Thanks for the catch!