Details

Reviewers

Sybren A. Stüvel (sybren)

Group Reviewers

Animation & Rigging

Maniphest Tasks

T81785: Implementation: Modal Key Manipulation Operators

Commits

rBfafd21b14c23: Animation: Generic Slider implementation

Summary

Slider upgrades

This patch adds a generic slider that can be used for GRAPH_OT_decimate and for the pose sliders in pose_slide.c.
It also implements it to pose_slide.c

Usage

an operator that wants to use this slider needs to save a pointer to tSlider. An initialized *tSlider is returned from ED_slider_create for that
modal operations should use ED_slider_init after create to set the needed values for mouse movement calculation
call ED_slider_modal to update the percentage value and the drawing
use ED_slider_status_string_get to get a string with help info that can be printed to the status bar
use ED_slider_destroy to free the memory and remove the drawing callback
use ED_slider_factor_get and ED_slider_factor_set for accessing the calculated factor
use ED_slider_allow_overshoot_get and ED_slider_allow_overshoot_set to disallow overshoot. some operators don't handle values beyond the 0-1 range

Diff Detail

Repository: rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Christoph Lendenfeld (ChrisLend) requested review of this revision.Oct 22 2020, 5:23 PM

Christoph Lendenfeld (ChrisLend) added a task: T81785: Implementation: Modal Key Manipulation Operators.

Christoph Lendenfeld (ChrisLend) mentioned this in T81785: Implementation: Modal Key Manipulation Operators.

Christoph Lendenfeld (ChrisLend) mentioned this in D9317: Extract functions from GRAPH_OT_decimate.Oct 22 2020, 7:52 PM

Christoph Lendenfeld (ChrisLend) edited the summary of this revision. (Show Details)

Christoph Lendenfeld (ChrisLend) mentioned this in D9326: Create generic modal functions from GRAPH_OT_decimate.Oct 23 2020, 12:30 PM

Christoph Lendenfeld (ChrisLend) edited the summary of this revision. (Show Details)Oct 23 2020, 12:37 PM

Moved the implementation of tSlider to ED_util.h

Christoph Lendenfeld (ChrisLend) set the repository for this revision to rB Blender.Oct 27 2020, 5:54 PM

rebase

update patch to use changes from D9054 which is now in master

Christoph Lendenfeld (ChrisLend) mentioned this in D9513: Add drawing to generic slider.May 16 2021, 7:33 PM

Christoph Lendenfeld (ChrisLend) edited the summary of this revision. (Show Details)May 16 2021, 10:17 PM

Christoph Lendenfeld (ChrisLend) edited the summary of this revision. (Show Details)

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.May 17 2021, 11:20 AM

Gavriel Lavi (Gavriel5578) awarded a token.May 29 2021, 10:34 PM

There are some compiler warnings now:

.../blender/source/blender/editors/util/ed_draw.c: In function ‘ED_slider_update’:
.../blender/source/blender/editors/util/ed_draw.c:339:46: warning: unused parameter ‘C’ [-Wunused-parameter]
  339 | void ED_slider_update(const struct bContext *C, tSlider *slider, const wmEvent *event)
      |                       ~~~~~~~~~~~~~~~~~~~~~~~^
.../blender/source/blender/editors/util/ed_draw.c: In function ‘ED_slider_status_string’:
.../blender/source/blender/editors/util/ed_draw.c:422:10: warning: function returns address of local variable [-Wreturn-local-addr]
  422 |   return status_string;
      |          ^~~~~~~~~~~~~
.../blender/source/blender/editors/util/ed_draw.c: In function ‘ED_slider_exit’:
.../blender/source/blender/editors/util/ed_draw.c:430:28: warning: passing argument 1 of ‘ED_workspace_status_text’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
  430 |   ED_workspace_status_text(C, NULL);
      |                            ^
