Cleanup Pose Slider code
AbandonedPublic
Actions

Authored by Christoph Lendenfeld (ChrisLend) on May 18 2021, 12:10 AM.

Details

Reviewers

Campbell Barton (campbellbarton)
Sybren A. Stüvel (sybren)

Group Reviewers

Animation & Rigging

Summary

As correctly pointed out by @Campbell Barton (campbellbarton) there were some issues with D9054 that I missed
Those are fixed in this patch

The slider value is now called factor instead of percentage in the gui
remove usages of vec2f and replace them with float[2]
update the doc string for region in the tPoseSlideOp struct
change term workspace footer to status bar
rename percentage variable to factor
rename region to region_header and ensure it is always only a header
changed struct rctf to rctf
use STRNCPY instead of BLI_strncpy. Some lines use BLI_snprintf which I didn't change yet as I am not 100% clear what the difference is

Diff Detail

Repository: rB Blender

Event Timeline

Christoph Lendenfeld (ChrisLend) requested review of this revision.May 18 2021, 12:10 AM

Christoph Lendenfeld (ChrisLend) created this revision.

Campbell Barton (campbellbarton) accepted this revision.May 18 2021, 2:49 AM

This revision is now accepted and ready to land.May 18 2021, 2:49 AM

Sybren A. Stüvel (sybren) requested changes to this revision.May 18 2021, 10:07 AM

Sybren A. Stüvel (sybren) added a reviewer: Sybren A. Stüvel (sybren). Sybren A. Stüvel (sybren) added 1 blocking reviewer(s): Animation & Rigging.

Sybren A. Stüvel (sybren) added inline comments.

source/blender/editors/armature/pose_slide.c
153	Let's rename this to `factor` as well then. Currently there are two names (`factor` in RNA and `percentage` in the struct) for the same thing, and that will at some point lead to confusion.

This revision now requires review to proceed.May 18 2021, 10:07 AM

Campbell Barton (campbellbarton) added inline comments.May 18 2021, 11:03 AM

source/blender/editors/armature/pose_slide.c
153	Noticed this too, I didn't think it's necessary to do as part of a follow up to D9054. If a general cleanup is being done, some suggestions: Rename `region` to `region_header` since a modal operator storing a region generally refers to the active window region, I find it confusing this happens to refer to the header and only sometimes, which is error prone. Use enum types for `tPoseSlideOp` (mode` `channels` & `axislock`). `struct rctf` can be `rctf`. `BLI_strncpy` -> `STRNCPY` (avoids extra sizeof args everywhere in status bar drawing). .. although no problem

rename percentage variable to factor
rename region to region_header and ensure it is always only a header
changed struct rctf to rctf
use STRNCPY instead of BLI_strncpy. Some lines use BLI_snprintf which I didn't change yet as I am not 100% clear what the difference is

@Campbell Barton (campbellbarton) what do you mean by

Use enum types for tPoseSlideOp

In D11284#288980, @Christoph Lendenfeld (ChrisLend) wrote:

@Campbell Barton (campbellbarton) what do you mean by

Use enum types for tPoseSlideOp

There are enum types being used in tPoseSlideOp which are using int/short types. This looses some type validation (no warning when mixing incompatible enums for example).

/** axis-limits for transforms (ePoseSlide_AxisLock) */
short axislock;

Would become:

/** Axis-limits for transforms. */
ePoseSlide_AxisLock axislock;

... similar changes for other enum types.

The enum definitions would need to be moved before tPoseSlideOp.

source/blender/editors/armature/pose_slide.c
150	This should be renamed too.

Campbell Barton (campbellbarton) added inline comments.May 19 2021, 8:07 AM

source/blender/editors/armature/pose_slide.c
297	Pass a pointer to the struct instead of it's value.

If you want, this patch could be split up into multiple commits. percentage → factor as one commit, BLI_strncpy → STRNCPY as another, etc. That way only one thing happens in each commit. I think this'll make things easier to describe in the commit message, and easier to understand when looking back at changes. With git rebase -i and git gui you can fairly easily split up a single commit into several; I can do that when landing the patch, or you can do it yourself and upload as several patches, I'll leave that to you.