ocornut / imgui

Dear ImGui: Bloat-free Graphical User interface for C++ with minimal dependencies
MIT License
60.64k stars 10.24k forks source link

API breaking change in 1.78: Slider/Drag final "float power = 1.0f" arg becomes flags #3361

Closed ocornut closed 4 years ago

ocornut commented 4 years ago

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 and SliderFloatXXX have a trailing float 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:

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);

bool SliderFloat(
   const char* label, float* v, float v_min, float v_max, 
   const char* format = "%.3f", float power = 1.0f);

Will become:

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", ImGuiSliderFlags flags = 0);

bool SliderFloat(
  const char* label, float* v, float v_min, float v_max, 
  const char* format = "%.3f", ImGuiSliderFlags flags = 0); 

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 the float 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;

Specifically:

A last thing is that ImGuiSliderFlags_Logarithmic do NOT behave the same as any use of power = 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:

*EDIT* EPILOGUE This has been merged, still open to feedback. Also added the following new flags for Slider and Drag:

// Flags for SliderFloat(), SliderInt() etc.
enum ImGuiSliderFlags_
{
    ImGuiSliderFlags_None                   = 0,
    ImGuiSliderFlags_ClampOnInput           = 1 << 4,       // Clamp value to min/max bounds when input manually with CTRL+Click. By default CTRL+Click allows going out of bounds.
    ImGuiSliderFlags_Logarithmic            = 1 << 5,       // Make the widget logarithmic (linear otherwise). Consider using ImGuiSliderFlags_NoRoundToFormat with this if using a format-string with small amount of digits.
    ImGuiSliderFlags_NoRoundToFormat        = 1 << 6,       // Disable rounding underlying value to match precision of the display format string (e.g. %.3f values are rounded to those 3 digits)
    ImGuiSliderFlags_NoInput                = 1 << 7        // Disable CTRL+Click or Enter key allowing to input text directly into the widget
};

You can version check with #if IMGUI_VERSION_NUM >= 17704

notnullnotvoid commented 4 years ago

Case 3 would apply not just to power == 1.0f but to all 1.0f <= power < 2.0f, wouldn't it? If the new function asserts on other truncated values, I'd expect it to assert for that range as well. Which seems to be what case 5 suggests will happen.

ShironekoBen commented 4 years ago

You're right - that's a tricky one. Since the values get truncated to integer, there's no way to distinguish between the 1.0f and 1.0f < power < 2.0f cases, sadly. So if we want to silently allow 1.0f (which seems reasonable, since that's the default), then we also have to allow 1.0f < power < 2.0f as well :-(

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 != 1.0f assert before calling the ImGuiSliderFlags overload? I haven't tested this, but I would imagine that way calls with power=1 will go to the ImGuiSliderFlags overload and hit the fallback path there, whilst calls with power=1.0f will go through the float overload and either pass through (in the ==1.0f case) or assert there.

thedmd commented 4 years ago

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. In other words this will also prevent conversions from double to ImGuiSliderFlags.

Solution will not cover older compilers and will require some ifdef magic to detect the feature, but should cover all other bases.

Cogno-Marco commented 4 years ago

Isn't 1.0f <= power < 2.0f actually covered by case 5? It's in one of the examples (1.8f specifically), so you would get the warning anyway + the bit check, right? @ShironekoBen @notnullnotvoid

ShironekoBen commented 4 years ago

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). This is the case where we can't distinguish between 1.0f and 1.8f from within the function, because by the time we get it the float->int truncation has already happened, so we don't really have much option but to rely on the user seeing the warning (or the compiler being new enough to treat that as an outright error). However, any other value between 0 and 15 will trigger the lower-bits-set check and thus assert... it's just 1.0f-1.9999f that is problematic.

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?

thedmd commented 4 years ago

@ShironekoBen Make sense.

