-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
API breaking change in 1.78: Slider/Drag final "float power = 1.0f" arg becomes flags #3361
Comments
Case 3 would apply not just to |
You're right - that's a tricky one. Since the values get truncated to integer, there's no way to distinguish between the I'm pretty sure that technically that could be resolved with template magic if we really wanted to, but template magic is exceptionally good at fixing a simple well-understood problem whilst introducing a cluster of exceptionally complex poorly-understood side-effects, so I'd be very nervous about that route. I guess another option would be to keep the float overload initially, as something like: bool SliderFloat(const char* _depreciated, float* _function, float _please, float _do, const char* _not = "%.3f", float _use = 1.0f); ...and have that internally do a |
For C++11 there is an option to mark function as deleted: bool DragFloat(
const char* label, float* v, float v_speed = 1.0f, float v_min = 0.0f, float v_max = 0.0f,
const char* format = "%.3f", float power = 1.0f) = delete; Support:
This will generate explicit build error when used, allowing version with flag to exists. Solution will not cover older compilers and will require some |
Isn't |
So I think there are a couple of distinct cases here that each have separate issues/mitigations. The first one is the obvious case of "function is declared with an enum but called with a float". On a modern compiler this will generate a warning (and potentially even an error - C2664 in the case of MSVC), and then will do an implicit cast of the float value to the underlying type of the enum, thus turning 1.8f into 1 (int). The other is the case where the function was declared as taking a float but then linked against a version that takes an enum (either as a result of version shenanigans in C land, or some sort of mix-up when using bindings for another language). This most likely won't produce any errors/warnings (unless the linker is really on the ball), but will have the effect of turning the binary representation of the float into an integer, hence turning 1.0f into 0x3f800000. This is what the top-bits-set check is testing for. At least, that's how the logic works in my head... does that make sense to anyone else? |
@ShironekoBen Make sense. Deleting function or adding an explicit overload for this case (which is done in if As for |
That's right, some extra thoughts:
This is essentially what we are doing already (Case 4) ?
So gut feeling is that it's not worth trying, but if you may to give it a try.
We'd get the conversion warning (IF enabled) but bit-check won't work as we'd silently allow 1. Honestly I feel the situation isn't so bad as-is but would be interested if anyone tries it in their codebase. (Bonus clarification: we could in theory decide to preserve all the |
..yes, indeed it is. I'd forgotten we were discussing the |
Rebase this and force-pushed with:
Branch over master: Branch over docking: Remaining todo:
|
Also now added |
FWIW I'm looking at the deadzone/reverse drag/keyboard navigation issues, so hopefully should have something on those in the near future. |
I've made some changes that add a deadzone around 0.0f, and nav delta accumulation so that sliders no longer get "stuck". I'm not massively happy with the latter as you still get the effect that you can sometimes press left/right on the keyboard/pad once and the slider doesn't noticeably move - you have to press it enough times to build up sufficient delta to get past whatever the next value "step" is. I briefly played with changing nav-based sliders to actually track the grab position internally, so it would be possible to move the grab by an increment that didn't actually change the underlying value (thus giving visual feedback in this situation), but that kinda felt wrong because then conversely you get the impression that you've changed something even if you haven't :-( (plus it causes issues with navigation on sliders where something else is altering the value independently from ImGui) The "nicest" solution would probably be to push the delta up to the next value that causes an actual change, but with the involvement of exponential values, various different underlying data precisions and the display-format-based snapping it feels... non-trivial to calculate that in any manner other than by brute force. So if we wanted to go down that road the least-worst option would probably be to effectively do "if the user pressed left/right and nothing useful happened, internally repeat the input until something does" (i.e. simulate what the user is going to do anyway!), but for the moment I've left it be to see what anyone else thinks! As for "Logarithmic reverse drags don't work"... looking at the code again, that seems to be a deliberate feature of drag sliders - specifically this in const bool is_locked = (v_min > v_max);
if (is_locked)
return false; Is this a documented/used feature we should be preserving, or would it make more sense for drags to work like regular sliders and treat min>max as meaning that the axis should be flipped? |
Thanks!
I think the situation may be good enough as is: most logarithmic drags/sliders would be using higher precision value than %.3f and then it's rarely an issue. I think I only changed your demo from %f to %.3f because I wanted to test the effectiveness of the new
Right. This was very vaguely advertised as a feature in 1.73 for one piece of code that eventually didn't need it. I have now replaced it with a One way to implement it might possibly be to support four directions (default being left-to-right), and flip the direction and min/max if min>max. |
I think this is already good as-is, we mostly need to write some more thorough specs for regression tests. |
This is now merged in master. |
Imgui 1.78 broke the float from SliderFloat ocornut/imgui#3361
Imgui 1.78 broke the float from SliderFloat ocornut/imgui#3361
Imgui 1.78 broke the float from SliderFloat ocornut/imgui#3361
Imgui 1.78 broke the float from SliderFloat ocornut/imgui#3361
Imgui 1.78 broke the float from SliderFloat ocornut/imgui#3361
Imgui 1.78 broke the float from SliderFloat ocornut/imgui#3361
Imgui 1.78 broke the float from SliderFloat ocornut/imgui#3361
Hey, I'm adding a custom drag/slider flag to my version of ImGui, and I'm just curious, why did you start the flags at |
See the comment in that enum:
Not using those bits for valid flags allows to assert for a few invalid uses of API and ABI. |
…lete in 1.77 and 1.78 (June 2020): (#3361) - DragScalar(), DragScalarN(), DragFloat(), DragFloat2(), DragFloat3(), DragFloat4() with signature ending with (..., float power = 1.0f) - SliderScalar(), SliderScalarN(), SliderFloat(), SliderFloat2(), SliderFloat3(), SliderFloat4() with signatures ending with (..., float power = 1.0f) - BeginPopupContextWindow(const char*, ImGuiMouseButton, bool)
This is pretty inconvenient, we have a variable that goes specifically to exp2(-value) for AMD FidelityFX Super Resolution smoothness configuration, and the power 2.0 gave us the exact distribution we wanted (the slider being halfway corresponding to the visually average sharpness, even though that was only 0.5 stops out of 2 stops). Now we are forced either to use a mathematical function that we have less control over than previously (it's on/off now, not a configurable value) and that gives us something completely different (the value of 0.2 stops out of 2 stops is displayed at around 2/3 on the slider even though its visual effect is much closer to 0 than to 2), or to introduce hacks such as making the slider linear, but displaying the value in stops outside of it rather than on top of it and handling text input manually — or we get an assertion failure in the new version. To me, this forced simplification resulting in degradation of functionality feels more like causing more harm than good, both options can be exposed in the API if the new flag provides something that couldn't be represented with the original power parameter (an even better solution possibly would be a callback for a user-provided remapping function, so distributions like logarithmic and exponential (with a user-specified base), hyperbolic, Gaussian could be used trivially). |
Unfortunately this happening 2 years ago and yours being the first push-back reaction to it suggest it was good tradeoff :( For a debug control of this kind it seems like a linear slider wouldn't be the end of the world? If you believe there is value in adding a variant that can be expressed with a single flag, I'm open to it. |
It's not a debug control, it's in the user-faced UI in our application (the Xenia Xbox 360 emulator). Though displaying the number is not clearly required (the purpose of that number is mostly consistency with FSR setup in the industry overall — so the value is in the same units as provided to the setup function in AMD's FSR library API, plus text output and input for easier sharing of preferred values for different emulated games between users), but with the power argument it just worked — the slider roughly (but quite closely) represented the visual effect of the values (major change between 0 and 1, still significant (unlike with the logarithmic flag) but not as much between 1 and 2, values larger are not exposed at all through the slider because they have mostly no effect), while the variable tweaked using it directly represented the needed value. But yes, I understand that this is a very specific use case for one certain mathematical function, and neither the power argument nor the logarithmic flag truly covers the variety of the scenarios that may be encountered in real life, some way of letting the application set up the curve arbitrarily may be necessary here (possibly with Begin/End and some API functions for calling between them for retrieving the latest visual position and setting the desired actual value, so that function pointers which may be complex to pass to another language through a wrapper and require some way of capturing the relevant state don't have to be used — though going the other way around, from actual to linear visual values, would be complicated, possibly requiring two callbacks). In my personal opinion though just having one six characters ( Another way how this functionality may be exposed though while still not including the power value in the signature is providing a flag for power distribution, and having a function setting the power for the next slider. Though if the old and the new curve correspond to the same mathematical function, a new flag would not be necessary — just a logarithm base setter would be sufficient. |
Keep in mind those "six characters" had to be explicitly specified for every single calls site from language not supporting default parameters, while only being used by a tiny fraction of users and call sites. Adding flags to the mix meant that this burden would extend to any users of flags in C++. And again you are the first push-back reaction in two years since this was done. We spend immense amount of energy and time supporting backward compatibility and occasionally breaking things (and no breaking is done casually). I probably personally spent 1000+ hours working on various aspects of backward/forward compatibility so those are not exactly light-hearted changes, but we have to move forward. We support more 1000+ features. A weird but functionally workaround is to use |
Yes, I understand that especially with two mutually exclusive options (combined with the longer function prototype) — among which the new one is conceptually more suitable for its purpose — it will be pretty difficult to maintain this widget. However, it's still interesting what additional control can be provided to configure the steepness of the slider curve, while not overcomplicating the interface with arbitrary functions, that would have issues with invertibility, with snapping that already has to be done for the logarithmic slider. Do the mathematical functions used in the logarithmic slider conceptually provide anything that would make that possible? I was thinking about the potential solutions, such as changing the base of the logarithm, however, it seems like the slider only uses logarithms divided by other logarithms (at least that's what I could find in a ratio of two logarithms: becomes: or: therefore the change of the base is simply cancelled out. Would, for instance, denormalizing the range — remapping the slider to just a part (not necessarily smaller — to make the growth slower — but may be wider as well to make the curve even steeper) of the function currently used — be conceptually correct here and configurable enough? Though I think this specific idea would not work as it doesn't seem to be possible to implement the edge case this way (primarily for testing purposes) — a linear function, with the power of 1, implemented in terms of a logarithmic curve. |
Answering to this specific message for visibility.
TL;DR;
|
This is a post to discuss a upcoming/possible API breaking change we devised with @ShironekoBen that may have larger side-effects on codebase than the usual breaking changes.
WHY
Both
DragFloatXXX
andSliderFloatXXX
have a trailingfloat power = 1.0f
parameter designed to make editing non-linear (e.g. have more precision in the small values than the large ones). Various people have rightly commented and discussed on the fact that my maths behind that parameter has always been a little peculiar and suggested using logarithmic widgets instead (#642, #1316, #1823). I expected most people who had an incentive of editing with more precision just used power=2.0f or 3.0f and stuck with "whatever it did" which sort of did the job.Aside from that, there are many requests which would benefits from exposing flags to the outer-most public layer of the Drag and Slider functions. e.g. Enforcing or disabling clamping when using CTRL+Click (#3209, #1829), Vertical dragging behavior, Request underlying value to only be written at end of edit (#701), Mouse wrap-around (#228), data type wrap-around options, disable text input etc..
The bottleneck for adding those flags in the outer-most layer of the public API has always been the function signature are long and I sort of expected the trailing
float power=1.0f
parameter should go so I've been hesitant with adding yet another optional parameter...SUGGESTED CHANGE
We would like to obsolete the trailing
float power = 1.0f
and REPLACE it with new set of flags e.g.ImGuiSliderFlags flags = 0
. Specifically,ImGuiSliderFlags_Logarithmic
would provide a similar-but-difference experience as toying with the power parameter. Addition of flags means many other requested features can more easily be added.So essentially this family of functions:
Will become:
It's a rather simple change but in term of breaking API compatibility is quite unusual.
In addition for the most-common functions we would leave a redirection entry point (unless
IMGUI_DISABLE_OBSOLETE_FUNCTIONS
is defined at compile-time) with thefloat power
argument but without a default, so omitting the last parameter would call the new functions.WHAT WOULD IT BREAK AND HOW WOULD WE HANDLE IT?
TL;DR;
power > 1.0f
should likely be replaced to use the_Logarithmic
flag instead.You may be wondering: a single flag is not going to be as expressive as a float, but the way
power
was handled seemingly it unpractical for people to actually tweak it wisely.Specifically:
Case 1: A call to
DragFloat()
andSliderFloat()
omitting the last parameter. I expect this to be the most common in C++ codebase. For that case, recompiling would just call the new function with no issue.Case 2: A call is made with
power = 1.0f
and withoutIMGUI_DISABLE_OBSOLETE_FUNCTIONS
: will call the redirection function. It will check that the value if 1.0f and call the new function.Case 3: A call is made with
power = 1.0f
and withIMGUI_DISABLE_OBSOLETE_FUNCTIONS
or after we decide to remove the redirection functions: the new function will be called with 1.0f cast to an integerImGuiSliderFlags flags = 1
.With some - but not all - compiler settings, the compiler will emit a warning:
warning C4244: 'argument' : conversion from 'float' to 'ImGuiSliderFlags', possible loss of data
Which is hepful to detect the issue.
The new function will safely accept the 1 at runtime and ignore it as it is equivalent to the default flags.
Case 4: A call is made with
power != 1.0f
and withoutIMGUI_DISABLE_OBSOLETE_FUNCTIONS
: will call the redirection function. The redirection function will trigger an assert:IM_ASSERT(power == 1.0f && "Call Slider function with ImGuiSliderFlags_Logarithmic instead of using the old 'float power' function!");
As a weird courtesy we'll also set the
ImGuiSliderFlags_Logarithmic
flag in this code path, so code compiled without asserts will somehow behave (tempted to remove this courtesy as it may lead user into a false sense that things are ok)Case 5: A call is made with
power != 1.0f
and withIMGUI_DISABLE_OBSOLETE_FUNCTIONS
or after we decide to remove the redirection functions: the new function will be called with float power casted to the integer flag. Sopower = (float)1.8f
will be cast to(int)1
,(float)3.1f
will be cast to(int)3
etc.With some - but not all - compiler settings, the compiler will emit a warning:
warning C4244: 'argument' : conversion from 'float' to 'ImGuiSliderFlags', possible loss of data
Which is hepful to detect the issue.
Here's the twist: the new function will assert if any of the lower 4-bit or upper 4-bit of the integer flags are set.
The lower 4-bits check means that any value from 0 to 15 (excluding 1) will assert. So floating point value from 0.0f to 16.0f (exclusive) will assert, and they constitute a large enough reasonable range of values that could have been used correctly as the
float power
parameter.The upper 4-bits check is a little overkill but means that most float >1.0f value which somehow made its way and got reinterpreted as a integer (unlikely in C++, as function signature change will require a rebuild, but may be more possible in high-level language) will be detected and assert. (e.g. 1.0f is stored as 0x3f800000). https://www.h-schmidt.net/FloatConverter/IEEE754.html
A last thing is that
ImGuiSliderFlags_Logarithmic
do NOT behave the same as any use ofpower = 2.0f
, but in spirit it provide a similar functionality.HELP NEEDED
I think I got cases covered but since I'm incredibly paranoid about breaking people code in mysterious ways..
I would like people to double-check my reasoning:
And then TRY those changes in your codebase so you can help us validate the reasoning and provide feedback.
You will ideally want to evaluate if the compiler/asserts are helping you, and then do additional searches/grep in your code to get a feel if you could have missed something.
Branch over master:
https://github.com/ocornut/imgui/tree/features/logarithmic_sliders
Branch over docking:
https://github.com/ocornut/imgui/tree/features/logarithmic_sliders_docking
POTENTIAL TRANSITION PLAN
If it works, the change will come with:
#define IMGUI_VERSION_NUM 17703
will be increased so in the case of code that needs to be shared across versions this may be used to use#ifdef
blocks.*EDIT* EPILOGUE
This has been merged, still open to feedback.
Also added the following new flags for Slider and Drag:
You can version check with
#if IMGUI_VERSION_NUM >= 17704
The text was updated successfully, but these errors were encountered: