Refactor: pose_slide.c #107610

Christoph Lendenfeld · 2023-05-04T11:49:30+02:00

2023-05-04 11:49:30 +02:00

add const where possible
reduce calls to ED_slider_factor_get and instead store that in const float factor
rename variables from camel case to snake case, using full words, to conform to naming convention
use interpf where possible

no functional changes

split off from: #107397: Fix: Slider bidirectionality in pose_slide.c

* add const where possible * reduce calls to `ED_slider_factor_get` and instead store that in `const float factor` * rename variables from camel case to snake case, using full words, to conform to naming convention * use interpf where possible no functional changes split off from: [#107397: Fix: Slider bidirectionality in pose_slide.c](https://projects.blender.org/blender/blender/pulls/107397)

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2023-05-04 11:49:30 +02:00

Christoph Lendenfeld added 1 commit 2023-05-04 11:49:36 +02:00

f875116cc5 constness, interpf, variable naming

Christoph Lendenfeld requested review from Sybren A. Stüvel 2023-05-04 12:14:37 +02:00

Christoph Lendenfeld referenced this pull request

2023-05-04 12:17:01 +02:00

WIP: Fix: Slider bidirectionality in pose_slide.c #107397

Sybren A. Stüvel approved these changes 2023-05-05 12:37:17 +02:00

Sybren A. Stüvel left a comment

LGTM! Just a few tiny things that can be addressed before landing the patch. Thanks!

source/blender/editors/armature/pose_slide.c Outdated

						
				@ -362,2 +360,2 @@

				  float sVal, eVal;

				  float w1, w2;

				  float prev_frame, next_frame;

				  float next_weight, prev_weight;

Sybren A. Stüvel commented

2023-05-05 12:22:52 +02:00

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

source/blender/editors/armature/pose_slide.c Outdated

						
				@ -376,2 +370,2 @@

				    w1 = ED_slider_factor_get(pso->slider); /* This must come second. */

				    w2 = 1.0f - w1;                         /* This must come first. */

				    next_weight = factor;             /* This must come second. */

				    prev_weight = 1.0f - next_weight; /* This must come first. */

Sybren A. Stüvel commented

2023-05-05 12:25:40 +02:00

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.

source/blender/editors/armature/pose_slide.c Outdated

						
				@ -397,1 +393,3 @@

				   *   the value the current frame is closer to.

				   * - 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.

Sybren A. Stüvel commented

2023-05-05 12:26:41 +02:00

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.

source/blender/editors/armature/pose_slide.c Outdated

						
				@ -687,2 +681,3 @@

				        /* Compute breakdown based on actual frame range. */

				        const float factor = (cframe - pso->prevFrame) / (float)(pso->nextFrame - pso->prevFrame);

				        const float interp_factor = (current_frame - pso->prevFrame) /

				                                    (float)(pso->nextFrame - pso->prevFrame);