Differential D4841 Diff 15264 source/blender/editors/space_graph/graph_edit.c

Changeset View

Standalone View

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(ac, 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;
		float ratio;

		/* get editor data */
		if (ANIM_animdata_get_context(C, &ac) == 0) {
		return OPERATOR_CANCELLED;
		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…
		}

		/* get cleaning threshold */
		ratio = RNA_float_get(op->ptr, "ratio");
		/* clean keyframes */
		decimate_graph_keys(&ac, ratio);

		/* 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)
		{
		/* identifiers */
		ot->name = "Decimate Keyframes";
		ot->idname = "GRAPH_OT_decimate";
		ot->description =
		"Decimate F-Curves by removing the ratio keyframes that influence the curve shape the "
		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
		"least.";

		/* api callbacks */
		// ot->invoke = // XXX we need that number popup for this!
		ot->exec = graphkeys_decimate_exec;
		ot->poll = graphop_editable_keyframes_poll;

		/* flags */
		ot->flag = OPTYPE_REGISTER \| OPTYPE_UNDO;

		/* properties */
		ot->prop = RNA_def_float(ot->srna, "ratio", 0.001f, 0.0f, FLT_MAX, "Ratio", "", 0.0f, 1.0f);
		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…
		}

	/* ****************** 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.
		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…
		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.
		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.
		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…
		sybrenUnsubmitted Not Done Inline Actions As discussed face-to-face: ratio → percentage sybren: As discussed face-to-face: ratio → percentage
		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…