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

First click goes through ImGui window #8431

Open
petrihakkinen opened this issue Feb 24, 2025 · 14 comments
Open

First click goes through ImGui window #8431

petrihakkinen opened this issue Feb 24, 2025 · 14 comments
Labels

Comments

@petrihakkinen
Copy link

Version/Branch of Dear ImGui:

Version 1.91.2 WIP, Branch: master

Back-ends:

Custom engine

Compiler, OS:

Windows 10 + MSVC 2022

Full config/build information:

No response

Details:

We are encountering a rare issue where the first mouse click over our application window sometimes goes through the hovered ImGui window and causes the app to process it. This happens very rarely but it is quite annoying when it happens (it's a custom editor so mis-clicks on the underlying 3d viewport can have all sorts of effects). I've tracked the source of at least one case where this happens. This might be an issue with ImGui itself but more likely in the way we integrate ImGui to our application.

As per documentation we use WantCaptureMouse in our event handling code to decide whether we should process the (mouse) input event in the application. We also feed ImGui input by calling AddMouseButtonEvent, AddMousePosEvent, AddFocusEvent, etc. before calling NewFrame as recommended. However, since WantCaptureMouse is updated in NewFrame (after our application has already processed input) the following can happen:

  1. Some other application is active; our application does not receive messages from Windows.
  2. The mouse is moved over our application and the mouse is pressed on the same frame.
  3. In this case WantCaptureMouse is still false and our application processes input events.
  4. Later WantCaptureMouse is set to true in NewFrame but this is too late since input events have already been processed.

Thus effectively the click seems to go through the ImGui window.

What is the recommended way to deal with this? Should we buffer all input events and process them after ImGui::NewFrame call? Should we perhaps call UpdateHoveredWindowAndCaptureFlags in our Windows message handler? Note that without buffering we would probably need to call UpdateHoveredWindowAndCaptureFlags for every mouse event to correctly track the hovered window.

Or should ImGui perhaps handle this internally somehow? It seems that the root problem is the same as with issue #7066.

Possibly related issues:
#7066
#1152

Screenshots/Video:

No response

Minimal, Complete and Verifiable Example code:

@ocornut ocornut added the inputs label Feb 24, 2025
@ocornut
Copy link
Owner

ocornut commented Feb 24, 2025

Hello,

This makes sense and I would need to investigate this to find a solution or workaround.

Seems indeed the same as #7066. (edit actually touch has a separate solution).

However, what I am most surprised about is the following:

The mouse is moved over our application and the mouse is pressed on the same frame.

While at e.g. 30 FPS it is not hard to do a double-click within same frame, a move within boundaries + click seems extremely unlikely to happen within that time frame with a mouse. While in theory it is possible and plausible, the fact that it happens in practice is surprising and prompts me to ask more questions to try to narrow down if your cause is the one we are discussing and not yet another cause.

  • What's your app typical framerate?
  • Do you have some idle mechanism when unfocused?
  • Can you clarify "This happens very rarely", e.g. does it happen once or a few times a day to you or some users, or more, or less?
  • Does your work pipeline makes your users very often swap focus between your editor and another application?

@ocornut
Copy link
Owner

ocornut commented Feb 24, 2025

A possible solution would indeed to be call UpdateHoveredWindowAndCaptureFlags() in message handler, or even in io.AddMousePos().

However, ideally it would be reworked to be more optimal and fine tuned for that sorts of situation: if the goal is only to update io.WantCaptureMouse, we need only a subset of information and the search may be much faster.

@petrihakkinen
Copy link
Author

I can reproduce the issue reliably by placing two applications side by side and clicking back and forth between them. It takes usually a minute if I'm being casual but if I try to be really fast with the mouse I can do it in less than 10 attempts. I'm using a remote connection, which may affect timing of Windows messages and may clump messages together, so that may be a factor here.

Our target frame rate is 60, but the the frame rate can vary as this is a development tool. Frame rates in the 20-30 range are not unheard of with weaker hardware or high system load. We also have a few hundred developers using the tool daily with varying hardware and setups. Some have multi-monitor setups, some work remotely etc. Unfortunately we don't have telemetry for this issue (although we probably could set it up if necessary), so it's hard to estimate how many times this happens per day. And in many cases the click throughs are harmless and go unnoticed. I think it's also "once you see it you cannot unsee it" kind of issue.

The application runs at full frame rate when unfocused, i.e. there are no idle mechanisms that should affect this. However, it is possible Windows will swap out pages when the app is inactive and memory usage is high. Under these conditions the first click issue would be more probable to happen.

Our workflow involves a lot of swapping between processes. External code or script editors are widely used, and also our application window is embedded within another window owned by another process (for complicated historical reasons I cannot go too deeply here). When the mouse is over the region owned by the host process, our application / ImGui is not fed with events as Windows only sends mouse events (unless captured) to the process owning the window. Part of the UI is in the host process, so this going back and forth between two processes happens a lot. I believe this increases the probability of the issue in a major way.

That said, I do think there is some another mechanism which can cause this issue to appear in our environment. I haven't gotten a clear understanding about it yet, but I have a video capture from a developer showing mouse being hovered over an imgui window for a second or so before the click which goes through an imgui window. The problem in the video cannot be explained by WantCaptureMouse lagging one frame. The video got me debugging this issue and led me to found the issue described in this ticket. I haven't been able to reproduce this exact problem on my end yet, but I will get back to you as soon as I have more information. To be clear, I don't suspect an issue in ImGui itself, more likely there's something wrong in our integration of ImGui.

I also realized calling UpdateHoveredWindowAndCaptureFlags after every AddMousePos probably does not fix the issue, as UpdateHoveredWindowAndCaptureFlags relies on the mouse position to be set in IO struct which AddMousePos does not do (for obvious reasons).

@ocornut
Copy link
Owner

ocornut commented Feb 25, 2025

Thanks for those details!

Is the remote connection RDP or another thing?

Indeed the last paragraph is particularly troubling:

our environment. I haven't gotten a clear understanding about it yet, but I have a video capture from a developer showing mouse being hovered over an imgui window for a second or so before the click which goes through an imgui window

Two suggested actions:

(1) Can you add some logging using IMGUI_DEBUG_LOG() to record the value of io.WantCaptureMouse at the offending time. Then open Tools>Debug Log>[X] IO, and try to get a recording of when the issue happen so we have a confirmation if the same frame suspicions is the main cause.

(2)
To narrow down the issue, and especially if you can repro on your side, I would suggest to try using UpdateHoveredWindowAndCaptureFlags() first.
After each call to io.AddMousePos():

  • backup io.MousePos and temporarily overwrite it
  • call UpdateHoveredWindowAndCaptureFlags()
  • immediately restore io.MousePos().

We can perfectly make this a parameter to the function if needed. But it would be good to first confirm that we are on the right track with the cause and solution, especially considering the doubts raised by the previous paragraph.

@petrihakkinen
Copy link
Author

I currently use Remote Desktop Connection, which is not the greatest... some developers are using better alternatives.

Update: So far I haven't been able to reproduce the issue with (modified version of) ImGui_demo.cpp. I tried adding the following to ShowDemoWindow:

if(ImGui::BeginPopupContextVoid("bg"))
{
    ImGui::Selectable("Hello");
    ImGui::EndPopup();
}
Sleep(100);

But I don't understand why it does not happen there... logically it should.
(And sorry, this should have been the first thing to try!)

So now I'm suspecting both problems are caused by some subtle difference between the demo app and our code.

@petrihakkinen
Copy link
Author

petrihakkinen commented Feb 25, 2025

Can you add some logging using IMGUI_DEBUG_LOG() to record the value of io.WantCaptureMouse at the offending time. Then open Tools>Debug Log>[X] IO, and try to get a recording of when the issue happen so we have a confirmation if the same frame suspicions is the main cause.

I did this yesterday when debugging the code. And indeed WantCaptureMouse is false on first click and it's set to true soon afterwards.

Edit: to clarify, this was the case where mouse goes down immediately when entering the window. I haven't been able to repro the issue in the video yet.

@petrihakkinen
Copy link
Author

Update: I can now reproduce the issue with imgui_demo.cpp by adding the following lines to imgui_impl_win32.cpp:

    case WM_XBUTTONDOWN: case WM_XBUTTONDBLCLK:
    {
        ImGuiMouseSource mouse_source = ImGui_ImplWin32_GetMouseSourceFromMessageExtraInfo();
        int button = 0;
        if (msg == WM_LBUTTONDOWN || msg == WM_LBUTTONDBLCLK) { button = 0; }
        if (msg == WM_RBUTTONDOWN || msg == WM_RBUTTONDBLCLK) { button = 1; }
        if (msg == WM_MBUTTONDOWN || msg == WM_MBUTTONDBLCLK) { button = 2; }
        if (msg == WM_XBUTTONDOWN || msg == WM_XBUTTONDBLCLK) { button = (GET_XBUTTON_WPARAM(wParam) == XBUTTON1) ? 3 : 4; }
        if (bd->MouseButtonsDown == 0 && ::GetCapture() == nullptr)
            ::SetCapture(hwnd); // Allow us to read mouse coordinates when dragging mouse outside of our window bounds.
        bd->MouseButtonsDown |= 1 << button;
        io.AddMouseSourceEvent(mouse_source);
        io.AddMouseButtonEvent(button, true);

        // ADDED
        if(io.WantCaptureMouse)
            IMGUI_DEBUG_LOG("ImGui captured mouse event (WantCaptureMouse = true)\n"); 
        else
            IMGUI_DEBUG_LOG("Application got input event (WantCaptureMouse = false)\n");
        //

        return 0;
    }

It usually prints "ImGui captured mouse event (WantCaptureMouse = true)" but occasionally "Application got input event (WantCaptureMouse = false)" when clicking back and forth between two apps.

@petrihakkinen
Copy link
Author

Confirmed that the suggested workaround fixes the issue in imgui_demo.cpp:

        // WORKAROUND
        ImVec2 prev_mouse = io.MousePos;
        POINT mouse_pos = { (LONG)GET_X_LPARAM(lParam), (LONG)GET_Y_LPARAM(lParam) };
	io.MousePos = ImVec2((float)mouse_pos.x, (float)mouse_pos.y);
	ImGui::UpdateHoveredWindowAndCaptureFlags();
	io.MousePos = prev_mouse;

        // ADDED
        if(io.WantCaptureMouse)
            IMGUI_DEBUG_LOG("ImGui captured mouse event (WantCaptureMouse = true)\n"); 
        else
            IMGUI_DEBUG_LOG("Application got input event (WantCaptureMouse = false)\n");

@ocornut
Copy link
Owner

ocornut commented Feb 25, 2025

Update: I can now reproduce the issue with imgui_demo.cpp

You mean in the example application, e.g. example_win32_directx11/main.cpp ?

Confirmed that the suggested workaround

This should be most naturally added in the WM_MOUSEMOVE handler but both would work.

Can you confirm that the equivalent workaround work in your real app using your own backend?

The reason for my questioning is that I find it surprising that bug exhibited in this manufactured case (using Sleep(100)) would manifest so often in a real-time framerate application. As per your paragraph "That said, I do think there is some another mechanism which can cause this issue to appear in our environment." it seems like there are two different issues, and the one we are fixing here might be the one affecting you the least commonly.

@petrihakkinen
Copy link
Author

You mean in the example application, e.g. example_win32_directx11/main.cpp

Yes, I used example_win32_directx10.

I find it surprising that bug exhibited in this manufactured case (using Sleep(100))

The latest repro with the example application does not need Sleep calls. I just added the log calls and clicked back and forth between two apps. Occasionally WantCaptureMouse is false when clicking the imgui window which indicates a problem. I've tried this locally and when running though remote desktop and the issue occurs in both cases, but it is much easier to reproduce with remote desktop.

Anyway, as I mentioned in the email there are two separate issues:

  1. The one described in this ticket which can be reproduced with the imgui example project just by adding logging to it.

  2. The second issue seems to be a bug in our code. We are getting spurious WM_SETFOCUS and WM_KILLFOCUS events that shouldn't normally be happening (most likely caused by special setup with embedded window). See the below event log.

WM_SETFOCUS -> AddFocusEvent(true)
WM_KILLFOCUS -> AddFocusEvent(false)
ImGui::NewFrame
ImGui::ClearInputMouse -> sets IO.MousePos to -2147483648, -2147483648
WantCaptureMouse changes from true -> false (mouse_avail true, g.HoveredWindow false, mouse_any_down false, has_open_popup false, mouse_pos -2147483648, -2147483648)
WM_SETFOCUS -> AddFocusEvent(true)
AddMousePosEvent(1965, 760)
AddMouseButtonEvent(button=1, down=true)
button event *not* consumed by imgui and application processes it (wantCaptureMouse false, unlessPopupClose false, hasMouse true)
 -> click goes through imgui window

If these spurious WM_KILLFOCUS and WM_SETFOCUS calls happens exactly on the boundary of two frames, WantCaptureMouse will be false and the next click will pass through to application.

However, it is interesting to note that there's technically nothing wrong with input messages. If we strip out everything else than the input event calls, we have:

AddFocusEvent(true)
AddFocusEvent(false)
AddFocusEvent(true)
AddMousePosEvent(1965, 760)
AddMouseButtonEvent(button=1, down=true)

This is a valid chain of events and technically should be interpreted as a button press on an imgui window. The window is at 1965, 760 where the click happens.

I'm now thinking that WantCaptureMouse is wrong conceptually. If the frame is long enough lots of input events can come, and it can contain arbitratry number of moves, clicks and focus events. Meaning that the state of WantCaptureMouse shoud be able potentially change after any input event, instead of being latched once per frame. This latching can currently happen at an arbitrary point in a chain of input events. So I'm wondering, is using an essentially global variable the right way to communicate input event consumption with the application? Should we take a more functional approach where AddMouseButtonEvent etc. have a return value (a boolean) which tells whether ImGui consumed the event? This is a pretty common pattern in event handling in general.

@petrihakkinen
Copy link
Author

It seems that this behavior (spurious WM_KILLFOCUS and WM_SETFOCUS events) is normal Windows behavior with child windows. Windows sets the focus momentarily to the parent window and then back to child window on first click.

I'm not sure how we can workaround this in our application. For example, WM_ACTIVATE messages are of no use because they are not received by child windows.

I could try to change the imgui example app to have a child window and try to reproduce the problem there, but that takes some time.

Hmm, any other ideas?

@ocornut
Copy link
Owner

ocornut commented Feb 26, 2025

The latest repro with the example application does not need Sleep calls. I just added the log calls and clicked back and forth between two apps. Occasionally WantCaptureMouse is false when clicking the imgui window which indicates a problem. I've tried this locally and when running though remote desktop and the issue occurs in both cases, but it is much easier to reproduce with remote desktop.

I'm finding it hard to comprehend so that's possible locally with the example application. How long does it takes you?

To clarify, I'm adding:

        if (io.WantCaptureMouse)
            IMGUI_DEBUG_LOG("ImGui captured mouse event (WantCaptureMouse = true)\n");
        else
            IMGUI_DEBUG_LOG("Application got input event (WantCaptureMouse = false)\n");

to Win32 mouse down handler and clicking back and forth repeatedly.

In your log prefer to use IMGUI_DEBUG_LOG() at it includes the frame count, which matters especially when it comes to io.AddFocusEvent() calls.

So I'm wondering, is using an essentially global variable the right way to communicate input event consumption with the application? Should we take a more functional approach where AddMouseButtonEvent etc. have a return value (a boolean) which tells whether ImGui consumed the event? This is a pretty common pattern in event handling in general.

It doesn't matter if it is global (as in, in io structure) or a return value, as the location testing that flag will be after the AddMouseXXX calls. What matter is essentially that the flag is not updated in the io.AddMousePos() call but in the NewFrame() call. It should probably be updated in the AddMousePos() call.

Combined with focus being cleared:

AddFocusEvent(true)
AddFocusEvent(false)
NewFrame() <---- clear mouse data
AddFocusEvent(true)
AddMousePosEvent(1965, 760)
AddMouseButtonEvent(button=1, down=true)

Anything after NewFrame() is technically next frame. Because of focus being false at the time of new frame, the subsequent events are equivalent to a mouse teleport, aka similar to if we used a very long sleep delay and performing multiple actions, and the "lag" become overly visible.

If we make AddMousePosEvent() update the capture flag it would fix the biggest issue. But the spurious focus change will lead to a mouse teleport which may have other subtler side effects.

I could try to change the imgui example app to have a child window and try to reproduce the problem there, but that takes some time.

That would be helpful because it gives me a test bed to work from if it repro. it might be a matter of adding only a few lines.
The ideal would be to create child window from top-left corner of parent to avoid any mouse coordinates issues with standard backends.

To recap

  • I don't yet understand/repro the claim that it would repro on vanilla example app, and I would like to understand this, because anything I can't understand seems fishy to me.
  • The main fix is probably that we should aim to update capture flag on AddMousePosEvent().
  • We should still aim to investigate and repro locally the focus events on child window so I can design a solution. For this it would be good if you can craft a repro.

@petrihakkinen
Copy link
Author

petrihakkinen commented Feb 27, 2025

I'm finding it hard to comprehend so that's possible locally with the example application. How long does it takes you?

I agree, it's very, very hard to repro it locally with 60 fps. I've seen it happen only a couple of times this week. I think it's a non-issue in this case. On remote it's a very different story. Here's a video: https://www.dropbox.com/scl/fi/6bfollwovz76h58dlcyss/first_click_issue.mkv?rlkey=r5dvpdfwnjhvrq4irax8us98b&st=9htl9sdh&dl=0

I also noticed the example app runs at 32 fps(?) under remote connection which is a bit strange. Locally it runs 60 fps. Not sure if the difference in frame rate explains the different behavior.

Anyway, I do think it is worth fixing based on this alone. Many developers use remote connection often (and may run at 30fps or lower occasionally locally).

I did some more debugging yesterday and found where those spurious WM_KILLFOCUS/WM_SETFOCUS messages come from. They are coming from the default window proc under some very specific circumstances: our host application (where the imgui application is embedded) sends synthetic WM_ACTIVATE messages. It sends the message because it is not normally received by child windows and some of our code depends on it. When that message gets into the default window proc, the parent window is activated momentarily and we get WM_KILLFOCUS (when the parent window is focused) & WM_SETFOCUS (when the child window regains focus). So it seems to be some weird Windows behavior that happens only with our specific environment.

Luckily we have a workaround for this issue; WM_ACTIVATE is only needed by a single piece of code and we can refactor it to not depend on WM_ACTIVATE. We can then remove the troublesome synthetic messages.

Changing AddMousePosEvent and/or AddMouseButtonEvent to update the capture flags (WantMouseCapture and the ..UnlessPopUp variant) would be a minimal fix for the problem in the video, but I still think the functional approach would make more sense from API design point of view. One problem with the global variable is that users of the library would have to be aware that the capture flags can change as a side-effect of some API calls, whereas a return value would be more explicit and a more natural part of the API. It is also very common pattern in UI libraries and event systems in general to have such return value which indicates consumption of the event. Just my two cents.

Unfortunately I can't spend much more time on this; other things need my attention. To sum up, we can work around the biggest issue on our end. Feel free to decide on the best course of action for the issue shown in the video. In the worst case, if nothing is done, we can probably live with it but it would certainly be nice to have an official fix for it.

@petrihakkinen
Copy link
Author

P.S. One more detail: in the video I'm pressing LMB on notepad and RMB on the example app. Not sure if it matters, but that's how I've been reproing it because initially we noticed the problem with a right-click context menu.

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

2 participants