Deleting function or adding an explicit overload for this case (which is done in if IMGUI_DISABLE_OBSOLETE_FUNCTIONS is not defined) will always sink float arguments (https://godbolt.org/z/Wecnfb). double is convertible to float and int which result in ambiguous call and fail with compilation error (https://godbolt.org/z/7M5999).

As for int type system will not help because ImGuiDragFlags is an alias for an int. Calls with 1 or 2 will still be redirected to here. This is where run-time assert may help. enum class will do the trick also but will require templates (or ton of boilerplate code) and is even bigger compatibility break.

ocornut commented 4 years ago

You're right - that's a tricky one. Since the values get truncated to integer, there's no way to distinguish between the 1.0f and 1.0f < power < 2.0f cases, sadly. So if we want to silently allow 1.0f (which seems reasonable, since that's the default), then we also have to allow 1.0f < power < 2.0f as well :-(

That's right, some extra thoughts:

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 != 1.0f assert before calling the ImGuiSliderFlags overload?

This is essentially what we are doing already (Case 4) ?

For C++11 there is an option to mark function as deleted: Solution will not cover older compilers and will require some ifdef magic to detect the feature, but should cover all other bases. This will generate explicit build error when used, allowing version with flag to exists.

So gut feeling is that it's not worth trying, but if you may to give it a try.

Isn't 1.0f <= power < 2.0f actually covered by case 5? It's in one of the examples (1.8f specifically), so you would get the warning anyway + the bit check, right?

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 power feature. Ben's initial branch did so. I thought removing it would both simplify our internal code and allow for a neater path of getting rid of those duplicates..)

ShironekoBen commented 4 years ago

This is essentially what we are doing already (Case 4) ?

..yes, indeed it is. I'd forgotten we were discussing the IMGUI_DISABLE_OBSOLETE_FUNCTIONS case there where the user has explicitly asked to remove that helper!

ocornut commented 4 years ago

Rebase this and force-pushed with:

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

Remaining todo:

ocornut commented 4 years ago

Also took advantage of newly added flags to introduce a ImGuiDragFlags_ClampOnInput / ImGuiSliderFlags_ClampOnInput flag which solve the problem discussed in (#1829, #3209, #946, #413)

Also now added ImGuiDragFlags_NoRoundToFormat / ImGuiSliderFlags_NoRoundToFormat in that branch (for #642) for good measure. Remaining todo list is in post above. Testing always welcome.

ShironekoBen commented 4 years ago

FWIW I'm looking at the deadzone/reverse drag/keyboard navigation issues, so hopefully should have something on those in the near future.

ShironekoBen commented 4 years ago

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 ImGui::DragBehaviorT():

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?

ocornut commented 4 years ago

Thanks!

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!

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 ImGuiDragFlags_NoRoundToFormat flag.

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 ImGui::DragBehaviorT(): [....] 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?

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 ImGuiDragFlags_Readonly flag (currently in imgui_internal because we'll want to promote a more global read-only item flag later). It is technically a minor api breaking change but I don't think anyone relied on that (it wasn't even demoed), will add docs about it when we add support for reverse slider min>max. Commit: eedc4a1

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.

ocornut commented 4 years ago

I think this is already good as-is, we mostly need to write some more thorough specs for regression tests. But it's going to be difficult to merge safely without any feedback from 1) people trying it on large codebase and 2) feedback from users language-wrappers (cc. @mellinoe @sonoro1234 sorry to ping again but cimgui / lua / c# are most at risk of issues so your feedback would be invaluable there).

ocornut commented 4 years ago

This is now merged in master.

torkeldanielsson commented 4 years ago

I ran into an issue with ImGuiSliderFlags_ClampOnInput and using min=max=0.0f to signal no limit of range: the ctrl-click input value is clamped to 0.0f. (Dragging the value works as expected.)

Looking briefly at the code I see that DragBehaviorT has "const bool is_clamped = (v_min < v_max);" which seems to be what triggers the non-clamp in the normal drag flow. For the input I found no similar check in "ClampBehaviorT". I can make a PR adding a check similar to is_clamped to ClampBehaviorT, but I get a feeling there might be some details in how these functions interact with other things so let me know if you want that :)

ocornut commented 4 years ago

@torkeldanielsson Thanks for reporting this, fixed by 4c201994d421089493a7a996978e8239ad619a20.

(I didn't add the check in ClampBehaviorT() because that (v_min < v_max) check in DragBehavior (which could be a v_min == v_max) has been pretty inconsistent with other widgets and is mostly a legacy thing that was initially designed to facilitate default value to DragFloat(). Using -FLT_MAX, +FLT_MAX as default bound would be preferable down the line.)

ShironekoBen commented 4 years ago

I've added a stab at allowing the case of a drag widget with no explicit bounds but logarithmic mode enabled. Conceptually what it does is calculate the magnitude of the current value, and then generate bounds around it based on that, so the drag is consistent until you cross a magnitude boundary and then it rescales appropriately.

It's by no means perfect - in particular, if your initial value is (say) 0.01f and you then drag such that it becomes 1.0, getting back to 0.01f again is hard because you've stepped "up" a couple of orders of magnitude. There's also an annoying interaction with the format-based precision clamping where (for example) an initial value of 0.001 cannot go above 0.005 with the default three-digit format string because it ends up generating a range that post-clamping can't quite reach the next transition point.

So I have mixed feelings about this - on one hand, it does work reasonably well for normal-ish values. On the other there are enough gotchas to make it a bit of a trap for the unwary, and I'm not convinced there is a good way to solve most of them without contextual information (ironically about the worst possible case is probably one of the most common - namely "logarithmic mode is on but the value is zero so we have absolutely no idea what scale the user really wants"). Thus I do wonder if we wouldn't be better simply saying "if you turn on the logarithmic flag then you have to provide bounds" so that people don't end up wondering why they get odd results.

(oh, and on a code cleanliness front, there's an issue right where retrieving the min/max bounds for non-float types generates warnings about constant truncation - this is easy enough to fix with some overload/template trickery but I didn't think it was worth doing until we were sure this was the right way to be going generally).

sonoro1234 commented 4 years ago

@ocornut Sorry, I was not aware that my comments were requested.

cimgui always uses IMGUI_DISABLE_OBSOLETE_FUNCTIONS to avoid handling so many cases. From the point of view of cimgui generation (being programatic) there is no problem at all. From the Lua side of things there is nothing special appart from what is happening in C++: Just the burden of having to update all uses of the power argument when it was present in users code (No changes if it was not used as default value is automatically setted in LuaJIT binding) C direct use, not having default arguments will need to change all uses of this functions (But who is using C alone?) In binding generation depending on cimgui it should not appear any issues as long as the binder makes use of the default values provided in json

RandyKnapp commented 2 years ago

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 1 << 4? Are there some shared flags that use those empty flag bits somewhere else?

ocornut commented 2 years ago

See the comment in that enum:

ImGuiSliderFlags_InvalidMask_ = 0x7000000F    // [Internal] We treat using those bits as being potentially a 'float power' argument from the previous API that has got miscast to this enum, and will trigger an assert if needed.

Not using those bits for valid flags allows to assert for a few invalid uses of API and ABI.

Triang3l commented 2 years ago

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).

ocornut commented 2 years ago

Unfortunately this happening 2 years ago and yours being the first push-back reaction to it suggest it was good tradeoff :( Those are crowded function signatures which really needed the simplification, and the previous maths was flaky at best.

For a debug control of this kind it seems like a linear slider wouldn't be the end of the world? Another workaround would be to manually display certain position e.g. the halfway position visually over or around the frame which would reduce pressure for needing this change.

If you believe there is value in adding a variant that can be expressed with a single flag, I'm open to it. I'm also very open to adding a callback if added at the lower DragBehavior()/SliderBehavior() level (so potentially you can wrap locally as e.g. SliderFloatHyberbolic()). One advantage is it would nicely isolate the existing _Logarithmic code while allowing more, but I suspect it's not an easy refactor.

Triang3l commented 2 years ago

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 (, 1.0f) long parameter that makes the function signature look subjectively crowded is a very questionable reason for removing meaningful functionality from a library used in a large number of applications and breaking wrappers, especially in a way that involves building a wall consisting of 5 cases for those who relied on this behavior, complex interaction with IMGUI_DISABLE_OBSOLETE_FUNCTIONS, and asking for help (taking into account that it's possibly not a very common situation when app developers actively monitor the issue trackers of all their dependencies) — while not providing an alternative that covers the existing usage.

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.

ocornut commented 2 years ago

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 ImGuiSliderFlags_NoInput to disable manual input and pass a pre-formatted value into the format (meaning the format won't include %.f).

Triang3l commented 1 year ago

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 ScaleRatioFromValueT). In this case, changing the base would apparently have no effect, as with the formula for changing the base of a logarithm:

$$ log_b x = {log_a x \over log_a b} $$

a ratio of two logarithms:

$$ log_a x \over log_a y $$

becomes:

$$ ({log_a x \over log_a b}) \over ({log_a y \over log_a b}) $$

or:

$$ {log_a x \over \cancel {log_a b}} {\cancel {log_a b} \over log_a y} $$

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.

ocornut commented 2 weeks ago

I ran into an issue with ImGuiSliderFlags_ClampOnInput and using min=max=0.0f to signal no limit of range: the ctrl-click input value is clamped to 0.0f. (Dragging the value works as expected.)

Looking briefly at the code I see that DragBehaviorT has "const bool is_clamped = (v_min < v_max);" which seems to be what triggers the non-clamp in the normal drag flow. For the input I found no similar check in "ClampBehaviorT". I can make a PR adding a check similar to is_clamped to ClampBehaviorT, but I get a feeling there might be some details in how these functions interact with other things so let me know if you want that :)

Answering to this specific message for visibility. b3c8747 (1.91.3 WIP) changed behaviors slightly.

TL;DR;