In file included from .../blender/source/blender/editors/util/ed_draw.c:46:
.../blender/source/blender/editors/include/ED_screen.h:287:48: note: expected ‘struct bContext *’ but argument is of type ‘const struct bContext *’
  287 | void ED_workspace_status_text(struct bContext *C, const char *str);
      |                               ~~~~~~~~~~~~~~~~~^

Once the code in this patch is good, I'm still in doubt what to do next. One thing that I try to avoid in general is committing unused code. I think it the removal of the non-generic code from the pose slider tools should be removed in this patch as well. That, or this slider should be used from another operator, which then serves as a clear example of how it should be used. In one case the "this code is moved" concept is clearer, but the other case the "this is how this slider is used" example effect is clearer.

In the end I think the "this code is moved" aspect is more important. By having it in the same commit, it's possible to really see that it's a proper move.

source/blender/editors/util/ed_draw.c
381	This function isn't going to work well, because it returns a pointer to a variable that was allocated on the stack. Either allocate the string on the heap, or have the caller supply the `status_string[UI_MAX_DRAW_STR]` pointer.

Daniel Salazar (zanqdo) awarded a token.Jun 10 2021, 6:22 PM

fixed the warnings
changed ED_slider_status_string to take a char * parameter instead of returning one. Please let me know if that is good.
Implemented the generic slider in pose_slide.c. I think it's a good idea to do that right away, though I am not sure if that's too many changes in one patch

Including the change to pose_slide.c is good, because it shows the new API in use. Makes it much easier to review :)

One thing that pops out is that it smells a bit of "feature envy" (described in Clean Code by Robert C. Martin). pso->slider->factor shouldn't be accessed by the code that manages pso, but only by the slider API. If some of the "innards" of the slider struct are part of the public API, this basically means that all of the innards are now part of that API, which makes for strongly coupled code.

You could make the tSlider struct an opaque struct. That way it's impossible for any other code to poke its innards, and expose only an API of some C functions. You could then add

struct tSlider;
float ED_slider_factor_get(struct tSlider *slider);
void ED_slider_factor_set(struct tSlider *slider, float factor);

The setter function can then also be used to do clamping if necessary. If you want, you can make the setter return the eventually set factor as well, so that something that needs the post-processed value can do so without calling the getter again.

