blender
/
blender
136
406
Fork 590
Code Issues 6.2k Pull Requests 764 Projects 19 Wiki Activity

Refactor: pose_slide.c #107610

Merged
Christoph Lendenfeld merged 4 commits from ChrisLend/blender:refactor_pose_slide into main 2023-05-05 15:30:36 +02:00
Conversation 0 Commits 4 Files Changed 1 +6 -10
1 changed files with 6 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 0266227240 - Show all commits

16
source/blender/editors/armature/pose_slide.c
View File

@ -358,7 +358,7 @@ static bool pose_frame_range_from_object_get(tPoseSlideOp *pso,
static void pose_slide_apply_val(tPoseSlideOp *pso, FCurve *fcu, Object *ob, float *val)
{
float prev_frame, next_frame;
float next_weight, prev_weight;
float prev_weight, next_weight;
Sybren A. Stüvel commented 2023-05-05 12:22:52 +02:00
Outdated
Review
Copy Link

Now that you're cleaning up anyway, probably a good idea to keep the prev/next order consistent between the two lines.

Now that you're cleaning up anyway, probably a good idea to keep the prev/next order consistent between the two lines.
pose_frame_range_from_object_get(pso, ob, &prev_frame, &next_frame);
const float factor = ED_slider_factor_get(pso->slider);
@ -367,8 +367,8 @@ static void pose_slide_apply_val(tPoseSlideOp *pso, FCurve *fcu, Object *ob, flo
/* Calculate the relative weights of the endpoints. */
if (pso->mode == POSESLIDE_BREAKDOWN) {
/* Get weights from the factor control. */
next_weight = factor; /* This must come second. */
prev_weight = 1.0f - next_weight; /* This must come first. */
next_weight = factor;
prev_weight = 1.0f - next_weight;
Sybren A. Stüvel commented 2023-05-05 12:25:40 +02:00
Outdated
Review
Copy Link

AAAAAAAA those comments (I know, not yours, just AAAAAA). IMO you can just remove those as well. I mean, saying "this must come first" on the 2nd thing just makes no sense to me.

AAAAAAAA those comments (I know, not yours, just AAAAAA). IMO you can just remove those as well. I mean, saying "this must come first" on the 2nd thing just makes no sense to me.
}
else {
/* - these weights are derived from the relative distance of these
@ -389,11 +389,7 @@ static void pose_slide_apply_val(tPoseSlideOp *pso, FCurve *fcu, Object *ob, flo
const float prev_frame_y = evaluate_fcurve(fcu, prev_frame);
const float next_frame_y = evaluate_fcurve(fcu, next_frame);
/* Depending on the mode, calculate the new value:
* - In all of these, the start+end values are multiplied by prev_weight and next_weight
* (respectively), since multiplication in another order would decrease the value the current
* frame is closer to.
*/
/* Depending on the mode, calculate the new value. */
switch (pso->mode) {
case POSESLIDE_PUSH: /* Make the current pose more pronounced. */
{
Sybren A. Stüvel commented 2023-05-05 12:26:41 +02:00
Outdated
Review
Copy Link

Aaaah that is what those comments were referring to. Yeah, just remove them. This one explains it all, and is just as clear without the confusion.

... wait... no it doesn't make sense at all. The 'prev' and 'next' values are just summed, so it doesn't matter in which order they're calculated. And with the naming that you're introducing now it's so logical that you multiply the prevs together and the nexts, that IMO this comment can just be removed as well.

Aaaah **that** is what those comments were referring to. Yeah, just remove them. This one explains it all, and is just as clear without the confusion. ... wait... no it doesn't make sense at all. The 'prev' and 'next' values are just summed, so it doesn't matter in which order they're calculated. And with the naming that you're introducing now it's *so* logical that you multiply the `prev`s together and the `next`s, that IMO this comment can just be removed as well.
@ -685,11 +681,11 @@ static void pose_slide_apply_quat(tPoseSlideOp *pso, tPChanFCurveLink *pfl)
interp_qt_qtqt(quat_breakdown, quat_prev, quat_next, interp_factor);
if (pso->mode == POSESLIDE_PUSH) {
Sybren A. Stüvel commented 2023-05-05 12:36:35 +02:00
Outdated
Review
Copy Link

Could you use interpf() here as well? Might simplify the code a little bit.

Could you use `interpf()` here as well? Might simplify the code a little bit.
Christoph Lendenfeld commented 2023-05-05 15:11:49 +02:00
Outdated
Review
Copy Link

not sure how tbh. This is calculation the factor to use in the interp_qt
how would I get the 0-1 value from an interpf

not sure how tbh. This is calculation the factor to use in the `interp_qt` how would I get the 0-1 value from an `interpf`
Sybren A. Stüvel commented 2023-05-05 15:13:22 +02:00
Outdated
Review
Copy Link

You're right, I mis-read.

You're right, I mis-read.
interp_qt_qtqt(quat_final, quat_breakdown, quat_curr, 1.0f + interp_factor);
interp_qt_qtqt(quat_final, quat_breakdown, quat_curr, 1.0f + factor);
}
else {
BLI_assert(pso->mode == POSESLIDE_RELAX);
interp_qt_qtqt(quat_final, quat_curr, quat_breakdown, interp_factor);
interp_qt_qtqt(quat_final, quat_curr, quat_breakdown, factor);
}
}
}