imgui-sfml icon indicating copy to clipboard operation
imgui-sfml copied to clipboard

Race condition with Window.hasFocus()

Open danieljpetersen opened this issue 2 years ago • 5 comments

imgui-sfml relies on the false assumption that Window.hasFocus() will report true immediately after creation. Unfortunately, there is a short period of time after window creation where Window.hasFocus() is not yet true. This is the case for both macOS, Windows, and likely Linux though not confirmed there.

This results in an intermittent bug where input such as mouse clicks will be skipped / not recorded.

This was an issue reported by l04 on the SFML Discord. You can find a (spread out making it somewhat hard to follow) conversation about the issue starting here, https://discord.com/channels/175298431294636032/175298431294636032/972715469310148658

Quoting l04 here

In the constructor of WindowContext, at line 217 of imgui-SFML.cpp, the statement windowHasFocus = window->hasFocus() is called.

However, the window might not have the focus yet, so it returns false.

Then in the function ImGui::SFML::ProcessEvent, the events LostFocus and GainedFocus are checked.

However, the window might have got the focus before calling the function, so it will not register the GainedFocus event. In this case WindowContext is stuck with windowHasFocus = false until you manually generate the GainedFocus event. If I replace line 217 of imgui-SFML.cpp with windowHasFocus = true, I can't reproduce the error anymore. Maybe it needs a better way to handle the initial focus?

A simple fix would be to set the windowHasFocus variable to true immediately on init rather than checking with the function call, though this is admittedly an ugly fix and I'm not sure that this is applicable in all cases. For example in the case of multiple windows.

danieljpetersen avatar May 08 '22 18:05 danieljpetersen

I didn't fully understand the symptoms.

This is the important part:

there is a short period of time after window creation where Window.hasFocus() is not yet true

This results in an intermittent bug where input such as mouse clicks will be skipped / not recorded.

Not recorded for how long, or until what happens? How haven't others noticed this?

oprypin avatar May 08 '22 20:05 oprypin

Apologies, I should have been more clear.

I didn't fully understand the symptoms.

When this bug is triggered, most sf::Events will not be forwarded to imgui until the user does something to cause an sf::Event::LostFocus, and then an sf::Event::GainedFocus.

In other words, when this bug is triggered the user needs to do something like click outside of the window and then click back onto the window, or alt tab in order to get events to propagate from sfml to imgui.

The following events are not forwarded to imgui when the bug is triggered:

sf::Event::MouseMoved sf::Event::MouseButtonPressed sf::Event::MouseButtonReleased sf::Event::TouchBegin sf::Event::TouchEnd sf::Event::MouseWheelScrolled: sf::Event::KeyPressed: sf::Event::KeyReleased: sf::Event::TextEntered: sf::Event::JoystickDisconnected:

This is because these events are all wrapped an an if statement checking whether WindowContext.windowHasFocus is true. Check the file imgui-SFML.cpp in the function processEvent on line 556 to see.


Not recorded for how long, or until what happens?

They are not recorded until the user does something to reset the variable windowHasFocus, such as click outside of the window or alt tab.


How haven't others noticed this?

I think it's hard to trigger this on non-M1 macs.

The initial problem was reported by someone with an M1 mac. Another M1 mac user was able to replicate the hasFocus() returning false behavior saying the following

False for the first 2 cycles of my 60 fps program and then true after that until I clicked out of the window. Same results after launching the program a few times. Same behavior using 2.5.1 or 3.0.0-master.

It seems the way you start the app on an M1 mac also plays a role. Quoting discord again

I think there may be a difference in the way macos handle the windows depending on how you run it.

  1. if you simply compile and run it, it will open a terminal window and then open the sfml window.
  2. if you bundle the executable in an application folder structure, it does not open the terminal, only the sfml window. with the first option, early I was getting 50% chance of error, but it varies. idk why. with the second option, so far I have not had the problem when bundling as an app.

A third user on windows also reported that hasFocus can initially report false, though this one is more involved. Apparently if you start a fullscreen window but click in the time between the app launch and the window creation it will initially report false before returning true if I understand correctly. I may have been mistaken when I originally wrote the issue, this may be the only way to trigger this on windows.

I myself cannot replicate Window.hasFocus() -> false on initial creation, but I'm on Linux.

danieljpetersen avatar May 09 '22 02:05 danieljpetersen

So

  • User creates Window.
  • User calls imgui::init(Window)
  • imgui::init sets WindowContext.windowHasFocus with a call to Window->hasFocus();
  • Window->hasFocus() on rare circumstances can return false immediately after creation, but will return true after a few cycles.
  • When this occurs, (Window.hasFocus() != WindowContext.windowHasfocus)
  • When this occurs, most of the events in processEvent do not get fired, because it's relying on the cached windowHasFocus value that does not match the actual value.
  • User must explicitly trigger a lostFocus and then gainFocus to sync variable back to the actual value

danieljpetersen avatar May 09 '22 02:05 danieljpetersen

I feel like it would be easier to just ignore Gain/Lost focus events and let the user handle them manually So, ImGui-SFML will process all events as long as ImGui::SFML::ProcessEvent is called. The the users would be able to check window focus events/status manually and just not call the function when focus is lost.

I feel like this will make ImGui-SFML's code much easier and predictable. What do you think?

eliasdaler avatar May 10 '22 11:05 eliasdaler

I think that makes a lot of sense. Like you said, it'd fix the bug and simplify the code. For me personally (and I imagine others) this code is redundant as I still need to handle suspending the code for the entire app when LostFocus, having imgui suspend itself is like is stopping a problem in a river-fork downstream rather than at the source.

I could be wrong here but it looks like with the way imgui currently suspends itself (with the check for lost focus / gain focus at the end of the processEvent function) the first click back which gains focus will never be processed by imgui. If so then you currently can't click a button when gaining focus with a mouseclick, which may be undesirable behavior for some.

danieljpetersen avatar May 10 '22 16:05 danieljpetersen