source/blender/editors/armature/pose_slide.c
172 ↗	(On Diff #38428)	No need to add a comment that adds a second repetition of the same word ;-)
source/blender/editors/include/ED_util.h
97	All the existing `ED_..._init()` functions only initialize, so they take a pointer the thing to init and they return `void`. In this case you "create" (i.e. alloc + init), so `ED_slider_create()` would be better.
98	I think `ED_slider_modal()` would be a better name, as "setting the status string" and "changing the factor" also are "updates".
99	The name `ED_slider_status_string()` doesn't tell you whether the status string is obtained or set. `status_string` being non-const hints to the former, but `slider` being non-const hints to the former. I'd name it to `ED_slider_status_string_get()` and use `const tSlider *slider`.
99	There should be an indication (just a comment would do) as to how large `status_string` has to be in order to not get some overflow. Alternatively, add a parameter that tells `ED_slider_status_string()` how much space it can use.
100	To make the naming symmetrical with `ED_slider_create()`, `ED_slider_destroy()` would be better.
source/blender/editors/util/ed_draw.c
407–409	Document where the 50 comes from.

This revision now requires changes to proceed.Jun 17 2021, 11:02 AM

Thanks for the feedback @Sybren A. Stüvel (sybren)
Really valuable guidance in terms of code design

made tSlider an opaque struct
added setters and getters for factor and allow_overshoot
changed naming: init -> create, update -> modal, exit -> destroy
changed parameters of ED_slider_status_string_get to pass a string size value
update summary to reflect changes

added a comment to ED_slider_satatus_string_get

Update from master and include comment style changes

Christoph Lendenfeld (ChrisLend) mentioned this in D9361: Implement generic slider in graph_slider_ops.Jun 26 2021, 8:51 PM

Sybren A. Stüvel (sybren) requested changes to this revision.Jul 2 2021, 12:06 PM

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

source/blender/editors/util/ed_draw.c
436	Change `int string_length` to `const size_t sizeof_status_string` (so rename + const + type change). Rename 1: repeating `status_string` in the name makes it even clearer that it relates to `status_string` Rename 2: having `sizeof` instead of `length` makes it clearer it's really an indication of the memory size, and not something that contains the length of the string (i.e. the number of bytes before the terminating `\0`). `const`: because this function doesn't change the parameter's value, and `size_t` because it's getting its value from `sizeof()` (which returns `size_t`) and passing it to a function expecting a `size_t`.
489	`const float factor`
502	`const bool value`

This revision now requires changes to proceed.Jul 2 2021, 12:06 PM

change setter parameters to const
rename string size parameter and use size_t

Christoph Lendenfeld (ChrisLend) marked 4 inline comments as done.Jul 4 2021, 1:36 PM

I'm now getting build errors:

blender/source/blender/editors/util/ed_draw.c:354:6: warning: no previous prototype for ‘slider_update_factor’ [-Wmissing-prototypes]
  354 | void slider_update_factor(tSlider *slider, const wmEvent *event)
      |      ^~~~~~~~~~~~~~~~~~~~
blender/source/blender/editors/util/ed_draw.c:436:6: error: conflicting types for ‘ED_slider_status_string_get’
  436 | void ED_slider_status_string_get(const tSlider *slider,
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from blender/source/blender/editors/util/ed_draw.c:48:
blender/source/blender/editors/include/ED_util.h:72:6: note: previous declaration of ‘ED_slider_status_string_get’ was here
   72 | void ED_slider_status_string_get(const struct tSlider *slider,
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~

Sybren A. Stüvel (sybren) requested changes to this revision.Jul 8 2021, 12:19 PM

This revision now requires changes to proceed.Jul 8 2021, 12:19 PM

Sorry about that
The compiler on Windows didn't complain about that for some reason
Fixed now by tweaking the declaration in ED_util.h

Christoph Lendenfeld (ChrisLend) added a child revision: D11878: Generic Pose slider Color Change.Jul 10 2021, 7:12 PM

Erick Tukuniata (erickblender) awarded a token.Jul 10 2021, 10:18 PM

Sybren A. Stüvel (sybren) accepted this revision.Jul 12 2021, 5:34 PM

This revision is now accepted and ready to land.Jul 12 2021, 5:34 PM

After a bit of testing, I found a way to crash the operator:

Use the breakdowner, and confirm the new pose
Open the redo panel
Change a setting there, causing the operator to be re-executed

This crashes because slider is NULL in ED_slider_factor_get():

ED_slider_factor_get(struct tSlider * slider) (blender/source/blender/editors/util/ed_draw.c:492)
pose_slide_apply_val(tPoseSlideOp * pso, FCurve * fcu, Object * ob, float * val) (blender/source/blender/editors/armature/pose_slide.c:379)
pose_slide_apply_vec3(tPoseSlideOp * pso, tPChanFCurveLink * pfl, float * vec, const char * propName) (blender/source/blender/editors/armature/pose_slide.c:457)
pose_slide_apply(bContext * C, tPoseSlideOp * pso) (blender/source/blender/editors/armature/pose_slide.c:785)
pose_slide_exec_common(bContext * C, wmOperator * op, tPoseSlideOp * pso) (blender/source/blender/editors/armature/pose_slide.c:1302)
pose_slide_breakdown_exec(bContext * C, wmOperator * op) (blender/source/blender/editors/armature/pose_slide.c:1663)
wm_operator_exec(bContext * C, wmOperator * op, const _Bool repeat, const _Bool store) (blender/source/blender/windowmanager/intern/wm_event_system.c:1065)
WM_operator_repeat(bContext * C, wmOperator * op) (blender/source/blender/windowmanager/intern/wm_event_system.c:1142)
ED_undo_operator_repeat(bContext * C, wmOperator * op) (blender/source/blender/editors/undo/ed_undo.c:722)
ED_undo_operator_repeat_cb_evt(bContext * C, void * arg_op, int UNUSED_arg_unused) (blender/source/blender/editors/undo/ed_undo.c:756)
ui_apply_but_funcs_after(bContext * C) (blender/source/blender/editors/interface/interface_handlers.c:983)
ui_handler_region_menu(bContext * C, const wmEvent * event, void * UNUSED_userdata) (blender/source/blender/editors/interface/interface_handlers.c:11032)
wm_handler_ui_call(bContext * C, wmEventHandler_UI * handler, const wmEvent * event, int always_pass) (blender/source/blender/windowmanager/intern/wm_event_system.c:692)
wm_handlers_do_intern(bContext * C, wmEvent * event, ListBase * handlers) (blender/source/blender/windowmanager/intern/wm_event_system.c:2841)
wm_handlers_do(bContext * C, wmEvent * event, ListBase * handlers) (blender/source/blender/windowmanager/intern/wm_event_system.c:2957)
wm_event_do_handlers(bContext * C) (blender/source/blender/windowmanager/intern/wm_event_system.c:3377)
WM_main(bContext * C) (blender/source/blender/windowmanager/intern/wm.c:647)
main(int argc, const char ** argv) (blender/source/creator/creator.c:558)

This revision now requires changes to proceed.Jul 12 2021, 6:06 PM

thanks for catching that @Sybren A. Stüvel (sybren)

In order to fix that I had to split the ED_slider_create function into two parts

ED_slider_create
ED_slider_init

The only difference is, that create doesn't set the float last_cursor[2] anymore, but that is moved to the init function.

The reason for that is that in the non-modal operator execution there is no wmEvent from which we could get the mouse cursor position. So now it goes like this:

exec() called: do only create
invoke() called: create and init

Updated the patch description

The splitup is nice, but it has some downsides too. Now you have code like this:

ED_slider_init(pso->slider, event);

/* Do common setup work. */
return pose_slide_invoke_common(C, op, pso);

It's a bit weird to have the call to ED_slider_init() before every call to pose_slide_invoke_common() -- given that it's always required, it would certainly match "common setup work", so by your own comments it should be part of pose_slide_invoke_common(). Maybe it's better to just pass event to pose_slide_invoke_common() and let that function do the setup?

Sybren A. Stüvel (sybren) requested changes to this revision.Jul 20 2021, 1:08 PM

This revision now requires changes to proceed.Jul 20 2021, 1:08 PM

moved ED_slider_init into pose_slide_invoke_common
for this I am now passing wmEvent but I have removed the tPoseSlideOp from the parameters since it can be gotten from op->customdata

that simplified it nicely

That cleaned it up well, indeed!

All that's left now is one compiler warning about the constness of the event pointer. Apply this to fix it:

diff --git a/source/blender/editors/armature/pose_slide.c b/source/blender/editors/armature/pose_slide.c
index a597abc2324..e87d221058c 100644
--- a/source/blender/editors/armature/pose_slide.c
+++ b/source/blender/editors/armature/pose_slide.c
@@ -955,7 +955,7 @@ static void pose_slide_draw_status(bContext *C, tPoseSlideOp *pso)
 /**
  * Common code for invoke() methods.
  */
-static int pose_slide_invoke_common(bContext *C, wmOperator *op, wmEvent *event)
+static int pose_slide_invoke_common(bContext *C, wmOperator *op, const wmEvent *event)
 {
   tPChanFCurveLink *pfl;
   wmWindow *win = CTX_wm_window(C);

Since this is such a small change, just do the change, update the patch here (so that what is committed == what is in the patch), and commit it to master. No need to wait for another review pass.

This revision is now accepted and ready to land.Jul 22 2021, 2:23 PM