Warn users if they are reusing the same ID on a different widget

[pthom]

Version/Branch of Dear ImGui: Version 1.90.7

Bonjour Omar,

There is a common user issue which is that developers may reuse the same ID, and will not understand why their widget is not responsive.

Example

#include "hello_imgui/hello_imgui.h"
#include "imgui.h"
#include "stdio.h"

char text1[500] = "Hello,";
char text2[500] = "world!";
int value = 0;
bool flag1 = false, flag2 = false;
float f1 = 0.f, f2 = 0.f;

void Gui()
{
    // Only the first button will be clickable
    if (ImGui::Button("button")) printf("Button1 pressed\n");
    if (ImGui::Button("button")) printf("Button2 pressed\n");

    // Those two inputs will share the same value (editing one will overwrite the other)
    ImGui::InputText("text", text1, IM_ARRAYSIZE(text1));
    ImGui::InputText("text", text2, IM_ARRAYSIZE(text2));

    // The second radio button cannot be selected
    ImGui::RadioButton("radio", &value, 0);
    ImGui::RadioButton("radio", &value, 1);

    // The second checkbox cannot be toggled
    ImGui::Checkbox("checkbox", &flag1);
    ImGui::Checkbox("checkbox", &flag2);

    // Those two sliders will share the same value (editing one will overwrite the other)
    ImGui::SliderFloat("slider", &f1, 0.f, 1.f);
    ImGui::SliderFloat("slider", &f2, 0.f, 1.f);
}

int main() {    HelloImGui::Run(Gui); }

I did want to add an assert for Python users of Dear ImGui bundle, as they might not be able to understand the reason or the failure.

So I added a patch like the one you can see here:
pthom@ea95177

With this patch, the example code would throw an assert for each reused ID.

---

The patch adds this method:

// a version of GetID that warns if the ID was already used
IMGUI_API ImGuiID ImGuiWindow::GetID_AssertUnique(const char* str_id);

The principle is to use this method in some rare and carefuly selected locations (ButtonEx, ArrowButtonEx, Checkbox, RadioButton , DragScalar, SliderScalar, Drag, InputTextEx), i.e. the most commonly used widgets, and to warn the user if they are using an existing ID.

The implementation is simple: just remember the already pushed IDs, and raise an assert if the user is raising the same ID in the same frame. This concerns only a very limited set of the IDs (i.e. those for the aforementioned widgets).

Would you be interested in such a change? If so, I can push a PR. I know that my current implementation can be improved and should not use a static variable (instead, perhaps rely on a member of ImGuiWindow)

I know that this change could have some implications in a lot of user code. But I would think that it is for the best because they would be warned that they were doing something bad.

As an encouraging hint, I tested it successfully in all of the numerous demonstrations of Dear ImGui Bundle, and it worked correctly

Feel free to close if you do not feel that this is useful.

Note: this feature could be protected by a compilation flag, in order to avoid potential regressions in user code.

Cheers!

[pthom]

Side note: I did add a related patch for the StackLayout feature (ImGui::Begin_Horizontal + ImGui::Begin_Vertical), because reusing the same ID would lead to an infinite loop (and Python users would not be able to guess the reason for this infinite loop): see pthom@8b45d6b

[ocornut]

This implementation is unfortunately extremely CPU costly, to a point where it is going to be unacceptable for performance.
Even if you used a better data structure for it, it would likely be too much. On a complex UI this is likely to cost more than the entirety of the rest of Dear ImGui. We quite rarely perform non-O(1) operations on standard widgets.

My strategy for this would be to perform a compare based on a shared field that's normally set once a frame, e.g. g.HoveredId, so it would become a lightweight and O(1) check, with the limitation that it would trigger during interaction (e.g. hover), but considering the alternative is redhibitory it seems like a good compromise.

I don't necessary agree that an assert is ideal, but we would turn that detection into an overlay warning with a button to break into either items. We would need to use there is a way (e.g. via ItemFlags) to allow it as I am convinced there are situation where it would be useful.

[pthom]

Hello again Omar,

Many thanks for your quick answer!

This answer is only related to the CPU cost part : the question whether this is a good idea, or whether this is a good way to implement this is still opened.

I did measure the CPU usage of my implementation.

For this, I used a simple benchmark utility, see this commit:
pthom@839d331

And I tested this with quite a lot of widgets. See video below:

bench.mp4

And the results are (for 1 minutes usage of an application with lots of varied widgets)

Function              |Nb calls|Total time|Av. time|Deviation|
----------------------+--------+----------+--------+---------+
GetId(const char*)    |  821501| 126.161ms| 0.154us|  2.275us|
GetID_AssertUniquePart|  260980|  19.860ms| 0.076us|  0.207us|
GetId(int)            |   91992|   3.090ms| 0.034us|  0.097us|
GetId(void*)          |   10523|   0.629ms| 0.060us|  0.091us|

PS: I wrote this benchmark utility in a previous life. It is quite easy to setup, which is why I chose it. I suspect another benchmarking tool would lead to similar results.

[GamingMinds-DanielC]

And I tested this with quite a lot of widgets. See video below:

Looks like quite a few widgets, but separated into different tabs and not that many widgets each frame. Your implementation searches linearly through a vector for each id it has to check while the same vector grows linearly as well. That's quadratic runtime and can escalate pretty quickly.

If the deviation in your benchmark table is what I think it is (difference between minimum and maximum time per call), then GetId(const char*) already grows to take almost 15x as long as the average call. And your average call already includes the id check, so even worse compared to the unmodified version.

[ocornut]

It’s indeed a small amount of widgets. Calling 1-10k/frame would be a good base but i don’t need to see benchmarks to know it’s going to be too expensive. The entire codebase is designed to avoid this sorts of scan as much as possible.

[pthom]

It’s indeed a small amount of widgets. Calling 1-10k/frame would be a good base but i don’t need to see benchmarks to know it’s going to be too expensive. The entire codebase is designed to avoid this sorts of scan as much as possible.

I see your point. I didn't mean to come across as overconfident, and I apologize if I did.

This issue was indeed just a suggestion, and potential food for thought for a possible implementation. If this issue is deemed useful, a totally different implementation could be used, of course.

Regarding the cache, it's quite small and could be limited to a max size (e.g., 100 entries, around 400 bytes). This would fit easily in the L1 cache, making a linear scan relatively fast. This revised implementation is an attempt in this direction.

[ocornut]

I provided a solution that would be near free and react on hover, it seems like a good enough tradeoff ?

[pthom]

My strategy for this would be to perform a compare based on a shared field that's normally set once a frame, e.g. g.HoveredId, so it would become a lightweight and O(1) check, with the limitation that it would trigger during interaction (e.g. hover). we would turn that detection into an overlay warning with a button to break into either items. We would need to use there is a way (e.g. via ItemFlags) to allow it as I am convinced there are situation where it would be useful.

I'm not sure I understood how to implement your proposed solution.

perform a compare based on a shared field that's normally set once a frame,

This is the part I do not understand

we would turn that detection into an overlay warning with a button to break into either items

IMHO we would need to keep track of the already registered IDs somewhere. Am I wrong here? In that case, we could limit the loop to only when the item is hovered.

But actually I think you had another idea which I did not understand.

Anyhow, there is no emergency.

Cheers

PS: I'm quite busy on another project at the moment. I might contact you by email in the next days to tell you about it. I would like your feedback (if you have time).

[pthom]

Additional info (if using the current version of the patch): there is an additional related commit that is needed
pthom@97622b0

After running for quite a while with this patch, the only places that do reuse the Id are TempInputScalar and TempInputText. This additional commit handles this.