Page MenuHome
Differential D4841 Diff 19226 source/blender/editors/space_graph/graph_edit.c

Changeset View

Standalone View

View Options

source/blender/editors/space_graph/graph_edit.c

Context not available.
RNA_def_boolean(ot->srna, "channels", false, "Channels", ""); RNA_def_boolean(ot->srna, "channels", false, "Channels", "");
} }
/* ******************** Decimate Keyframes Operator ************************* */
static void decimate_graph_keys(bAnimContext *ac, float ratio)
{
ListBase anim_data = {NULL, NULL};
bAnimListElem *ale;
int filter;
/* filter data */
filter = (ANIMFILTER_DATA_VISIBLE | ANIMFILTER_CURVE_VISIBLE | ANIMFILTER_FOREDIT |
ANIMFILTER_SEL | ANIMFILTER_NODUPLIS);
ANIM_animdata_filter(ac, &anim_data, filter, ac->data, ac->datatype);
/* loop through filtered data and clean curves */
for (ale = anim_data.first; ale; ale = ale->next) {
decimate_fcurve(ale, ratio);
ale->update |= ANIM_UPDATE_DEFAULT;
}
ANIM_animdata_update(ac, &anim_data);
ANIM_animdata_freelist(&anim_data);
}
/* ------------------- */
static int graphkeys_decimate_exec(bContext *C, wmOperator *op)
{
bAnimContext ac;
sybrenUnsubmitted
Done
Inline Actions

Shouldn't this be done in a poll function?

sybren: Shouldn't this be done in a poll function?
zeddbAuthorUnsubmitted
Done
Inline Actions

This is how it is done in the other exec functions in this file.
Should we change this?

zeddb: This is how it is done in the other exec functions in this file. Should we change this?
brechtUnsubmitted
Done
Inline Actions

The poll function already does it.

Strictly speaking, you could assume that this never returns 0. But still I think it's good practice to always check the return value of this function in case future refactoring breaks that assumption.

brecht: The poll function already does it. Strictly speaking, you could assume that this never returns…
float ratio;
/* get editor data */
if (ANIM_animdata_get_context(C, &ac) == 0) {
return OPERATOR_CANCELLED;
}
/* get cleaning threshold */
ratio = RNA_float_get(op->ptr, "ratio");
/* clean keyframes */
decimate_graph_keys(&ac, ratio);
sybrenUnsubmitted
Done
Inline Actions

There is no need to add a comment above every line, saying what the line does.

sybren: There is no need to add a comment above every line, saying what the line does.
/* set notifier that keyframes have changed */
WM_event_add_notifier(C, NC_ANIMATION | ND_KEYFRAME | NA_EDITED, NULL);
return OPERATOR_FINISHED;
}
void GRAPH_OT_decimate(wmOperatorType *ot)
{
sybrenUnsubmitted
Done
Inline Actions

I don't quite understand what "the ratio keyframes" means.

sybren: I don't quite understand what "the ratio keyframes" means.
zeddbAuthorUnsubmitted
Done
Inline Actions

I hope that the modified wording is clearer. I not we'll try to figure something out.

zeddb: I hope that the modified wording is clearer. I not we'll try to figure something out.
sybrenUnsubmitted
Done
Inline Actions

LGTM

sybren: LGTM
/* identifiers */
ot->name = "Decimate Keyframes";
ot->idname = "GRAPH_OT_decimate";
ot->description =
"Decimate F-Curves by removing keyframes that influence the curve shape the least";
/* api callbacks */
ot->exec = graphkeys_decimate_exec;
sybrenUnsubmitted
Done
Inline Actions

This line should IMO just be removed, or add a reference to a Txxx/Dxxx and make it possible for others to help out and implement the missing feature.

sybren: This line should IMO just be removed, or add a reference to a Txxx/Dxxx and make it possible…
ot->poll = graphop_editable_keyframes_poll;
/* flags */
ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO;
sybrenUnsubmitted
Done
Inline Actions

A ratio between 0 and FLT_MAX is weird. How can you remove more than 100% of the points?
It's also unclear what the ratio indicates. What would ratio=0.2 do? Remove 20% of the points? Or keep 20% of the points?

I've tried it with the default ratio, and it removed all the points except the first and last one, even though there were quite big jumps in the curve.

sybren: A ratio between 0 and `FLT_MAX` is weird. How can you remove more than 100% of the points? It's…
/* properties */
ot->prop = RNA_def_float_percentage(ot->srna,
sybrenUnsubmitted
Not Done
Inline Actions

As discussed face-to-face: change to 33.3333333333333333333333333333333333333333333333333333333333333333333% (so 1.0f/3.0f).

sybren: As discussed face-to-face: change to 33.3333333333333333333333333333333333333333333333333333333…
"ratio",
sybrenUnsubmitted
Done
Inline Actions

This still needs a description explaining what 'ratio' means. Right now it's unclear (it doesn't even say it's about the number of keyframes), and it's ambiguous whether this is the percentage to keep or the percentage to remove. Since the default value is 50%, you can't even tell which one it is from the result, as removing 50% is the same as retaining 50%.

sybren: This still needs a description explaining what 'ratio' means. Right now it's unclear (it…
sybrenUnsubmitted
Done
Inline Actions

Using RNA_def_float_percentage() would make sense here, I think.

sybren: Using `RNA_def_float_percentage()` would make sense here, I think.
0.5f,
0.0f,
sybrenUnsubmitted
Not Done
Inline Actions

Probably better to change the label to "Remove"; it's only shown in the redo panel, which already has "Decimate Keyframes" as title.

sybren: Probably better to change the label to "Remove"; it's only shown in the redo panel, which…
1.0f,
sybrenUnsubmitted
Not Done
Inline Actions

As discussed face-to-face: ratio → percentage

sybren: As discussed face-to-face: ratio → percentage
"Ratio",
"The ratio of keyframes to remove",
0.0f,
1.0f);
}
/* ******************** Bake F-Curve Operator *********************** */ /* ******************** Bake F-Curve Operator *********************** */
/* This operator bakes the data of the selected F-Curves to F-Points */ /* This operator bakes the data of the selected F-Curves to F-Points */
Context not available.