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

Race condition with Window.hasFocus() #206

Open
danieljpetersen opened this issue May 8, 2022 · 5 comments
Open

Race condition with Window.hasFocus() #206

danieljpetersen opened this issue May 8, 2022 · 5 comments

Comments

@danieljpetersen
Copy link

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.

@oprypin
Copy link
Member

oprypin commented May 8, 2022

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?

@danieljpetersen
Copy link
Author

danieljpetersen commented May 9, 2022

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
Copy link
Author

danieljpetersen commented May 9, 2022

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

@eliasdaler
Copy link
Contributor

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?

@danieljpetersen
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants