blender
/
blender
135
425
Fork 608
Code Issues 6.2k Pull Requests 780 Projects 19 Wiki Activity

Anim: Change how Only Insert Needed works #115360

Merged
Christoph Lendenfeld merged 11 commits from ChrisLend/blender:refactor_insert_needed_logic into main 2023-11-30 14:45:59 +01:00
Conversation 2 Commits 11 Files Changed 1 +53 -57
1 changed files with 53 additions and 57 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit b4f92e593d - Show all commits

110
source/blender/animrig/intern/keyframing.cc
View File

@ -241,84 +241,80 @@ enum {
* 2. Keyframe to be added on frame where two keyframes are already situated.
nathanvegdahl marked this conversation as resolved Outdated
Nathan Vegdahl commented 2023-11-30 11:43:29 +01:00
Outdated
Review
Copy Link

How did you arrive at this value? I don't mean that you need a super strong justification or anything, mostly just curious.

How did you arrive at this value? I don't mean that you need a super strong justification or anything, mostly just curious.
Christoph Lendenfeld commented 2023-11-30 14:27:59 +01:00
Outdated
Review
Copy Link

honestly just a wild guess. 32 numbers difference seemed sufficient for me
Correct me if you think this should be different.

honestly just a wild guess. 32 numbers difference seemed sufficient for me Correct me if you think this should be different.
Nathan Vegdahl commented 2023-11-30 14:42:23 +01:00
Outdated
Review
Copy Link

No, 32 should be fine. I think elsewhere in the code base it's usually 64, but as far as I can tell that's equally arbitrary, and people have basically just been copy/pasting it.

No, 32 should be fine. I think elsewhere in the code base it's usually 64, but as far as I can tell that's equally arbitrary, and people have basically just been copy/pasting it.
* 3. Keyframe lies at point that intersects the linear line between two keyframes.
*/
static short new_key_needed(FCurve *fcu, float cFrame, float nValue)
static short new_key_needed(FCurve *fcu, float frame, float value)
{
if (fcu == nullptr) {
return KEYNEEDED_JUSTADD;
}
int totCount = fcu->totvert;
if (totCount == 0) {
if (fcu->totvert == 0) {
nathanvegdahl marked this conversation as resolved Outdated
Nathan Vegdahl commented 2023-11-30 11:32:48 +01:00
Outdated
Review
Copy Link

Is there a particular reason to use compare_ff_relative() for a key that already exists, rather than just a straight == comparison? My intuition is that since there's already a key there, the worst-case with a false positive is that the existing key gets slightly adjusted by a minuscule amount, which seems fine. And then we avoid any risk of false negatives. But I could be missing something.

Is there a particular reason to use `compare_ff_relative()` for a key that already exists, rather than just a straight `==` comparison? My intuition is that since there's already a key there, the worst-case with a false positive is that the existing key gets slightly adjusted by a minuscule amount, which seems fine. And then we avoid any risk of false negatives. But I could be missing something.
Christoph Lendenfeld commented 2023-11-30 14:28:52 +01:00
Outdated
Review
Copy Link

Fair point. Changed it to a simple !=

Fair point. Changed it to a simple `!=`
return KEYNEEDED_JUSTADD;
}
bool replace;
const int bezt_index = BKE_fcurve_bezt_binarysearch_index(
fcu->bezt, frame, fcu->totvert, &replace);
if (replace) {
/** In case there is a key at the current frame just let
* the keyframe insertion code do its thing. */
return KEYNEEDED_JUSTADD;
}
/* Loop through checking if any are the same. */
BezTriple *bezt = fcu->bezt;
BezTriple *prev = nullptr;
for (int i = 0; i < totCount; i++) {
float prevPosi = 0.0f, prevVal = 0.0f;
float beztPosi = 0.0f, beztVal = 0.0f;
beztPosi = bezt->vec[1][0];
beztVal = bezt->vec[1][1];
float prevPosi = 0.0f, prevVal = 0.0f;
float beztPosi = 0.0f, beztVal = 0.0f;
if (prev) {
/* There is a keyframe before the one currently being examined. */
prevPosi = prev->vec[1][0];
prevVal = prev->vec[1][1];
beztPosi = bezt->vec[1][0];
beztVal = bezt->vec[1][1];
/* Keyframe to be added at point where there are already two similar points? */
if (IS_EQF(prevPosi, cFrame) && IS_EQF(beztPosi, cFrame) && IS_EQF(beztPosi, prevPosi)) {
if (prev) {
/* There is a keyframe before the one currently being examined. */
prevPosi = prev->vec[1][0];
prevVal = prev->vec[1][1];
/* Keyframe to be added at point where there are already two similar points? */
if (IS_EQF(prevPosi, frame) && IS_EQF(beztPosi, frame) && IS_EQF(beztPosi, prevPosi)) {
return KEYNEEDED_DONTADD;
}
/* Keyframe between prev+current points? */
if ((prevPosi <= frame) && (frame <= beztPosi)) {
/* Is the value of keyframe to be added the same as keyframes on either side? */
if (IS_EQF(prevVal, value) && IS_EQF(beztVal, value) && IS_EQF(prevVal, beztVal)) {
return KEYNEEDED_DONTADD;
}
/* Keyframe between prev+current points? */
if ((prevPosi <= cFrame) && (cFrame <= beztPosi)) {
/* Is the value of keyframe to be added the same as keyframes on either side? */
if (IS_EQF(prevVal, nValue) && IS_EQF(beztVal, nValue) && IS_EQF(prevVal, beztVal)) {
return KEYNEEDED_DONTADD;
}
/* Get real value of curve at that point. */
const float realVal = evaluate_fcurve(fcu, frame);
float realVal;
/* Get real value of curve at that point. */
realVal = evaluate_fcurve(fcu, cFrame);
/* Compare whether it's the same as proposed. */
if (IS_EQF(realVal, nValue)) {
return KEYNEEDED_DONTADD;
}
return KEYNEEDED_JUSTADD;
}
/* New keyframe before prev beztriple? */
if (cFrame < prevPosi) {
/* A new keyframe will be added. However, whether the previous beztriple
* stays around or not depends on whether the values of previous/current
* beztriples and new keyframe are the same.
*/
if (IS_EQF(prevVal, nValue) && IS_EQF(beztVal, nValue) && IS_EQF(prevVal, beztVal)) {
return KEYNEEDED_DELNEXT;
}
return KEYNEEDED_JUSTADD;
/* Compare whether it's the same as proposed. */
if (IS_EQF(realVal, value)) {
return KEYNEEDED_DONTADD;
}
return KEYNEEDED_JUSTADD;
}
else {
/* Just add a keyframe if there's only one keyframe
* and the new one occurs before the existing one does.
/* New keyframe before prev beztriple? */
if (frame < prevPosi) {
/* A new keyframe will be added. However, whether the previous beztriple
* stays around or not depends on whether the values of previous/current
* beztriples and new keyframe are the same.
*/
if ((cFrame < beztPosi) && (totCount == 1)) {
return KEYNEEDED_JUSTADD;
if (IS_EQF(prevVal, value) && IS_EQF(beztVal, value) && IS_EQF(prevVal, beztVal)) {
return KEYNEEDED_DELNEXT;
}
}
/* Continue. Frame to do not yet passed (or other conditions not met) */
if (i < (totCount - 1)) {
prev = bezt;
bezt++;
return KEYNEEDED_JUSTADD;
}
else {
break;
}
else {
/* Just add a keyframe if there's only one keyframe
* and the new one occurs before the existing one does.
*/
nathanvegdahl marked this conversation as resolved Outdated
Nathan Vegdahl commented 2023-11-30 11:45:47 +01:00
Outdated
Review
Copy Link

Since this variable isn't used except inside the if condition, and since new_key_needed() is already named very clearly, I'd say just inline the function call into the if condition.

Since this variable isn't used except inside the if condition, and since `new_key_needed()` is already named very clearly, I'd say just inline the function call into the if condition.
Christoph Lendenfeld commented 2023-11-30 14:26:13 +01:00
Outdated
Review
Copy Link

good point

good point
if ((frame < beztPosi) && (fcu->totvert == 1)) {
return KEYNEEDED_JUSTADD;
}
}
@ -339,7 +335,7 @@ static short new_key_needed(FCurve *fcu, float cFrame, float nValue)
valB = bezt->vec[1][1] + 1.0f;
}
if (IS_EQF(valA, nValue) && IS_EQF(valA, valB)) {
if (IS_EQF(valA, value) && IS_EQF(valA, valB)) {
return KEYNEEDED_DELPREV;
}