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

Missing mouse clicks imgui_impl_osx #1992

Closed
marcel303 opened this issue Aug 1, 2018 · 14 comments
Closed

Missing mouse clicks imgui_impl_osx #1992

marcel303 opened this issue Aug 1, 2018 · 14 comments

Comments

@marcel303
Copy link
Contributor

Hi,

There's an issue on Macos where mouse clicks are likely to be missed when using the 'soft click' feature of the touchpad available on Macbooks.

I investigated the issue, and it boils down to 'soft clicks', where the OS simulates a pair of mouse down and mouse up events in response to the user softly tapping the touchpad, generates a pair of events in immediate succession, causing both events to be handled before the next Imgui process call.

Imgui's design (where it only tracks MouseDown = true/false) requires the following order of events to function correctly: MouseDown(true) -> ImguiProcess -> MouseDown(false).

If the order is: MouseDown(true) -> MouseDown(false) -> ImguiProcess, the mouse click is lost.

This is an issue that potentially affects not just Macos, but any type of hardware or platform where mouse up and down events are emulated rather than the result of actual (slow) physical button presses. It could also manifest itself during low frame rate conditions.

I guess the above situations are rare enough not to generally cause issues. But the touchpad issue does affect usability.

I'm opening this issue to track progress. I'm working on a fix for imgui_impl_osx.

Also I want to discuss two things:

  • What would be an acceptable solution? My current solution feels like a workaround. I'm deferring the next event (the mouse up event) until the next Imgui process call. I guess this is the only feasible solution though. So maybe it's ok?
  • Should the solution be integrated for Macos only, or should the workaround for handling mouse up and down events arriving in immediate succession be part of Imgui's core?
@ocornut
Copy link
Owner

ocornut commented Aug 1, 2018

Hello @marcel303,

Thanks for your post. You are right on everything you stated and this is an issue.

The current imgui_impl_glfw.cpp bindings have a specific workaround for that, look for g_MouseJustPressed in the code and the beginning of ImGui_ImplGlfw_UpdateMousePosAndButtons().
https://github.com/ocornut/imgui/blob/master/examples/imgui_impl_glfw.cpp#L57

This is good enough of a workaround and I suggest that (short-term) we implement the same in imgui_impl_osx.mm, if you can make a PR I would appreciate it. If possible you could use the same style as in the imgui_impl_glfw.cpp code for consistency (if not let me know).


The better solution would be to create a system where events would be queued and fed to imgui over multiple frames when events cannot be merged. The nature of imgui internals limits the number of events that can be processed in one frame (which simplify a lot of code in the system), which is why it is taking levels instead events as inputs. The good news is that this hybrid layer doing some merging and some queuing would work great and solve most of our problems.

I believe that some users/engine have this sort of queue in place in their application, but ultimately this system should be part of dear imgui and we could encourage people to transition toward calling the "event" function.

See this draft, if you are curious about what I was envisioning:
https://gist.github.com/ocornut/8417344f3506790304742b07887adf9f

The idea would be that AddMousePosEvent(), AddMouseButtonEvent(), AddMouseWheelEvent() would be members of ImGuiIO.

There's not much work to finish that, maybe I can schedule to finish and integrate this system in the upcoming months.

(Short-term wise, implementing the PR discussed above would be good)

@ocornut ocornut added the osx/ios label Aug 1, 2018
@marcel303
Copy link
Contributor Author

Heya, I looked at ImGui_ImplGlfw_UpdateMousePosAndButtons and it seems like a really simple but effective solution. I think something like this should end up in imgui asap to avoid the same fix/workaround from spreading across implementations, worsening the situation. I'm not really in favor for adding it to the Mac implementation for this reason.

The better solution would be to create a system where events would be queued and fed to imgui over multiple frames when events cannot be merged. The nature of imgui internals limits the number of events that can be processed in one frame (which simplify a lot of code in the system), which is why it is taking levels instead events as inputs. The good news is that this hybrid layer doing some merging and some queuing would work great and solve most of our problems.

Or just add an api for adding events for now and let ImGui decide internally how to deal with it. Which could be a workaround similar to the GLFW implementation short term, and something more elaborate (if called for) later on.
EDIT: After reading your entire reply I see you already propose such an API!

In general, I'm not really in favor of adding more complexity than needed, unless the route is a dead end or eventually leads to more complexity. I think adding an api for adding mouse button and movement events will do. Since keyboards and mice are separate input devices and generate separate input steams, separate events in parallel to each other, and since text input relies on IME already I don't think this event system needs a lot more complexity than what's already in the GLFW implementation. Probably at some point someone will complain about low fps conditions for which mouse clicks are lost, but let's address it then?

