Page MenuHome
Differential D6379 Diff 21839 source/blender/blenkernel/intern/fcurve.c

Changeset View

Standalone View

View Options

source/blender/blenkernel/intern/fcurve.c

Show First 20 Lines • Show All 968 Lines • ▼ Show 20 Linesvoid fcurve_store_samples(FCurve *fcu, void *data, int start, int end, FcuSampleFunc sample_cb)
} }
/* store the samples */ /* store the samples */
fcu->bezt = NULL; fcu->bezt = NULL;
fcu->fpt = new_fpt; fcu->fpt = new_fpt;
fcu->totvert = end - start + 1; fcu->totvert = end - start + 1;
} }
/* Convert baked/sampled fcurves into bezt/regular fcurves. */
void fcurve_samples_to_keyframes(FCurve *fcu, int start, int end)
sybrenUnsubmitted
Done
Inline Actions

start and end can be const.

sybren: `start` and `end` can be `const`.
zeddbAuthorUnsubmitted
Done
Inline Actions

I can't as I increment start in the code.
I think it would be wasteful do to a const as the int is not a reference and I would need to delare and alternate start variable if I declare it as const.

zeddb: I can't as I increment start in the code. I think it would be wasteful do to a const as the int…
{
/* sanity checks */
sybrenUnsubmitted
Done
Inline Actions

Comments should be full sentences, so start with capital and use punctuation. That includes copy-pasted comments.

sybren: Comments should be full sentences, so start with capital and use punctuation. That includes…
/* TODO: make these tests report errors using reports not CLOG's */
sybrenUnsubmitted
Done
Inline Actions

TODOs should have the name of the person who knows more about this.

sybren: TODOs should have the name of the person who knows more about this.
if (fcu == NULL) {
CLOG_ERROR(&LOG, "No F-Curve with F-Curve Modifiers to Un-Bake");
return;
}
if (start > end) {
CLOG_ERROR(&LOG, "Error: Frame range to unbake F-Curve is inappropriate");
return;
}
if (fcu->fpt == NULL) {
/* No data to unbake */
CLOG_ERROR(&LOG, "Error: Curve containts no baked keyframes");
return;
}
/* free any existing sample/keyframe data on curve */
if (fcu->bezt) {
MEM_freeN(fcu->bezt);
}
BezTriple *bezt;
FPoint *fpt = fcu->fpt;
int tot_kf = end - start;
int tot_sp = fcu->totvert;
sybrenUnsubmitted
Done
Inline Actions

tot_kf and tot_sp can be declared const.

sybren: `tot_kf` and `tot_sp` can be declared `const`.
zeddbAuthorUnsubmitted
Done
Inline Actions

They can't be declared as const as they are decremented in the code.

zeddb: They can't be declared as const as they are decremented in the code.
sybrenUnsubmitted
Done
Inline Actions

Try to avoid overly abbreviated names.

sybren: Try to avoid overly abbreviated names.
bezt = fcu->bezt = MEM_callocN(sizeof(*fcu->bezt) * (size_t)tot_kf, __func__);
fcu->totvert = tot_kf;
/* Get first sample point to 'copy' as keyframe. */
for (; tot_sp && (fpt->vec[0] < (float)start); fpt++, tot_sp--) {
sybrenUnsubmitted
Done
Inline Actions

Why the (float) cast? I don't think that's necessary here.

sybren: Why the `(float)` cast? I don't think that's necessary here.
/* pass */
}
/* Add leading dummy flat points if needed. */
sybrenUnsubmitted
Done
Inline Actions

The comment below says that the points are linear, which means that they won't have handles. Since that is the case, what does "flat points" mean? What are "dummy" points, and why are they needed at all?

sybren: The comment below says that the points are linear, which means that they won't have handles.
zeddbAuthorUnsubmitted
Done
Inline Actions

This is so that points are added if there are no data before the start frame.

So if for example the bakes keys start at frame 10 but we start at frame 5.
Then keys are inserted from 5-10 with the "height" of the key on frame 10.
This will then create a "flat" line in this range.

I'm guessing this is so that there is always data on the unbake frame range.
Not really sure why either, this is simply copy pasted, so I guess there was a good reason as some point at least.

zeddb: This is so that points are added if there are no data before the start frame. So if for…
for (; tot_kf && (fpt->vec[0] > (float)start); start++, bezt++, tot_kf--) {
sybrenUnsubmitted
Done
Inline Actions

This skips the fpt->vec[0] == start case, which is also skipped by the above for-loop.

sybren: This skips the `fpt->vec[0] == start` case, which is also skipped by the above for-loop.
zeddbAuthorUnsubmitted
Done
Inline Actions

Yes?
If we are at the correct point already then the functions should do nothing in those two functions.
So this is correct.

zeddb: Yes? If we are at the correct point already then the functions should do nothing in those two…
/* Linear interpolation, of course. */
bezt->f1 = bezt->f2 = bezt->f3 = SELECT;
bezt->ipo = BEZT_IPO_LIN;
bezt->h1 = bezt->h2 = HD_AUTO_ANIM;
bezt->vec[1][0] = (float)start;
bezt->vec[1][1] = fpt->vec[1];
}
/* Copy actual sample points. */
for (; tot_kf && tot_sp; start++, bezt++, tot_kf--, fpt++, tot_sp--) {
/* Linear interpolation, of course. */
bezt->f1 = bezt->f2 = bezt->f3 = SELECT;
bezt->ipo = BEZT_IPO_LIN;
bezt->h1 = bezt->h2 = HD_AUTO_ANIM;
copy_v2_v2(bezt->vec[1], fpt->vec);
}
/* Add trailing dummy flat points if needed. */
sybrenUnsubmitted
Done
Inline Actions

'leading' and 'heading' mean the same thing. Maybe 'leading' and 'trailing'? I know it's just a copy-paste, but it's an excellent opportunity to make things actually correct.

sybren: 'leading' and 'heading' mean the same thing. Maybe 'leading' and 'trailing'? I know it's just a…
for (fpt--; tot_kf; start++, bezt++, tot_kf--) {
/* Linear interpolation, of course. */
bezt->f1 = bezt->f2 = bezt->f3 = SELECT;
bezt->ipo = BEZT_IPO_LIN;
bezt->h1 = bezt->h2 = HD_AUTO_ANIM;
sybrenUnsubmitted
Done
Inline Actions

This exact same code is duplicated twice. Better to turn it into a small function and call it three times.

sybren: This exact same code is duplicated twice. Better to turn it into a small function and call it…
bezt->vec[1][0] = (float)start;
bezt->vec[1][1] = fpt->vec[1];
}
MEM_SAFE_FREE(fcu->fpt);
/* Not strictly needed since we use linear interpolation, but better be consistent here. */
calchandles_fcurve(fcu);
}
/* ***************************** F-Curve Sanity ********************************* */ /* ***************************** F-Curve Sanity ********************************* */
/* The functions here are used in various parts of Blender, usually after some editing /* The functions here are used in various parts of Blender, usually after some editing
* of keyframe data has occurred. They ensure that keyframe data is properly ordered and * of keyframe data has occurred. They ensure that keyframe data is properly ordered and
* that the handles are correctly * that the handles are correctly
*/ */
/* Checks if the F-Curve has a Cycles modifier, and returns the type of the cycle behavior. */ /* Checks if the F-Curve has a Cycles modifier, and returns the type of the cycle behavior. */
eFCU_Cycle_Type BKE_fcurve_get_cycle_type(FCurve *fcu) eFCU_Cycle_Type BKE_fcurve_get_cycle_type(FCurve *fcu)
{ {
FModifier *fcm = fcu->modifiers.first; FModifier *fcm = fcu->modifiers.first;
if (!fcm || fcm->type != FMODIFIER_TYPE_CYCLES) { if (!fcm || fcm->type != FMODIFIER_TYPE_CYCLES) {
return FCU_CYCLE_NONE; return FCU_CYCLE_NONE;
} }
if (fcm->flag & (FMODIFIER_FLAG_DISABLED | FMODIFIER_FLAG_MUTED)) { if (fcm->flag & (FMODIFIER_FLAG_DISABLED | FMODIFIER_FLAG_MUTED)) {
return FCU_CYCLE_NONE; return FCU_CYCLE_NONE;
} }
sybrenUnsubmitted
Done
Inline Actions

"We match" is ambiguous. My grammar terminology is failing me here; I can read "we match the baked curve shape" as "we make sure [the unbaked curve shape] matches the baked curve shape", but also as "our shape matches the baked curve shape". Are you saying I should go to the gym more often? ;-)

I think "Baked FCurve points always use linear interpolation." would be clearer.

sybren: "We match" is ambiguous. My grammar terminology is failing me here; I can read "we match the…
if (fcm->flag & (FMODIFIER_FLAG_RANGERESTRICT | FMODIFIER_FLAG_USEINFLUENCE)) { if (fcm->flag & (FMODIFIER_FLAG_RANGERESTRICT | FMODIFIER_FLAG_USEINFLUENCE)) {
return FCU_CYCLE_NONE; return FCU_CYCLE_NONE;
} }
FMod_Cycles *data = (FMod_Cycles *)fcm->data; FMod_Cycles *data = (FMod_Cycles *)fcm->data;
if (data && data->after_cycles == 0 && data->before_cycles == 0) { if (data && data->after_cycles == 0 && data->before_cycles == 0) {
if (data->before_mode == FCM_EXTRAPOLATE_CYCLIC && if (data->before_mode == FCM_EXTRAPOLATE_CYCLIC &&
data->after_mode == FCM_EXTRAPOLATE_CYCLIC) { data->after_mode == FCM_EXTRAPOLATE_CYCLIC) {
sybrenUnsubmitted
Done
Inline Actions

The name is supposed to be in the form TODO(aligorith). But, given upcoming changes to the commenting guidelines (T81452, most importantly Brecht's comment T81452#1029052), I'm not even sure what will be the final verdict on/form of this. I think I agree with Brecht in that the comment should be so complete that it doesn't require any knowledge of who added it or who to ask for more information. So in that light, and sorry for the inconsistency, I withdraw my earlier "add names to TODOs" note.

sybren: The name is supposed to be in the form `TODO(aligorith)`. But, given upcoming changes to the…
zeddbAuthorUnsubmitted
Done
Inline Actions

This is consistent with the rest of the file now.
If we are going to change it again, that should be in an other commit.

zeddb: This is consistent with the rest of the file now. If we are going to change it again, that…
sybrenUnsubmitted
Done
Inline Actions

👍

sybren: :+1:
return FCU_CYCLE_PERFECT; return FCU_CYCLE_PERFECT;
} }
if (ELEM(data->before_mode, FCM_EXTRAPOLATE_CYCLIC, FCM_EXTRAPOLATE_CYCLIC_OFFSET) && if (ELEM(data->before_mode, FCM_EXTRAPOLATE_CYCLIC, FCM_EXTRAPOLATE_CYCLIC_OFFSET) &&
ELEM(data->after_mode, FCM_EXTRAPOLATE_CYCLIC, FCM_EXTRAPOLATE_CYCLIC_OFFSET)) { ELEM(data->after_mode, FCM_EXTRAPOLATE_CYCLIC, FCM_EXTRAPOLATE_CYCLIC_OFFSET)) {
return FCU_CYCLE_OFFSET; return FCU_CYCLE_OFFSET;
} }
} }
▲ Show 20 Lines • Show All 964 Lines • Show Last 20 Lines