SliderInt/Float Doesn't Handle Max == Min Correctly.

skl131313 commented

2016-11-24 01:21:01 +00:00

(Migrated from github.com)

The SliderInt implementation is just a SliderFloat, so the problem exists there as well. If you set min and max to the same value, then the outputted value will be outside the bounds of "min" and "max" when the user attempts to move the slider. It appears it might be because of a division by zero (exists in a few other places as well). Which causes the result to be NaN, which is then converted to an integer in SliderInt.

The SliderInt implementation is just a SliderFloat, so the problem exists there as well. If you set min and max to the same value, then the outputted value will be outside the bounds of "min" and "max" when the user attempts to move the slider. [It appears it might be because of a division by zero (exists in a few other places as well)](https://github.com/ocornut/imgui/blob/master/imgui.cpp#L6544). Which causes the result to be NaN, which is then converted to an integer in SliderInt.

ocornut commented

2016-11-27 14:14:52 +00:00

(Migrated from github.com)

It is illegal to pass a null range to SliderInt / SliderFloat. What would you expect it to happens appart from erroring/asserting?

Early 1.0-era version of imgui supported it by disabling the actual slider behavior but that code-path got removed while cleaning up.

It is illegal to pass a null range to SliderInt / SliderFloat. What would you expect it to happens appart from erroring/asserting? Early 1.0-era version of imgui supported it by disabling the actual slider behavior but that code-path got removed while cleaning up.

ocornut commented

2016-11-27 16:43:47 +00:00

(Migrated from github.com)

@sprinkle131313 Could you provide a repro for the outputted value will be outside the bounds of "min" and "max". Afaik, the code you linked only affects the position of the drawn sliders but values assignments looks right to me:
new_value = ImLerp(v_min, v_max, clicked_t);

@sprinkle131313 Could you provide a repro for ` the outputted value will be outside the bounds of "min" and "max" `. Afaik, the code you linked only affects the position of the drawn sliders but values assignments looks right to me: ```new_value = ImLerp(v_min, v_max, clicked_t);```

skl131313 commented

2016-11-28 09:52:22 +00:00

(Migrated from github.com)

@ocornut
Not sure if it is considered a null range. The bounds is inclusive for the range. I'm using the slider as the index for an array. So to allow for it to exist for an array of size 1, min and max have to be equal. Using the range [0, 1] would allow the values 0 and 1 to be valid. But only 0 is actually valid. On the other hand [0, 1) would be the range I need and in that case [0, 0) is a null range. But [0, 0] has a value.

Ah I linked to the master and it changed before you looked at the link. Here it does max - min, which can lead to division by zero.

To reproduce it, just create a SliderInt where min and max are 0 or equal, then in the UI click the slider and drag left/right quickly trying to change the value.

@ocornut Not sure if it is considered a null range. The bounds is inclusive for the range. I'm using the slider as the index for an array. So to allow for it to exist for an array of size 1, min and max have to be equal. Using the range [0, 1] would allow the values 0 and 1 to be valid. But only 0 is actually valid. On the other hand [0, 1) would be the range I need and in that case [0, 0) is a null range. But [0, 0] has a value. Ah I linked to the master and it changed before you looked at the link. [Here](https://github.com/ocornut/imgui/blob/v1.49/imgui.cpp#L6459) it does max - min, which can lead to division by zero. To reproduce it, just create a SliderInt where min and max are 0 or equal, then in the UI click the slider and drag left/right quickly trying to change the value.

👍 1

ocornut commented

2016-11-28 10:06:56 +00:00

(Migrated from github.com)

You are right that it makes sense namely for integer.

That code you linked doesn't affect the value itself, which is why I am reacting to your "the outputted value" comment. I guess because my test bed doesn't throw an exception on divide-by-zero I don't see that affect and the grab is invisible (which may be desirable).

I made the display works (and it makes more senses for integers).
And reverted a change from yesterday that prevented the value from being written if with a v_min==v_max range. It does now write to the write when interacting with the slider, which is consistent with other widgets behaviors. So if your range is [1,1] and your value is 2 it'll stay 2 until you click.

You are right that it makes sense namely for integer. That code you linked doesn't affect the value itself, which is why I am reacting to your "the outputted value" comment. I guess because my test bed doesn't throw an exception on divide-by-zero I don't see that affect and the grab is invisible (which may be desirable). I made the display works (and it makes more senses for integers). And reverted a change from yesterday that prevented the value from being written if with a v_min==v_max range. It does now write to the write when interacting with the slider, which is consistent with other widgets behaviors. So if your range is [1,1] and your value is 2 it'll stay 2 until you click.

ocornut commented

2016-11-28 10:07:50 +00:00

(Migrated from github.com)

Confusion lies in the fact that I don't think " then the outputted value will be outside the bounds of "min" and "max" when the user attempts to move the slider " was ever true.

skl131313 commented

2016-11-28 10:33:39 +00:00

(Migrated from github.com)

Yah your right, my mistake. A division of zero was happening but I didn't look closely enough to find the right place.

If max == min, then this value will be zero

Which leads to a division by zero here. Causing normalized_pos to be NaN. Which ends up propagating to new_value. It then compares the old value with NaN, which is true as it can't be equal.

It doesn't seem to happen all the time, it might be under a certain condition. Clicking rapidly and moving slowly seems to cause as well as click-holding and moving the mouse rapidly left/right.

Yah your right, my mistake. A division of zero was happening but I didn't look closely enough to find the right place. [If max == min, then this value will be zero](https://github.com/ocornut/imgui/blob/v1.49/imgui.cpp#L6367) [Which leads to a division by zero here](https://github.com/ocornut/imgui/blob/v1.49/imgui.cpp#L6393). Causing ```normalized_pos``` to be NaN. Which ends up propagating to ```new_value```. It then compares the old value with NaN, which is true as it can't be equal. It doesn't seem to happen all the time, it might be under a certain condition. Clicking rapidly and moving slowly seems to cause as well as click-holding and moving the mouse rapidly left/right.

ocornut commented

2016-11-28 19:43:25 +00:00

(Migrated from github.com)

Yah your right, my mistake. A division of zero was happening but I didn't look closely enough to find the right place.
If max == min, then this value will be zero

I don't understand your statement, if you look at the code it doesn't appear to read like what you say.

(There is one bug, which I'm fixing now, happening for integers if v_min is > v_max if (v_min - v_max)==-1, but that feature was introduced yesterday.)

> Yah your right, my mistake. A division of zero was happening but I didn't look closely enough to find the right place. > If max == min, then this value will be zero I don't understand your statement, if you look at the code it doesn't appear to read like what you say. (There is one bug, which I'm fixing now, happening for integers if v_min is > v_max if (v_min - v_max)==-1, but that feature was introduced yesterday.)

ocornut commented

2016-11-28 19:44:49 +00:00

(Migrated from github.com)

I don't think there is any issue left but as you imply so let me know if there's anything I can check.

skl131313 commented

2016-11-29 08:13:21 +00:00

(Migrated from github.com)

Not sure what is happening then but it is definitely being set to NaN.

Not sure what is happening then but it is definitely being set to NaN. ![debug](https://cloud.githubusercontent.com/assets/8701667/20701566/c6325b04-b5e1-11e6-84aa-f175686c8f11.png)

ocornut commented

2016-11-29 08:16:59 +00:00

(Migrated from github.com)

Oh but you are still using the old code. I made commits to fix the issues.

skl131313 commented

2016-11-29 08:37:56 +00:00

(Migrated from github.com)

Just tried the master branch code, the bug still seems to exist. Ok that might explain why it seems to happen randomly. It's when mouse_abs_pos is equal to slider_usable_pos_min. That then becomes zero divided by zero, when slider_usable_sz is also zero.

ocornut/imgui@ca9a918535/imgui.cpp (L6519)

Just tried the master branch code, the bug still seems to exist. Ok that might explain why it seems to happen randomly. It's when ```mouse_abs_pos``` is equal to ```slider_usable_pos_min```. That then becomes zero divided by zero, when ```slider_usable_sz``` is also zero. https://github.com/ocornut/imgui/blob/ca9a91853584c0095be233a19a7b3705914457bc/imgui.cpp#L6519

ocornut commented

2016-11-29 20:03:35 +00:00

(Migrated from github.com)

Neither mouse_abs_pos or slider_usable_pos_min or the values deriving from them (such as clicked_t) are used as denominator in a division.

slider_usable_sz being zero however is a problem.
I think I understand your problem.
You may have set the style value of style.GrabMinSize to a rather large value, or at least larger than your style.WindowMinSize and you are getting into a situation with a very small window where (slider_sz < style.GrabMinSize) which indeed would lead to a division per zero.

Could you share your style information and what your windows look like?

Neither `mouse_abs_pos` or `slider_usable_pos_min` or the values deriving from them (such as `clicked_t`) are used as denominator in a division. `slider_usable_sz` being zero however is a problem. I think I understand your problem. You may have set the style value of `style.GrabMinSize` to a rather large value, or at least larger than your `style.WindowMinSize` and you are getting into a situation with a very small window where (slider_sz < style.GrabMinSize) which indeed would lead to a division per zero. Could you share your style information and what your windows look like?

skl131313 commented

2016-11-29 21:20:45 +00:00

(Migrated from github.com)

Sorry my confusion was thinking that division by zero caused a NaN to occur, but division by zero just causes the value to become infinite, which clamping solves for that issue. The Problem occurs with a NaN which happens with a division of zero by zero (0.0f / 0,0f).

I am using all default values from the example, so GrabMinSize is the default.

slider_usable_sz is zero because only a lower limit is defined for the slider. Which is style.GrabMinSize. There is no upper limit for the slider grabber size, other than the size of the entire slider. This is why it becomes zero, because there is only 1 valid value, it takes up the entire slider.

Sorry my confusion was thinking that division by zero caused a NaN to occur, but division by zero just causes the value to become infinite, which clamping solves for that issue. The Problem occurs with a NaN which happens with a division of zero by zero (0.0f / 0,0f). I am using all default values from the example, so GrabMinSize is the default. slider_usable_sz is zero because only a lower limit is defined for the slider. Which is ```style.GrabMinSize```. There is no upper limit for the slider grabber size, other than the size of the entire slider. This is why it becomes zero, because there is only 1 valid value, it takes up the entire slider. ![bug](https://cloud.githubusercontent.com/assets/8701667/20729130/c42e9a58-b64e-11e6-9b2c-5340d96a0af9.png)

ocornut commented

2016-11-29 21:30:57 +00:00

(Migrated from github.com)

Ah that makes sense, thanks. It looks like it solved itself (and some other edges cases) with the previous few commits.

Now although slider_usable_sz will still be zero (that is correct) we will always set clicked_t to == 0.0 as per the commit pushed an hour ago. So the slider grab will light up but moving will always write the initial value.

Looks like this is a solved case? Sorry it took that long!

Ah that makes sense, thanks. It looks like it solved itself (and some other edges cases) with the previous few commits. Now although `slider_usable_sz` will still be zero (that is correct) we will always set `clicked_t` to == 0.0 as per the commit pushed an hour ago. So the slider grab will light up but moving will always write the initial value. Looks like this is a solved case? Sorry it took that long!

skl131313 commented

2016-11-29 21:39:19 +00:00

(Migrated from github.com)

Yah it's fixed now, it's no problem as long as it gets fixed eventually. Thanks for taking the time to fix it.

❤️ 1

erwincoumans commented

2021-01-31 17:53:33 +00:00

(Migrated from github.com)

I just ran into this issue, while moving the slider (mouse down) the value becomes 0, while min=2 and max=2.

                    int test=2;
                    if(ImGui::SliderInt("Test",&test,2,2,0,ImGuiSliderFlags_ClampOnInput))
                    {
                        std::cout << std::to_string(test) <<std::endl;
                    }

Clicking on the slider makes the value 0, while the lower and upper is 2.

The code responsible happens here:

//clicked_t = 0
//v_min = 2
//v_max = 2
//v_new  = 0...

if (set_new_value)
        {
            TYPE v_new = ScaleValueFromRatioT<TYPE, FLOATTYPE>(data_type, clicked_t, v_min, v_max, is_logarithmic, logarithmic_zero_epsilon, zero_deadzone_halfsize);

and then

TYPE ImGui::ScaleValueFromRatioT(ImGuiDataType data_type, float t, TYPE v_min, TYPE v_max, bool is_logarithmic, float logarithmic_zero_epsilon, float zero_deadzone_halfsize)
{
    if (v_min == v_max)
        return (TYPE)0.0f;

Can't we clamp the v_new to be in range v_min, v_max there, or return v_min or v_max, so the values stays in range while dragging?

if (v_min == v_max)
        return (TYPE)v_min;

I just ran into this issue, while moving the slider (mouse down) the value becomes 0, while min=2 and max=2. ~~~~~~~~~~~~~~~ int test=2; if(ImGui::SliderInt("Test",&test,2,2,0,ImGuiSliderFlags_ClampOnInput)) { std::cout << std::to_string(test) <<std::endl; } ~~~~~~~~~~~~~~~ Clicking on the slider makes the value 0, while the lower and upper is 2. The code responsible happens here: ~~~~~~~~~~~~~~~~ //clicked_t = 0 //v_min = 2 //v_max = 2 //v_new = 0... if (set_new_value) { TYPE v_new = ScaleValueFromRatioT<TYPE, FLOATTYPE>(data_type, clicked_t, v_min, v_max, is_logarithmic, logarithmic_zero_epsilon, zero_deadzone_halfsize); ~~~~~~~~~~~~~~~~ and then ~~~~~~~~~~~~~~ TYPE ImGui::ScaleValueFromRatioT(ImGuiDataType data_type, float t, TYPE v_min, TYPE v_max, bool is_logarithmic, float logarithmic_zero_epsilon, float zero_deadzone_halfsize) { if (v_min == v_max) return (TYPE)0.0f; ~~~~~~~~~~~~~~ Can't we clamp the v_new to be in range v_min, v_max there, or return v_min or v_max, so the values stays in range while dragging? ~~~~~~~~~~~~~~~ if (v_min == v_max) return (TYPE)v_min; ~~~~~~~~~~~~~~~

Rows
Columns

SliderInt/Float Doesn't Handle Max == Min Correctly. #919