Opening popups with same name but non-conflicting local IDs breaks rendering

PathogenDavid commented 8 months ago

Version/Branch of Dear ImGui:

1.90.3

Back-ends:

imgui_impl_dx11.cpp + imgui_impl_win32.cpp

Compiler, OS:

Windows 10 + MSVC 2022 (17.9.0p5)

Full config/build information:

No response

Details:

Popups effectively have two IDs:

The ID of the popup its self, which participates in the ID stack
The ID of the ImGuiWindow representing the popup, which does not participate in the ID stack

Both are derived from the ID passed to BeginPopup, OpenPopup, etc.

As a result, it's relatively easy to create two popups with different popup IDs but the same window ID.

If both of these popups are visible at the same time neither will render, and the case of modals UI will become unresponsive.

To reproduce:

Copy the snippet into any example app and run
Open the metrics window and expand both DrawLists and Popups
Click Show outer test modal
Modal opens as expected and is visible under both DrawLists and Popups in the metrics window.
Click Show inner test modal
Contents submitted to inner modal will be visible in the outer modal for a single frame
Both modals disappear, both popups will be in the Popups list but neither will be in the DrawLists list.

I suspect this might be the same as https://github.com/ocornut/imgui/issues/6939 but the user couldn't figure out what was happening.

Screenshots/Video:

After opening outer modal:

After opening inner modal:

(Note that metrics window shows both popups as open but their draw lists are not being submitted.)

Minimal, Complete and Verifiable Example code:

if (ImGui::Button("Show outer test modal"))
    ImGui::OpenPopup("TestModal");

if (ImGui::BeginPopupModal("TestModal"))
{
    ImGui::Text("This is the outer test modal");

    if (ImGui::Button("Show inner test modal"))
        ImGui::OpenPopup("TestModal");

    if (ImGui::BeginPopupModal("TestModal"))
    {
        ImGui::Text("This is the inner test modal");
        if (ImGui::Button("Close"))
            ImGui::CloseCurrentPopup();
        ImGui::EndPopup();
    }

    ImGui::EndPopup();
}

PathogenDavid commented 8 months ago

In my opinion it's fairly confusing that popups effectively have two IDs like this. Dear ImGui is trying to concatenate the two windows but I'd argue this isn't ever what the user wants even without the rendering issue. (If the user really wanted this they'd concatenate by using BeginPopup twice in the same ID stack context--which does work today.)

As a related aside, I think it's somewhat unfortunate that ImGui::Begin isn't affected by PushID/PopID as well.

I think a possible fix to both of these issues would be to introduce a new flag ImGuiWindowFlags_InheritIdStack, which would instruct ImGui::Begin to use g.CurrentWindow->GetID(name) for its ID instead of ImHashStr as it does today.

Since popup IDs are based on the current ID stack, they would always pass this flag to their inner ImGui::Begin. Additionally, the flag is there for users who want it as well (like me.)

This flag might also be helpful for ImGui::BeginChild, which has a similar issue that is being handled manually.

(This whole thing came up organically while typing up an example for a suggestion on handling https://github.com/ocornut/imgui/issues/7324 since I was supporting nested keybinding windows as CDDA already does today.)

PathogenDavid commented 8 months ago

I did a quick and (very) dirty implementation of ImGuiWindowFlags_InheritIdStack just to get an rough idea of the impact it might have: https://github.com/PathogenDavid/imgui/commit/30334ce5e8238e09ef62de0b45d92d896683a963

One issue that is immediately apparent is that the ID of the window isn't recorded in imgui.ini, so two windows with the same name and different IDs will conflict and their settings can't be loaded and applied properly. Not insurmountable, but I didn't want to push further right now.

I quickly looked over other uses of ImHashStr to see if anything else assumes a window's ID is always the hash of its name but didn't notice anything else.

ocornut commented 8 months ago

I think there are multiple issues and some of them are going to requires rather deep work to untangle.

What's a little bit arbitrary is how BeginPopup() assumes no title bar, and pretty transient state, while BeginPopupModal() doesn't, and that has effects on how their name are generated, BeginPopupModal() being somewhat in-between Begin(). and BeginPopup(). In my experience, over time I found that modals were generally closer to regular windows in term of how users wants to use them, and I wondered if the slightly odd modal API couldn't be reorganized to steer closer to regular Begin() with closure being written to bool* p_open. The popup essentially carrying that open state is many times a great convenience but not always so ideally we could consider to support both paths (it is also why I kept #402 open).

Test Engine favors using "paths" to both windows and items and I found it quite valuable when those paths are rather obvious to the users rather than requiring some guessing and convoluted function (and I think it could become valuable to main lib as well, e.g. #331). The names generated by BeginChild() are currently both required (as they do inherit ID stack) and an annoyance for those paths (it's currently handled by a WindowInfo() helper in test engine). I recently tried to change "ParentName/ChildName_XXXXXXXX" where XXXXXX is = HashOfChildNameFromIdStackOfParent. ideally it would be "ParentName/ChildName" when in root of parent but it created a problem with use of ### operators, but that is parsing problem that can be solved. I do have a bunch of scattered notes relating to the name vs id of windows and the general tendency is toward favoring that ID can be inferred from name.

There's just a lots of untangle together here so I don't have a clear answer. But as the problem AFAIK only apply to modals, and it would be easier and consistent that BeginPopupModal() decorates the window name similarly to other functions (as "WindowName##PopupID" as a first step.

ocornut commented 8 months ago

After submitting that wall of text I recalled there was also value that a modal state (e.g. position, size) would be reused regardless of the modal calling spot in the popup stack, making this even more tricky to deal with... (and consider that IsPopupOpen() doesn't know if the name is for a regular popup or a modal).

Considering the fact that it is currently possible to manually perform the equivalent of your ImGuiWindowFlags_InheritIdStack suggestion, AKA there is a workaround in this direction, I don't think there is any pragmatic change we should do it right now without solving #331 + #402 + general desired to facilitate and support named paths (https://github.com/ocornut/imgui_test_engine/wiki/Named-References). It might be that your suggested flag would exists and be implemented by performing a prepend or append, I don't know.

ocornut / imgui