Page MenuHome

Nondestructive Keyframe Insert and deletion
Needs ReviewPublic

Authored by cris (InsigMathK) on Apr 21 2017, 4:38 PM.

Details

Summary

The motivation for this patch is to allow users to safely insert keyframes in the middle of an fcurve without distorting the curve and changing the original position of the object before the keyframe was inserted.
The use of Castle de jau's algorithm was used to split the bezier curve so that the insertion results in a new bezier curve that is the same shape as before.
This patch also allows for safe deletion of keyframes between keyframes so that the shape is maintained if the keyframe deleted is between two increasing (decreasing) keyframes.

Diff Detail

Repository
rB Blender
Branch
nondestructive_keyframe_insert
Build Status
Buildable 694
Build 694: arc lint + arc unit

Event Timeline

@Jason schleifer (jasonschleifer) @Luciano Muñoz Sessarego (looch)
You guys might be interested in testing this patch sometime ;)

source/blender/editors/animation/keyframes_general.c
128

Why are you manually removing the points one by one (and resizing the array), if we already know that that all points *are* selected? Wouldn't it be better to just do a:

MEM_freeN(fcu->bezt); 
fcu->bezt = NULL; 
fcu->totvert = 0;
i = 0;
changed = true;
source/blender/editors/animation/keyframing.c
399

Potential numerical stability issues with large numbers (e.g. overflow, etc.)

400

Mark any unchanging variables as const

415

See comments on the other function

454
  1. You might as well make the last arg float P[5][2] to make it explicit to both programmer + compiler what the bounds of that array are
  1. Is compute_keyframe() exposed/used outside this function. If not, mark as static. If it used outside this file, the naming is ambiguous.
461

General comment: Follow the style guides on the wiki :)

510

Any reason why this needs to be at the top of the function?

Suggest to move after the 3 defines below. In general, try to limit definition of variables to the top of blocks only (i.e. try to not interleave code + definitions too much; if you do define variables in the middle of code, try to group those definitions together, to make it easier to find them).

521

Why change this?

666

Group these assignments, e.g.

prev->h1  = prev->h2  = HD_ALIGN;
next->h1  = next->h2   = HD_ALIGN:
// ...
697

Why the duplication of code with the previous branch? Just keep all these handle recalculation bits outside of the if/else blocks.

If however there is some difference between the two cases, please make sure to clearly label them, so that
it is easy to tell whether something was intentional (i.e. a critical difference) vs an accidental typo.

source/blender/editors/include/ED_keyframes_edit.h
286

Is this used anywhere else?

Joshua Leung (aligorith) requested changes to this revision.May 14 2017, 6:38 PM
This revision now requires changes to proceed.May 14 2017, 6:38 PM
cris (InsigMathK) edited edge metadata.

I made changes suggested by Joshua Leung.

  1. For deleting keyframes I added the code that he suggested:

MEM_freeN(fcu->bezt);
fcu->bezt = NULL;
fcu->totvert = 0;
i = 0;
changed = true;

  1. I changed some of the documentation for get_parameterization_on_x and get_parameterization_on_y.
  1. I indicated that compute_keyframe() is static so that other developers will know that it is only used within that file.
  1. Made some style changes.
  1. In insert_vert_fcurve I removed the else clause and streamlined the code.

Some observations after just looking through the patch.

source/blender/editors/animation/keyframing.c
427

Why are there two functions that are basically Copy&Paste with some indices changed?

480

Somehow this part with different t for different coordinates feels iffy - a bezier curve is a 2D curve parameterized by common t after all.

517

Replace vs insert is determined within insert_bezt_fcurve. This is logic duplication and potential error source. Also, in this function it is already possible to detect replace vs insert by comparing oldTot and fcu->totvert - that's what oldTot is for.

609

Don't change handle types arbitrarily. As I see it, the usefulness of this patch is to be seen in correcting manually set handles when inserting a key between them. It should never force insertion of a manual key instead of auto, unless possibly when inserting between two manual handles. If at least one of the adjoining handles is auto, the inserted key should remain auto.

The reason for this is that non-auto keyframes are a big liability: once they exist, it is impossible to fully control the animation without the use of the Graph editor - adjusting the key via dopesheet or insert keyframe will break smoothness of the curve as the handles won't accomodate the change.

what ever happened to this patch?

@Luciano Muñoz Sessarego (looch), it got slipped through the cracks. Lets blow some dust away as this sounds an interesting feature.

Check out my alternative to this patch (although it has not been updated for a long time either): D3172