Refactor: Allow to explicitly set the range on the slider UI element #107406

Christoph Lendenfeld · 2023-04-27T18:00:27+02:00

2023-04-27 18:00:27 +02:00

The current implementation goes like this

default to a 0-1 range
you can set is_bidirectional to get a -1/1 range
if the slider is bidirectional the overshoot goes both ways, otherwise only on the positive side

The issue is that it is not possible to have a 0-1 range but overshoot both ways.
This is useful for example on the "Blend to Default" operator, where 0 means current position and 1 default.
But overshoot also makes sense for this operator since the other way just scales away from default.

So this patch:

removes is_bidirectional
adds float range[2] to tSlider and a setter for it
rewrite some logic to handle arbitrary ranges
split the allow_overshoot flag into one flag for either side

The current implementation goes like this * default to a 0-1 range * you can set `is_bidirectional` to get a -1/1 range * if the slider is bidirectional the overshoot goes both ways, otherwise only on the positive side The issue is that it is not possible to have a 0-1 range but overshoot both ways. This is useful for example on the "Blend to Default" operator, where 0 means current position and 1 default. But overshoot also makes sense for this operator since the other way just scales away from default. So this patch: * removes `is_bidirectional` * adds `float range[2]` to `tSlider` and a setter for it * rewrite some logic to handle arbitrary ranges * split the `allow_overshoot` flag into one flag for either side

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2023-04-27 18:00:27 +02:00

Christoph Lendenfeld added 1 commit 2023-04-27 18:00:33 +02:00

585f8e90c4 refactor code

Christoph Lendenfeld requested review from Sybren A. Stüvel 2023-04-27 18:01:10 +02:00

Christoph Lendenfeld commented

2023-04-27 18:31:14 +02:00

@dr.sybren I had a memory leak with this patch that I cannot reproduce again. The only possible culprit could be the range array, but if I understand correctly, as long as I don't calloc anything I don't need to free either. Is that right?

Sybren A. Stüvel commented

2023-05-02 10:40:34 +02:00

@dr.sybren I had a memory leak with this patch that I cannot reproduce again.

Hmm that's annoying. Have you tried with blender --debug-memory? Or the ASAN leak detector?

The only possible culprit could be the range array, but if I understand correctly, as long as I don't calloc anything I don't need to free either. Is that right?

That's correct, a fixed-size array is just allocated in-place.

I'm not sure how I feel about the range[2] though. Setting now requires two lines:

float range[2] = {-1, 1};
ED_slider_range_set(pbd->slider, range);

How would you feel splitting this up into factor_bound_lower and factor_bound_upper? That way we could just
have ED_slider_range_set(pbd->slider, -1, 1);

I was thinking of ..._min/..._max first, but IMO that would be inappropriate given that there's the possibility to overshoot.

As for the overshoot, I have the feeling that the code is now quite tightly coupled to the UI. That's not necessarily a bad thing, given that it's a UI element. What would you think of replacing allow_overshoot_{left,right} with allow_overshoot_{lower,upper}? That way it's in the 'space' of the value that's being manipulated, and the terminology would be the same as the field names for the upper/lower bounds.

> @dr.sybren I had a memory leak with this patch that I cannot reproduce again. Hmm that's annoying. Have you tried with `blender --debug-memory`? Or the ASAN leak detector? > The only possible culprit could be the range array, but if I understand correctly, as long as I don't calloc anything I don't need to free either. Is that right? That's correct, a fixed-size array is just allocated in-place. I'm not sure how I feel about the `range[2]` though. Setting now requires two lines: ```c float range[2] = {-1, 1}; ED_slider_range_set(pbd->slider, range); ``` How would you feel splitting this up into `factor_bound_lower` and `factor_bound_upper`? That way we could just have `ED_slider_range_set(pbd->slider, -1, 1);` I was thinking of `..._min`/`..._max` first, but IMO that would be inappropriate given that there's the possibility to overshoot. As for the overshoot, I have the feeling that the code is now quite tightly coupled to the UI. That's not necessarily a bad thing, given that it's a UI element. What would you think of replacing `allow_overshoot_{left,right}` with `allow_overshoot_{lower,upper}`? That way it's in the 'space' of the value that's being manipulated, and the terminology would be the same as the field names for the upper/lower bounds.

Sybren A. Stüvel requested changes 2023-05-02 11:05:13 +02:00

Sybren A. Stüvel left a comment

Just marking 'request changes' to put the PR into the right queue.

Christoph Lendenfeld added 2 commits 2023-05-04 10:28:04 +02:00

934361035c Merge branch 'main' into slider_overshoot_refactor

6a01a46b7b implement sybrens notes

Christoph Lendenfeld commented

2023-05-04 10:31:28 +02:00

use terminology of lower/upper bounds
remove float[2] as parameter and instead use 2 separate floats

* use terminology of lower/upper bounds * remove float[2] as parameter and instead use 2 separate floats

Christoph Lendenfeld requested review from Sybren A. Stüvel 2023-05-04 10:31:39 +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

Christoph Lendenfeld referenced this pull request

2023-05-04 12:18:08 +02:00

Refactor: Bidirectionality on Graph Editor Sliders #107173

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

Sybren A. Stüvel left a comment

👍

:+1:

source/blender/editors/include/ED_util.h Outdated

						
				@ -99,0 +97,4 @@

				/* One bool value for each side of the slider. Allows to allow overshoot only on one side. */

				void ED_slider_allow_overshoot_set(struct tSlider *slider, bool left, bool right);

				void ED_slider_factor_bounds_set(struct tSlider *slider, float lower_bound, float upper_bound);

Sybren A. Stüvel commented

2023-05-05 12:43:17 +02:00

I think a comment here would be in order, something like:

/**
 * Set the bounds for the slider, which are applied until the user enables overshooting.
 */
void ED_slider_factor_bounds_set(struct tSlider *slider, float lower_bound, float upper_bound);

Just to make it clear what the relation between 'bounds' and 'overshoot' is.

I think a comment here would be in order, something like: ``` /** * Set the bounds for the slider, which are applied until the user enables overshooting. */ void ED_slider_factor_bounds_set(struct tSlider *slider, float lower_bound, float upper_bound); ``` Just to make it clear what the relation between 'bounds' and 'overshoot' is.

👍 1

Christoph Lendenfeld added 2 commits 2023-05-05 15:35:31 +02:00

dcd0801a3e Merge branch 'main' into slider_overshoot_refactor

f13b43f068 add comment and improve comment

Christoph Lendenfeld merged commit c9b51591da into main

2023-05-05 17:22:46 +02:00

Christoph Lendenfeld deleted branch slider_overshoot_refactor

2023-05-05 17:22:47 +02:00

Christoph Lendenfeld referenced this issue from a commit

2023-05-05 17:22:55 +02:00

Refactor: Allow to explicitly set the range on the slider UI element

Christoph Lendenfeld referenced this pull request