ocornut / imgui

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

Context menus don't work on ColorEdit4 #4167

Open CobaltXII opened 3 years ago

CobaltXII commented 3 years ago

Version/Branch of Dear ImGui:

Version: dear imgui, v1.79 WIP Branch: master

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_opengl3.cpp + imgui_impl_sdl2.cpp Operating System: macOS

My Issue/Question:

Context menus don't work on combo boxes but they work on other widgets. In the following code, if I right-click on Combo, all of ImGui freezes (but not the program itself). If I right-click on Slider, it works fine.

Standalone, minimal, complete and verifiable example:

// window
ImGui::Begin("Test", NULL, ImGuiWindowFlags_NoSavedSettings);

// combo
static int a = 0;
if (ImGui::BeginCombo("Combo", std::to_string(a).c_str())) {
    for (int i = 0; i < 10; i++) {
        bool selected = i == a;
        if (ImGui::Selectable(std::to_string(i).c_str(), selected)) a = i;
        if (selected) ImGui::SetItemDefaultFocus();
    }
    ImGui::EndCombo();
}

// context for combo
if (ImGui::BeginPopupContextItem("Combo")) {
    if (ImGui::MenuItem("Reset")) a = 0;
    ImGui::EndPopup();
}

// slider
static int b = 0;
ImGui::SliderInt("Slider", &b, 0, 10);

// context for slider
if (ImGui::BeginPopupContextItem("Slider")) {
    if (ImGui::MenuItem("Reset")) b = 0;
    ImGui::EndPopup();
}

ImGui::End();
ocornut commented 3 years ago

Hello, Thanks for the report.

That's quite interesting. A few things:

There is indeed an issue with the fact that the Combo popup will share the same ID as the Context Item Popups and they will keep overriding each others.

The immediate/easy workaround is simply to use another ID when caling BeginPopupContextItem(). This will fix your issue.

However from my POV we are trying to encourage people from not passing an ID override when calling BeginPopupContextItem() and just use the last item ID, so this is a problem that will need fixing on our end.

ocornut commented 3 years ago

Pushed a fix: 029c83c73e724af28cd1cc0714671b0673d95751

CobaltXII commented 3 years ago

Thanks so much for the quick response. Combo boxes work well with the fix. I changed all my code to call BeginPopupContextItem with no arguments. This works fine for all the widgets I am using except for ColorEdit4. However, it continues to work fine if I call BeginPopupContextItem with the ID of the color editor as I was doing before. Here is the code I am using:

ImGui::ColorEdit4(name, &color->x, ImGuiColorEditFlags_Float);
// if (ImGui::BeginPopupContextItem(name)) { <-- this works!
if (ImGui::BeginPopupContextItem()) {
    if (ImGui::MenuItem("Reset")) (void*)0;
    ImGui::EndPopup();
}

And this is the assertion that is failing:

Assertion failed: (id != 0), function BeginPopupContextItem, file lib/imgui/imgui.cpp, line 7931.

I will open a separate issue for this if you want. Thanks in advance.

ocornut commented 3 years ago

Thanks! I'll look at this from pov of view of ColorEdit later (it uses a group + popup so may be different)

ocornut commented 3 years ago

All widgets using groups such as SliderFloat3(), ColorEdit4() will have the same issue.

What I would do right now is to use if (ImGui::BeginPopupContextItem("Context")) { } Or if you have multiple of them in same id stack level: if (ImGui::BeginPopupContextItem("mywidget##context")) {}

The fix is not presently trivial, relates to #840, #2840, #2550, #1875

I think we may have to investigate the idea presented in #840 (which would mean that last item id would change when activating a sub-component, may be fine).