ocornut / imgui

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

Regression? IsItemDeactivatedAfterEdit on InputScalar step buttons doesn't happen on frame writing to backing data #8149

Open olivier-gerard opened 1 week ago

olivier-gerard commented 1 week ago

Version/Branch of Dear ImGui:

Version 1.95.5

Back-ends:

imgui_impl_Vulkan.cpp + imgui_impl_GLFW.cpp

Compiler, OS:

Windows 11, Visual Studio 2022

Full config/build information:

No response

Details:

My Issue/Question:

Hi, I noted a regression from 1.91.4 using IsItemDeactivatedAfterEdit() on 'temporary' (I mean not static nor saved in global objects) values. With 1.91.4 I used to get the new value after calling IsItemDeactivatedAfterEdit(), now I always get the value before user input.

I didn't find anything on the changelog about it, and believe I have no particular configuration regarding this. Anyway just changing the version of DearImgui changes the behavior.

I provide a small example below, better than words. (I use this trick with temporary objects to resize vectors.)

Hope this is clear enough. Regards

Screenshots/Video:

No response

Minimal, Complete and Verifiable Example code:

 static int bak = 10;
 int tmp = bak;
 ImGui::InputInt("tmp", &tmp);
 if (ImGui::IsItemDeactivatedAfterEdit()) {
     int newval = tmp; //now always 10, was 9/11/user input in 1.91.4
 }
ocornut commented 1 week ago

Thank you for reporting. This came as an accidental side-effect to 9a0dff1 which altered the timing of reacting to the +/- buttons.

I actually introduced that change exactly for those +/- buttons, because it felt not right that a single click would react on mouse-up while a held button would repeat. For a repeating button of that kind it feels right than the initial mouse-down alter the value and then holding the button for a longer time alters it further.

How it affected the timing if setting ActiveId and breaking this I will investigate. Note that I'm frankly not even sure this use case for non-string value has ever been officially supported.

ocornut commented 1 week ago

It actually never correctly worked before:

int tmp = 10;
if (ImGui::InputInt("tmp", &tmp))
    IMGUI_DEBUG_LOG("%d - Edited\n", tmp);
if (ImGui::IsItemDeactivatedAfterEdit())
    IMGUI_DEBUG_LOG("%d - DeactivatedAfterEdit\n", tmp);

A single/fast click would work because initial edit was on deactivated frame:

[00253] 11 - Edited
[00253] 11 - DeactivatedAfterEdit

But a hold-to-repeat click could release on any frame:

[01328] 11 - Edited
[01331] 11 - Edited
[01334] 11 - Edited
[01337] 11 - Edited
[01338] 10 - DeactivatedAfterEdit

The new behavior is actually more consistent.

As per the API definition the current behavior is not incorrect. You may use a variable to store last value after InputInt() and use that in the IsItemDeactivatedAfterEdit() path.

(we partially/intently supported this idiom for direct InputText() calls but partly because InputText() works in a way not super consistent with other widgets, which we aim to fix. See e.g. https://github.com/ocornut/imgui/issues/8004#issuecomment-2364090416, and https://github.com/ocornut/imgui/issues/4714#issuecomment-969084179)

ocornut commented 1 week ago

(I use this trick with temporary objects to resize vectors.)

I'd be interested if you can elaborate on that, while I investigate if it seems sane to add a workaround to support this.

olivier-gerard commented 1 week ago

Thanks for your quick feedback. Yes I was only using it on single clicks. There is no problem to find an alternative by storing values if you don't plan to manage this behavior, I just wanted to highlight what I found as a regression.

A basic use case is a spline curve controlled by some drag points. Standard is having 3 control points, but you can control this number. If you change it then I have to add an item in the std::vector holding the points.

lets say:

 int vecSz = myVec.size();
 ImGui::InputInt("Items", &vecSz);
 if (ImGui::IsItemDeactivatedAfterEdit()) {
     myVec.resize(vecSz);
     ...
 }
ocornut commented 1 week ago

I poked a bit. I don't think there is a sane way to workaround this from the point of view of the library (other than relying on the previous repeat behavior). The previous behavior was accidental and as mentioned didn't work if you held the step buttons. The general paradigm of dear imgui is that you should carry the only source of truth.

I have amend the Changelog 8be0723 to resurface this better.

Note that you are free to use Button() and react on them. Here specifically using IsItemDeactivatedAfterEdit() would suggest you want to handle batches edits at once.

But if you change your code to:

 int vecSz = myVec.size();
 if (ImGui::InputInt("Items", &vecSz))
     myVec.resize(vecSz);
     ...
 }

It'll work just as well.

But I agree it's not ideal because you would ideally want immediate feedback on pressing +/- but delayed feedback when inputting a number...

ocornut commented 1 week ago

The hypothetical ImGuiInputTextFlags_NoLiveEdit feature from #701 would solve since, then you can use the return value from InputInt().

We did some simplification work on InputText() recently so I am going to reevaluate if _NoLiveEdit is easier to implement now.

olivier-gerard commented 1 week ago

Yes, sure that i should hold the data, but in this case it would mean adding a variable per vector, which I find not very nice. Directly picking the return of InputInt works, but in other cases than the spline the vector represents configuration which is sent to a hardware, so I prefer limiting the updates and avoiding setting '1' before '10' when the user is typing. So the flag ImGuiInputTextFlags_NoLiveEdit looks the best option, but I understand it means some work to be easy to integrate.

For the moment I will patch my code with 1st or 2nd solution depending on the presence of hardware behind, and if some day you manage the flag it will be great!

ocornut commented 1 week ago

I think the best workaround may be to use InputInt() with step == 0 to disable those buttons, and then IsItemDeactivatedAfterEdit() should work fine for you.

olivier-gerard commented 1 week ago

Thanks, I patched my stuff, mainly using a step to Null (not equal to 0) to hide +/- buttons, and manually added them right after. It does not handle repeat, but this is ok for now.