I believe that some users/engine have this sort of queue in place in their application, but ultimately this system should be part of dear imgui and we could encourage people to transition toward calling the "event" function.

Yes. I worked on a few triple-A titles and I shoe horned in a very similar solution into two of those games. :-) Those engines weren't using ImGui, but Scaleform/Flash content. But the issues were similar too! A lot of games get made for consoles first and 'forget' about event based controllers like keyboards and mice.

Also, I had a very similar issue in the creative coding library I'm writing. For reasons similar to ImGui (which if I'd have to guess are to avoid event listeners/callbacks because if the horribleness of those, and to replace them with just querying state (changes) directly, 'immediate' style, ..).
https://github.com/marcel303/framework

This is the solution I have in my own lib,
https://pastebin.com/326KDuZZ

As you can tell, it's a lot more complicated. It works under low fps conditions too. It's a separate object which manages the events, since I have multi-window support in there. But it's also a lot of code, so perhaps for ImGui first try the GLFW fix? I can work on that if you want.

Cheers,
Marcel

@ocornut
Copy link
Owner

ocornut commented Aug 7, 2018

Heya, I looked at ImGui_ImplGlfw_UpdateMousePosAndButtons and it seems like a really simple but effective solution.

There must a misunderstanding. By its nature I don't know how the 3-lines fix currently in ImGuiImpl_Glfw can be added to core ImGui without adding a new API.

If we add a new inputs API, that's pretty serious stuff and needs to be carefully designed. I have too many large tasks open and won't have time tackling a new event-driven inputs API short-term.

Meanwhile Mac are the most affected by the issue (and perhaps the only system, as the track-pad on my Windows even without the JustPressed workaround works fine), so surely a 3 lines fix would be the reasonable thing to do short term.

In general, I'm not really in favor of adding more complexity than needed, unless the route is a dead end or eventually leads to more complexity.

??

Since keyboards and mice are separate input devices and generate separate input steams, separate events in parallel to each other, and since text input relies on IME already I don't think this event system needs a lot more complexity than what's already in the GLFW implementation. Probably at some point someone will complain about low fps conditions for which mouse clicks are lost, but let's address it then?

The point of the workaround is to deal with low FPS and this why it was added. The event-based API is just a generalization of that solution on imgui side, and I don't see why it would make anything more complex for the user. It would be just replacing simple io variables assignments with simple functions calls taking the same data.

@marcel303
Copy link
Contributor Author

marcel303 commented Aug 7, 2018

Hey,

I think indeed a misunderstanding arose. I didn't mean to suggest the GLFW fix will work without api changes.

As I see it two things need to be designed,

  1. The ImGui api for letting it know about (mouse) events.
  2. The actual fix ImGui implements to ensure correct behavior by segmenting the event stream over multiple frames.

I agree the api for (1) needs to be designed carefully. The keyboard input/IME remark was intended to narrow down the scope of the api. I think it suffices to only let ImGui know about mouse events through this api. The thing I tried to suggest is that I'm not in favor (in general) of making more elaborate apis than needed, so in this case it wouldn't make sense to send all input events through this api. Just mouse will do I think, unless there is a good reason to do so.

For (2) I can imagine multiple solutions, for which the GLFW workaround is one. The GLFW solution isn't the ultimate solution though.

Possible solutions:

  • Implement the api, without fix. This doesn't change behavior for Macos, but causes a regression for GLFW.
  • Implement the api with GLFW workaround. This 'fixes' the low framerate issue.
  • Implement a more elaborate fix.

When I mentioned I'm not in favor of adding more complexity than currently called for, I meant to implement the GLFW solution at first, without further complexities.

The GLFW fix isn't the ultimate fix though.

It still doesn't work properly under all conditions. It just registers the first mouse click, but if framerate is very low, the user may be pressing twice, or three times, between updates. ImGui in this case will fail to detect double or triple clicks. Also, drag and drop like features will be broken, because those require the event stream to be segmented, and setting the mouse X/Y to the X/Y at the time of the mouse down/mouse release, instead of the latest mouse X/Y. So a more elaborate fix is needed than GLFW to really get rid of all problems. This is what the solution I linked to does. What I meant to suggest, is to not add this much complexity at first, but just aim at the GLFW fix, as it works well enough for most cases, doesn't cause regressions for the GLFW version, and lets us test the api and update existing backends to use it.

