Skip to content
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

Switching between control type using the same label hits assertion #6304

Closed
n0phx opened this issue Apr 5, 2023 · 5 comments
Closed

Switching between control type using the same label hits assertion #6304

n0phx opened this issue Apr 5, 2023 · 5 comments
Labels

Comments

@n0phx
Copy link

n0phx commented Apr 5, 2023

Version/Branch of Dear ImGui:

Version: 1.89.4
Branch: master

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_opengl3.cpp + imgui_impl_glfw.cpp
Compiler: MSVC
Operating System: Windows 10

My Issue/Question:

When rendering either an int input or a combo control based on the value of a variable (and the controls use the same label, since only one of them will be rendered at any time based on the value), when switching back from the int input to combo, I hit an assert as seen in following screenshot.
Picking the value 2 in the combobox will switch to the int input control, and entering 0 into the int control will trigger the assert.

Screenshot:

image

Standalone, minimal, complete and verifiable example:

        static int itemIndex = 0;
        static int limit = 1;
        static const int itemCount = 3;
        static const char* items[] = {"0", "1", "2"};
        if (itemIndex < limit) {
            if (ImGui::BeginCombo("SharedControlName", items[itemIndex])) {
                for (int i = {}; i < itemCount; ++i) {
                    const bool isSelected = (itemIndex == i);
                    if (ImGui::Selectable(items[i], isSelected)) {
                        itemIndex = i;
                    }
                    if (isSelected) {
                        ImGui::SetItemDefaultFocus();
                    }
                }
                ImGui::EndCombo();
            }
        } else {
            ImGui::InputInt("SharedControlName", &itemIndex, 1, 100, ImGuiInputTextFlags_CharsNoBlank);
        }

Workaround #1 for problem:

Same snippet, only labels are unique:

        static int itemIndex = 0;
        static int limit = 1;
        static const int itemCount = 3;
        static const char* items[] = {"0", "1", "2"};
        if (itemIndex < limit) {
            if (ImGui::BeginCombo("NotSharedControlName1", items[itemIndex])) {
                for (int i = {}; i < itemCount; ++i) {
                    const bool isSelected = (itemIndex == i);
                    if (ImGui::Selectable(items[i], isSelected)) {
                        itemIndex = i;
                    }
                    if (isSelected) {
                        ImGui::SetItemDefaultFocus();
                    }
                }
                ImGui::EndCombo();
            }
        } else {
            ImGui::InputInt("NotSharedControlName2", &itemIndex, 1, 100, ImGuiInputTextFlags_CharsNoBlank);
        }

Hackaround #2 for problem:

Ugly tampering with imgui internals to simulate a mouse button down (in else branch of top-level if).

#include <imgui_internal.h>

...

        static int itemIndex = 0;
        static int limit = 1;
        static const int itemCount = 3;
        static const char* items[] = {"0", "1", "2"};
        if (itemIndex < limit) {
            if (ImGui::BeginCombo("SharedControlName", items[itemIndex])) {
                for (int i = {}; i < itemCount; ++i) {
                    const bool isSelected = (itemIndex == i);
                    if (ImGui::Selectable(items[i], isSelected)) {
                        itemIndex = i;
                    }
                    if (isSelected) {
                        ImGui::SetItemDefaultFocus();
                    }
                }
                ImGui::EndCombo();
            }
        } else {
            if (ImGui::InputInt("SharedControlName", &itemIndex, 1, 100, ImGuiInputTextFlags_CharsNoBlank)) {
                ImGuiContext* context = ImGui::GetCurrentContext();
                context->ActiveIdMouseButton = ImGuiButtonFlags_MouseButtonLeft;
            }
        }
@emoon
Copy link
Contributor

emoon commented Apr 5, 2023

Read https://github.com/ocornut/imgui/blob/master/docs/FAQ.md#q-how-can-i-have-multiple-widgets-with-the-same-label

@ocornut
Copy link
Owner

ocornut commented Apr 5, 2023

As answered the simpler is you change the ID using some ## suffix, while keeping the same label.

Note thet doing this has one side effect which is that Keyboard/Gamepad Focus will be lost if it was on the swappes item. We are a missing a way to “recover” to nearest item which is something we should do eventually.

And generally I think we should still be looking at this as a bug and see if we can run without assert while causing harm to the stuff this is supposed to protect for. So i’ll look at this.

@n0phx
Copy link
Author

n0phx commented Apr 5, 2023

Thanks for the replies, I was aware of the issues caused by two controls having the same ID, but I think this issue is different, because at no point in time will both controls be rendered at the same time, but depending on a variable, at each frame, only the code for one of them will be executed and one will be rendered, which makes me still lean towards treating this as a bug.

@GamingMinds-DanielC
Copy link
Contributor

because at no point in time will both controls be rendered at the same time

But you have different controls with identical IDs in subsequent frames, making the state from the previous frame that is used for some things incompatible.

@ocornut: if you want to secure this the easy way, you could maybe push/pop different IDs based on the widget type onto the ID stack internally. Then only a switch between "different" widgets of the same type would go unnoticed, but they will have compatible internal state and therefore shouldn't pose a problem.

@ocornut ocornut added the bug label Apr 5, 2023
ocornut added a commit that referenced this issue Apr 5, 2023
…r while active and using same id could lead to an assert. (#6304)

+ Demo: use BeginDisabled() block in BackendFlags section.
I'd still consider this undefined behavior as some combination may not work properly, but let's fix things while we can as we encounter them.
ocornut added a commit to ocornut/imgui_test_engine that referenced this issue Apr 5, 2023
@ocornut
Copy link
Owner

ocornut commented Apr 5, 2023

While I consider this a little bit undefined-behavior, as-in, not all transition have been well thought out nor tested, I'm OK to fix cases as we go.
Pushed fix as part of 47a07d8
And Test ocornut/imgui_test_engine@fb750e5

@ocornut ocornut closed this as completed Apr 5, 2023
kjblanchard pushed a commit to kjblanchard/imgui that referenced this issue May 5, 2023
…r while active and using same id could lead to an assert. (ocornut#6304)

+ Demo: use BeginDisabled() block in BackendFlags section.
I'd still consider this undefined behavior as some combination may not work properly, but let's fix things while we can as we encounter them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants