imgui icon indicating copy to clipboard operation
imgui copied to clipboard

Can not open modal popup from menu / Issue with ID stack

Open nem0 opened this issue 10 years ago • 22 comments

This works:

 if (ImGui::Button("Import asset"))
 {
    ImGui::OpenPopup("ImportAssetDialog");
 }
 if (ImGui::BeginPopupModal("ImportAssetDialog"))
 {
    ImGui::Text("test");
 }

This does not:

 if (ImGui::MenuItem("Import asset"))
 {
    ImGui::OpenPopup("ImportAssetDialog");
 }
 if (ImGui::BeginPopupModal("ImportAssetDialog"))
 {
    ImGui::Text("test");
 }

This happens

imgui_popup_error

nem0 avatar Sep 14 '15 11:09 nem0

Is that exactly the code you are actually using/testing with ?

Popup Identifiers have to be part of the ID stack to clear ambiguity and allows a same popup to work from different items sources. So your calls OpenPopup() and BeginPopupModal() have to be in the same level of the ID stack. I imagine your MenuItem() is inside a BeginMenu/EndMenu pair in your code and that adds to the popup stack?

Now there is another problem here is that MenuItem() will close your current popup as well.

If you understand those things you should be able to come up with a workaround very easily, it is likely that your real code is misplaced (you aren't pasting the full code here).

But I still would like to improve the situation here.

  • Clarify or improve the situation with MenuItem/Selectable closing their parent popup and how it affect ID and popups created there.
  • Perhaps be able to sort of "clear" the value at the top of the ID stack so those locations can refer to the same identifier. This would also be useful in the future to programmatically refer to widgets from outside their location.

ocornut avatar Sep 14 '15 11:09 ocornut

The exact code I use is complicated, therefore I did not paste it here. However I tried to find the simplest code to reproduce the problem, here it is:

        ImGui::NewFrame();

        if (ImGui::BeginMainMenuBar())
        {
            if (ImGui::BeginMenu("menu"))
            {
                if (ImGui::MenuItem("menu item"))
                {
                    ImGui::OpenPopup("popup");
                }
                if (ImGui::BeginPopupModal("popup"))
                {
                    ImGui::Text("Lorem ipsum");
                }
                ImGui::EndMenu();
            }
            ImGui::EndMainMenuBar();
        }

        ImGui::Render();

Just to be sure I tried to place BeginPopupModal from the example in every possible place, nothing works.

The only solution I found is this, however I hope there is something better:

        ImGui::NewFrame();

        bool b = false;
        if (ImGui::BeginMainMenuBar())
        {
            if (ImGui::BeginMenu("menu"))
            {
                if (ImGui::MenuItem("menu item"))
                {
                    b = true;
                }
                ImGui::EndMenu();
            }
            ImGui::EndMainMenuBar();
        }

        if (b)
        {
            ImGui::OpenPopup("popup");
        }

        if (ImGui::BeginPopupModal("popup"))
        {
            ImGui::Text("Lorem ipsum");
            ImGui::EndPopup();
        }

        ImGui::Render();

I've pulled the source code of ImGui yesterday.

nem0 avatar Sep 14 '15 12:09 nem0

Your first example can't work anyway because the if (ImGui::BeginPopupModal("popup")) block is only called when the menu is being browsed. If it worked it would assert anyway because you aren't calling `EndPopup' anywhere.

The second example is correct and exactly what I have mentioned, the ID will match.

Now I think what would be desirable is to make this example work (currently it won't because the opened popup identifier will be "mainmenubar+popup" and the begin is on "popup", this why I was suggesting a way to alter how the popup is using - or not - the id stack.

        ImGui::NewFrame();

        bool b = false;
        if (ImGui::BeginMainMenuBar())
        {
            if (ImGui::BeginMenu("menu"))
            {
                if (ImGui::MenuItem("menu item"))
                   ImGui::OpenPopup("popup");
                ImGui::EndMenu();
            }
            ImGui::EndMainMenuBar();
        }

        if (ImGui::BeginPopupModal("popup"))
        {
            ImGui::Text("Lorem ipsum");
            ImGui::EndPopup();
        }

        ImGui::Render();

ocornut avatar Sep 14 '15 12:09 ocornut

Now I understand why the first example does not work. Can there be some special way/flag in OpenPopup so that your example works?

For example:

 ImGui::OpenPopup("some_id", FlagPutInRoot)

 // somewhere else in root 
 if (ImGui::BeginPopupModal("popup"))
 {

nem0 avatar Sep 14 '15 12:09 nem0

Yes I would like to solve that but it would need to be solved in a generic manner not only for popup. I don't know how and when yet (considering there is a workaround for popups it isn't a top priority).

ocornut avatar Sep 14 '15 12:09 ocornut

Ok, I will use the workaround until it's solved. Thank you.

nem0 avatar Sep 14 '15 12:09 nem0

Hello, I also encoutered this "problem". I have a humble idea, what if when label contains for example 4# at the begining like: ImGui::Button("####I am a global label"); Then when generating ID it would just simply ignore the stack and use only inputed string for hashing. Of course that would mean that user must be more careful with them because of collisions. But it seems like relatively easy solution to me. Of course I might be wrong, because of some internal structure or something....

janwaltl avatar Sep 27 '15 19:09 janwaltl

This is probably a good solution to solve this specific problem (aside from the fact that the amount of # characters may start to be a little worrying). My concern is that it doesn't provide a clear solution to identifying any widget without relying on global id, so it may be good for popup but not for other things. At this point my tendency is to sit on ideas until I can tackle a wider set of related problems all-together to avoid making too many short-term mistakes. So I'd be happy if you want to think about or discuss about identifiers in general and see where it can lead us.

Interestingly, for an hypothetical ActivateWidget() function, appending strings without any separator would work (syntactically we could allow a separator such as '/' to make them more readable) but we might still need a way to pass on chain of identifiers involving integers/pointers.

ocornut avatar Sep 27 '15 19:09 ocornut

Aha, just wanted to say that took me a while to understand why the popups didnt work after a MenuItem() as well. Got puzzled exactly like nem0 and found the same "solution".

r-lyeh-archived avatar Aug 14 '16 22:08 r-lyeh-archived

I stumbled upon the same issue, I kind of understand why this is happening and I was wondering if we could find a temporary solution.

Would it be possible to open a modal popup using a variable? I thought I could use the p_open argument in BeginPopupModal, but it doesn't work and neither using the variable as a condition to create the modal itself:


var openModal = false;

if ( ImGui::Button("open") )
	openModal = true;

if ( ImGui::BeginPopupModal("foo", &openModal) )
{
    ImGui::Text("Hello!");    
    if ( ImGui::Button("Ok") ) 
    { 
    	ImGui::CloseCurrentPopup(); 
    	openModal = false;
    }
    ImGui::EndPopup();
}

if ( openModal )
{
	ImGui::BeginPopupModal("foo");
    ImGui::Text("Hello!");    
    if ( ImGui::Button("Ok") ) 
    { 
    	ImGui::CloseCurrentPopup(); 
    	openModal = false;
    }
    ImGui::EndPopup();
}

I ended up going back using a Button which kind of works(parent popup stays open), however I can't close the parent popup when I close the modal, ideally I'd like to call ImGui::ClosePopup("parent-popup") but it's not accessible.

if ( ImGui::BeginPopupModal("foo") )
{
    ImGui::Text("Hello!");    
    if ( ImGui::Button("Ok") ) 
    { 
    	ImGui::CloseCurrentPopup();                 // close modal
        ImGui::ClosePopup("parent-popup");   // close parent popup
    }
    ImGui::EndPopup();
}

q-depot avatar May 16 '17 19:05 q-depot

I had a similar situation trying to write a Save File Menu Item. It opens up a modal popup for you to write the full path for the file to be saved(temporary until I figure out a good way to incorporate proper File Dialogs) and that ,if the file already exists ,in turn has to open up a stacked modal popup that asks for permission to overwrite. The first modal is easy. The solution above with the bool flag toggle inside the MenuItem works fine. But that got a bit messy with bool flags too quickly due to dependencies. The first modal upon clicking Save has to either complete the action or wait for positive feedback from the second stacked modal.

Instead an enum with the states of this situation worked on the first try. Much cleaner and more straightforward.

NPatch avatar Oct 26 '18 16:10 NPatch

In the case of MenuItems...if you've got many modals and possibly stacked as well, having an enum with the full flow in states for each different operation makes it easier to handle multiple modals code in the same scope with minimal variables. You basically have one variable for all the modals and the menu item they get triggered by for each operation without affecting the others.

NPatch avatar Oct 26 '18 16:10 NPatch

I was about to open an issue regarding the same problem, good thing I've searched first.

flamendless avatar Jan 03 '20 03:01 flamendless

My solution to this is to use a lambda. A function pointer can be set at any state (even inside a menu click) and in the end of your code you simply need to call it.

Not sure if this has perfomance issues I may be overlooking though, otherwise its pretty handy:

{

ImGui::NewFrame();

static void(*ShowPopup)() = []() {};

if (ImGui::BeginMainMenuBar())
{
	if (ImGui::BeginMenu("menu"))
	{
		if (ImGui::MenuItem("menu item"))
		{
			ShowPopup = []()
			{
				if (!ImGui::IsPopupOpen("popup"))
					ImGui::OpenPopup("popup");

				if (ImGui::BeginPopupModal("popup"))
				{
					ImGui::Text("Lorem ipsum");

					if (ImGui::Button("Close", ImVec2(80, 0)))
					{
						ImGui::CloseCurrentPopup();
						ShowPopup = []() {};
					}
					ImGui::EndPopup();
				}
			};
		}
		ImGui::EndMenu();
	}
	ImGui::EndMainMenuBar();
}

ShowPopup();

ImGui::Render();

}

AnimatorJeroen avatar Feb 19 '20 19:02 AnimatorJeroen

I found a temporary solution to this bug for anyone waiting for a fix. Hopefully ocornut fixes it soon.

bool openpopuptemp = false;

//in render loop
if (ImGui::BeginMenu("Menu")){
		if (ImGui::MenuItem("Open Popup", NULL)) { 
			openpopuptemp = true;
		}
		ImGui::EndMenu();
}
if (openpopuptemp == true) {
		ImGui::OpenPopup("popup");
		openpopuptemp = false;
}
if (ImGui::BeginPopupModal("popup")){
            ImGui::Text("Lorem ipsum");
            ImGui::EndPopup();
}

ppekko avatar Dec 26 '20 16:12 ppekko

A tricky bit with the solution in https://github.com/ocornut/imgui/issues/331#issuecomment-140055181 is that if you want to set size/position on the popup with SetNextWindow*, you need to make sure the calls to SetNextWindow are right nearby the call to OpenPopup and that no other windows sneak in while you're not looking:

		if imgui.IsKeyDown(sdl.SCANCODE_ESCAPE) {
			imgui.OpenPopup("Menu")
			imgui.SetNextWindowSize(imgui.Vec2{X: 400, Y: 0})
			imgui.SetNextWindowPosV(imgui.Vec2{X: float32(wd) / 2, Y: float32(ht) / 2}, 0, imgui.Vec2{X: .5, Y: .5})
		}
                // --------- No windows in here, or else! -----------
		if imgui.BeginPopupModalV("Menu", nil, imgui.WindowFlagsNoScrollbar|imgui.WindowFlagsNoResize) {
			if imgui.ButtonV("Quit", imgui.Vec2{X: -1, Y: 0}) {
				running = false
			}
			if imgui.ButtonV("Return", imgui.Vec2{X: -1, Y: 0}) {
				imgui.CloseCurrentPopup()
			}
			imgui.EndPopup()
		}

(this is go, but it maps pretty directly to the c++)

Or am I missing a trick somewhere?

decitrig avatar Oct 12 '21 01:10 decitrig

Just came here to say I also ran into this problem, and experienced the current behavior when opening a popup from a MenuItem to be very confusing and non-discoverable from the demo application.

I'm using the workaround proposed by @nem0 in 2015, which works, but it kind of ruins the nice immediate property of the rest of the ImGui API by having to explicitly track menu activation state using a boolean and defer the logic of the modal itself to some point after where it is triggered (which could be somewhere way down the source code if you have a long menu).

In my case I just want to open a 'confirm quit yes/no?' dialog triggered from the 'Quit..' menu item in the main menu bar, which seems like a very common use case. Would be nice if there would be a better solution.

w0utert avatar Sep 05 '22 15:09 w0utert

Computing the popup ID ahead of time should help.

In the unpublished TestEngine we use path names for identifier and we intend to provide that same functionality in Core eventually. They should also help with this situation. The gist of how those path works is:

“some_id” “../some_id” // id base seed is one level before. “../something/some_id” // replace parent level with another base “/some_id” // id base seed using window base seed “//some_id” // id base seed is 0

ocornut avatar Sep 05 '22 15:09 ocornut

That sounds like a nice solution, I will keep watching this issue so I can adapt my code when this is available, thanks!

w0utert avatar Sep 05 '22 15:09 w0utert

Hi, I came up with a different work-around to this after struggling like OP has. I use PushOverrideID from the internal api:

ImGuiID popup_id = ImHashStr( "POPUP" );
ImGui::PushOverrideID( popup_id );
ImGui::OpenPopup( "POPUP" );
ImGui::PopID();
[...]
ImGui::PushOverrideID( popup_id );
 if( ImGui::BeginPopup( "POPUP" ) ) {
    [...]
    ImGui::EndPopup();
}
ImGui::PopID();

This will work in any context :) hope this helps someone.

katemonster33 avatar May 11 '23 00:05 katemonster33

does this still not work still after almost 10 years or am i missing some new api? the following code still fails:

if (ImGui.BeginPopupContextItem("generate"))
{
	if (ImGui.MenuItem("events"))
		ImGui.OpenPopup("confirm");
	ImGui.EndPopup();
}
if (ImGui.BeginPopupModal("confirm"))
{
	ImGui.Text("hello")
	ImGui.EndPopup();
}

and honestly the approach above works but is silly

aymen157 avatar Sep 23 '24 13:09 aymen157

The issue is open because the status quo hasn't changed yet. My plan is to add support for "/" "../" prefixes as mentioned, and to be in line with what imgui_test_engine support, but for various reasons it requires specific design for imgui which I have not done yet.

ocornut avatar Sep 23 '24 13:09 ocornut