@ocornut
Copy link
Owner

ocornut commented Aug 7, 2018

so in this case it wouldn't make sense to send all input events through this api. Just mouse will do I think, unless there is a good reason to do so.

I disagree on this specific strategy. We already have event-ish io.AddInputCharacter() I think it would make sense to go all the add the Keydown/Keyup functions so everything is consistent, it's not exactly more difficult for the user, and it's ok even if it's the first version don't do anything special (yet) - either way they won't break backward or forward compatibility.

If we are going to suggest people to replace code in their bindings better do it at once and be over with it. If we continuously repeatedly requests user to catch up they are less likely to want to keep up to date.

I think we agree on the nature of the fix. I'm just saying i'm very overwhelmed by imgui requests and tasks the moment (trying to get docking to a releasable point) so this is unlikely to make progress soon, hence the validity of the 3 lines workaround in osx.mm which won't harm anyone.

@hsm4
Copy link

hsm4 commented Sep 10, 2018

Hi,
first of all: I'm blown away by the speed and extensiveness of your answers. The solution is a forced repaint after the touchdown event. We do indeed capture all events into an event queue and the problem was indeed that no paint came before the touch-up event. So thanks a lot for your tips.
Having this problem solved I can state that imgui works perfectly on hires touch devices.
You can close this issue.
Thanks.
Marcus

szellmann added a commit to szellmann/visionaray that referenced this issue Sep 22, 2018
Keeps mouse down event from being swallowed when mouse up happens in same frame
@dhiahassen
Copy link

This issue also happens when running imgui emscripten demo on a smartphone or a tablet , this is what I noticed the day before I read this post , what a coincidence

@ocornut
Copy link
Owner

ocornut commented Sep 29, 2018 via email

@dhiahassen
Copy link

dhiahassen commented Sep 29, 2018

This demo https://pbrfrat.com/post/imgui_in_browser.html

screenshot_20180929-113041
Clicking on a widget only causes the cursor to move to that widget , a clicked widget is only highlighted and not trigered ( see capture ) , mouse clicks cannot be done , ( ofc draging also is not possible , which implies windows can't be moved )

I am running that demo in a tablet ( android 6 ) on the chrome browser ( updated yesterday ) with all webassembly flags enabled

@josht000
Copy link

This issue is technically resolved in example_glfw_opengl using the make build, i.e:

cd example_glfw_opengl2
brew install glfw
make
./example_glwf_opengl2

If I would've actually tried the build with make months ago... nuts.

My 2 cents: we should rename the 'example_apple_opengl2' to something regarding 'reference for xcode setup only' and instruct apple users to the above steps for usable example/demo. This will help reduce the number of issues posted about mac stuff.

@cesss
Copy link

cesss commented May 3, 2020

Has this been fixed? I never hit this using SDL on MacOS, but today I was using the GLUT backend and got this bug... do you recommend any workaround for the GLUT backend?

@ocornut
Copy link
Owner

ocornut commented May 3, 2020 via email

@cesss
Copy link

cesss commented May 3, 2020

I understand, @ocornut, and in fact my backend of choice is SDL2. However, there are situations in which you are dealing with old (and big) code and you need to show results soon, so temporarily using the GLUT backend is saving the day for me, because I can progress and show results now. In coming weeks, I can port this old code tree so that it builds nicely side by side with SDL2, but in this moment GLUT is the only option for progressing without changing anything.

BTW, just in case somebody hits this issue in GLUT, just call glut_display_func() at the end of ImGui_ImplGLUT_MouseFunc() and you are done (yes, glut_display_func() is in main.cpp, so you first need to define it in the header for the GLUT backend.)

ocornut pushed a commit that referenced this issue Jan 17, 2022
…nt() api + updated all Backends. (#4858) (input queue code will be next commit)

Details: note that SDL, OSX and GLFW backends removed recording of MouseJustPressed[] which will be unnecessary with input queue (which is the NEXT commit). (#2787, #1992, #3383, #2525, #1320)
@ocornut
Copy link
Owner

ocornut commented Jan 17, 2022

FYI dear imgui 1.87 WIP now uses a new IO api (e.g. io.AddKeyEvent() io.AddMouseButtonEvent() etc.) which implement an input trickling queue, normally solving this problem everywhere for all types of inputs. We'll monitor feedback and expect bugs to have creeped as we reoverhauled that system.

See e.g.

Also see #4858 for a general overview.

@ocornut ocornut closed this as completed Jan 17, 2022
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

6 participants