Changeset View
Standalone View
source/blender/editors/animation/keyframes_general.c
| Context not available. | |||||
| #include "BLI_blenlib.h" | #include "BLI_blenlib.h" | ||||
| #include "BLI_utildefines.h" | #include "BLI_utildefines.h" | ||||
| #include "BLI_string_utils.h" | #include "BLI_string_utils.h" | ||||
| #include "BLI_math.h" | |||||
| #include "DNA_anim_types.h" | #include "DNA_anim_types.h" | ||||
| #include "DNA_object_types.h" | #include "DNA_object_types.h" | ||||
| #include "DNA_scene_types.h" | #include "DNA_scene_types.h" | ||||
| #include "BKE_action.h" | #include "BKE_action.h" | ||||
| #include "BKE_curve.h" | |||||
| #include "BKE_fcurve.h" | #include "BKE_fcurve.h" | ||||
| #include "BKE_report.h" | #include "BKE_report.h" | ||||
| #include "BKE_main.h" | #include "BKE_main.h" | ||||
| Context not available. | |||||
| /* ---------------- */ | /* ---------------- */ | ||||
| /** | |||||
| * F-Curve 'decimate' function that removes a certain ratio of curve | |||||
| * points that will affect the curves overall shape the least. | |||||
| */ | |||||
| void decimate_fcurve(bAnimListElem *ale, float ratio) | |||||
| { | |||||
| FCurve *fcu = (FCurve *)ale->key_data; | |||||
| BezTriple *old_bezts, *bezt; | |||||
| /* Check if the curve actually has any points */ | |||||
| if (fcu == NULL || fcu->bezt == NULL || fcu->totvert == 0) { | |||||
| return; | |||||
| } | |||||
| const int target_fcurve_verts = max_ii(2, fcu->totvert - fcu->totvert * ratio); | |||||
sybren: You may want to add a comment here that explains that you don't want to decimate by using the… | |||||
Done Inline ActionsThe error here is a bit unclear; it could mean "this is the target length of the keyframes in error", e.g. the ones that are removed, or it could be "this is the target length of the keyframes to keep, for whatever function deals with errory things", e.g. the ones to keep. sybren: The `error` here is a bit unclear; it could mean "this is the target length of the keyframes in… | |||||
| old_bezts = fcu->bezt; | |||||
Done Inline ActionsI wouldn't mind just passing BEZT_FLAG_TEMP_TAG directly to the BKE_curve_decimate_bezt_array() function. It's only used in one place anyway. The other variables at least clarify the numbers assigned to them, so they have added value, but passing a flag is IMO clear enough. sybren: I wouldn't mind just passing `BEZT_FLAG_TEMP_TAG` directly to the… | |||||
Done Inline ActionsThe above variables could be declared in the if-block they are used in. sybren: The above variables could be declared in the `if`-block they are used in. | |||||
sybrenUnsubmitted Done Inline ActionsThe old_bezts variable is declared at the top of the function, whereas target_fcurve_verts and num are declared at this spot. This seems a bit strange to me, to have two declaration styles used intermixed in new code. sybren: The `old_bezts` variable is declared at the top of the function, whereas `target_fcurve_verts`… | |||||
| uint num = fcu->totvert; | |||||
sybrenUnsubmitted Done Inline Actionsnum is unnecessarily short and undescriptive; totvert or even fcu_totvert makes things a lot clearer. The placement of this line is confusing; the code probably exists at all to make the for-loop below faster, but num is already declared here. Furthermore, the use of fcu->totvert in the if-statement below isn't using num even though it's closer to this line. Either declare here and avoid fcu->totvert in the if, or just declare it directly above the for-loop before the fcu->totvert = 0 assignment. sybren: `num` is unnecessarily short and undescriptive; `totvert` or even `fcu_totvert` makes things a… | |||||
| if (target_fcurve_verts != fcu->totvert) { | |||||
| /* We don't want to limit the decimation to a certain error margin */ | |||||
| const float error_sq_max = FLT_MAX; | |||||
| BKE_curve_decimate_bezt_array(fcu->bezt, | |||||
| fcu->totvert, | |||||
| 12, | |||||
sybrenUnsubmitted Done Inline ActionsMagic number; add a comment to explain why 12 is a good value here. sybren: Magic number; add a comment to explain why 12 is a good value here. | |||||
| false, | |||||
| SELECT, | |||||
| BEZT_FLAG_TEMP_TAG, | |||||
| error_sq_max, | |||||
| target_fcurve_verts); | |||||
| } | |||||
| fcu->bezt = NULL; | |||||
| fcu->totvert = 0; | |||||
| for (int i = 0; i < num; i++) { | |||||
| bezt = (old_bezts + i); | |||||
| if ((bezt->f2 & BEZT_FLAG_TEMP_TAG) == 0) { | |||||
| insert_bezt_fcurve(fcu, bezt, 0); | |||||
| } | |||||
| } | |||||
| /* now free the memory used by the old BezTriples */ | |||||
| if (old_bezts) { | |||||
| MEM_freeN(old_bezts); | |||||
| } | |||||
| } | |||||
| /* ---------------- */ | |||||
| /* temp struct used for smooth_fcurve */ | /* temp struct used for smooth_fcurve */ | ||||
| typedef struct tSmooth_Bezt { | typedef struct tSmooth_Bezt { | ||||
| float *h1, *h2, *h3; /* bezt->vec[0,1,2][1] */ | float *h1, *h2, *h3; /* bezt->vec[0,1,2][1] */ | ||||
| Context not available. | |||||
Done Inline ActionsUnused parameter ‘ac’. sybren: Unused parameter ‘ac’. | |||||
Done Inline ActionsNo need for parentheses around the equality checks. sybren: No need for parentheses around the equality checks. | |||||
Done Inline ActionsI would always write full sentences, so "Check if there are any points." or something like that. sybren: I would always write full sentences, so "Check if there are any points." or something like that. | |||||
You may want to add a comment here that explains that you don't want to decimate by using the error, but only by the number of remaining keyframes.