ocornut / imgui

Dear ImGui: Bloat-free Graphical User interface for C++ with minimal dependencies
MIT License
57.55k stars 9.89k forks source link

WantCaptureMouse updated too late #4991

Closed CarloWood closed 2 years ago

CarloWood commented 2 years ago

Version: 1.87 Branch: master

Back-ends: custom Operating System: linux / XCB

Hi,

I believe there is a problem with WantCaptureMouse (and keyboard etc).

I am using 1.87 with the new io.AddMouseButtonEvent etc. Those functions only add an input event to a queue however, nothing is processed. This means that only after the call to NewFrame() the value of WantCaptureMouse is correct.

At first (naively, but -if you think about it- logical) I tried this (bare with me, I know this is wrong):

  case EventType::button_release:
  case EventType::button_press:
    if (m_imgui.on_mouse_move(x, y))
    {
      if (input_event->button <= 2)
      {
        m_imgui.update_modifiers(input_event->modifier_mask.imgui()); // Calls io.AddKeyModsEvent
        m_imgui.on_mouse_click(input_event->button, active);    // Calls io.AddMouseButtonEvent
      }
      break;
    }
    // Pass button press/release to application here...
    break;

where on_mouse_move(x, y) is implemented as:

bool ImGui::on_mouse_move(int x, int y) const
{
  ImGuiIO& io = GetIO();
  io.AddMousePosEvent(x, y);
  return io.WantCaptureMouse;
}

which erroneously assumes that calling AddMousePosEvent(x, y) immediately updates WantCaptureMouse.

At some point I had to debug something for which I turned off mouse motion notifies to reduce debug output. That would break hovering, but should not break mouse clicks as they are accompanied with coordinates (aka, the code above).

I then discovered that the above code didn't work because WantCaptureMouse isn't updated immediately and despite clicking on -say- an imgui button, I'd not call on_mouse_click.

The FAQ says to always pass mouse events to imgui, so that would look like this:

  case EventType::button_release:
  case EventType::button_press:
    m_imgui.on_mouse_move(x, y);    
    if (input_event->button <= 2)  
    {  
      m_imgui.update_modifiers(input_event->modifier_mask.imgui());  
      m_imgui.on_mouse_click(input_event->button, active);  
    }  
    if (io.WantCaptureMouse)
      break;
    // Pass button press/release to application here...
    break;

Now Dear ImGui gets all mouse events and it would react to the click, but it would still not update WantCaptureMouse in time, causing me to pass the click also to my application.

In fact, if I click inside imgui and outside imgui before a frame happens, then there is no way I can tell if imgui wants me to process the event(s). Lets say I first click outside imgui and then inside. Both have coordinates, so the first will be ignored by imgui and the second accepted and THEN WantCaptureMouse will be set.

If then I process the events AFTER NewFrame() I would pass nothing the my application and the first event would get lost.

Also note that my mouse events come in on a different thread, but I can't pass them to imgui immediately because that would be thread unsafe (and I don't want to lock a mutex while calling NewFrame(), that would block it way too long). So what I had to do is implement my own event queuing - but in a thread-safe way - and then process that queue at the start of the render loop (in the imgui/render thread) (the above code) copying all the events from my queue to imgui's queue. This seems silly enough.

If on top of that I have to call NewFrame() before I can use WantCaptureMouse to decide if the events should also be passed to my application or not... that would require yet another queue :/. It isn't really workable and would still fail - as pointed out above - if multiple clicks came in that don't all have the same WantCaptureMouse associated with it.

I don't know the solution for this; it seems that imgui should process input events immediately to the point that it knows whether or not it is going to consume them or not and report that back on a per-event basis, so that I know if I have to pass it to the application or not. Kinda like a "hey imgui, do you want a mouse click at x,y?" - aka, the first block of code that I pasted. Preferably thread-safe - so I don't have to buffer the input events to make them synchronous :p

ocornut commented 2 years ago

This means that only after the call to NewFrame() the value of WantCaptureMouse is correct.

This has always been the case and has never changed, so I am not sure what your new problem actually is. Do you suggest that something worked differently for you in 1.86 ?

The FAQ says to always pass mouse events to imgui, so that would look like this:

That's the correct code AFAIK.

CarloWood commented 2 years ago

No, but you want me to report the version that I am using :D.

It is my hope however that the API change of 1.87 where all input events are added to a queue through a well defined interface is intended as a prelude to fixing problems like this.

CarloWood commented 2 years ago

The TL;DR of this issue is that I think that imgui should be able to instantly tell me if it wants an input event to be passed to the application, as a return code of the call to AddSomethingEvent.

ocornut commented 2 years ago

No, but you want me to report the version that I am using :D

OK so you are a new user of Dear ImGui, I thought there was a regression related to this. Thanks for clarifying.

Lets say I first click outside imgui and then inside. Both have coordinates, so the first will be ignored by imgui and the second accepted and THEN WantCaptureMouse will be set.

I think it can only happens with a touch device not reporting position before the click (that's a known issue).Are you in this situation?

With a mouse you'll get mouse several move/hover events and imgui will notice you are outside its bounds so I don't understand how you can miss a click? Can you explain?

The TL;DR of this issue is that I think that imgui should be able to instantly tell me if it wants an input event to be passed to the application, as a return code of the call to AddSomethingEvent.

I always think in terms of XY Problem and looking at root issue before deciding on solution, right now you are pushing a solution "Y" but I need to be proven that you have a real problem, so this needs to be clarified.

CarloWood commented 2 years ago

What I did (in my custom backend I suppose you'd call it - but it is also going to be the backbone of input event handling for the application itself) is process XCB notifications (mouse movement, button presses/release, key presses/releases, modifier key changes etc (using XKB)) input this struct for every event:

struct InputEvent
{
  MousePosition mouse_position;
  ModifierMask modifier_mask;
  ButtonsEventType flags;
  union {
    uint32_t keysym;
    uint8_t button;
  };
};

Thus, for every event I know where the event happened (mouse coordinates) what the active (non-consumed) modifier keys are and which mouse buttons are pressed. This includes window leave/enter and focus events.

The struct is a POD (can be copied with memcpy) and is put in a lock-free single-producer/single-consumer ringbuffer. That obviously has a limited size, but if a user manages to produce more than 32 events before the next frame then he has other problems then losing event :D That is - this does not include mere mouse motion events and mouse wheel events: those are simply accumulated and processed as "the latest value" once the frame starts (or the accumulated offset in case of the mouse wheel). This way the size of 32 is MORE than enough.

Then at the start of the render loop the consumer thread consumes the events from this ringbuffer and dispatches/translates them into imgui Add*Event calls. However, apparently I can't know if imgui is going to consume them at that point.

I could hold on to my events and process them again after the call to NewFrame - discarding all of them if the last one was consumed by imgui (aka, WantCaptureMouse was set), but that doesn't feel right. It would be much better if imgui wouldn't delay processing the events and gives more accurate feedback about event consumption.

CarloWood commented 2 years ago

I think it can only happens with a touch device not reporting position before the click (that's a known issue).Are you in this situation?

No

With a mouse you'll get mouse several move/hover events and imgui will notice you are outside its bounds so I don't understand how you can miss a click? Can you explain?

It might not be practical when mouse motion notifies are on and the FPS is high enough. In that case this is a theoretical issue. But I am a perfectionist; so assume either a very low frame rate or assume that hovering has been turned off for some reason (that is: I do not get mouse movement notifications; only the new coordinates at the moment of a click).

If the frame rate is high enough then it is probably not likely that someone clicks outside imgui (an event that has to be processed by the application) AND inside imgui (an event that has to be processed by imgui) and will be bothered by one of the events being lost ;). But theoretically this is possible and that would be the most serious case (events totally getting lost).

But even with a normal frame rate, as pointed out above, you can get event duplication: a single click inside imgui that is processed by both imgui and the application.

CarloWood commented 2 years ago

I don't think there is an XY situation here; I am writing this issue because I think imgui could be improved (and I'm more than willing to do that if you agree); but in terms of problem X that I have, that is simply: how to know if an input event is going to be processed by imgui, or should be passed on to the application? I hope that I've given enough information to clarify this problem above, but if not then please ask for more.

CarloWood commented 2 years ago

Let me describe the problem again - ignoring all the details and possible solutions :P

Say someone manages to move the mouse from outside imgui to a button inside an imgui window and click on it all within one frame; that is: WantCaptureMouse is false - and then an "left mouse button pressed" input event comes in for x,y coordinate "on top of the button".

How should I deal with that, using the existing API?

If I call

io.AddMousePosEvent(x, y);
io.AddMouseButtonEvent(0, true);

then imgui will handle the mouse click (x,y is on top of an imgui button).

but at this point WantCaptureMouse is still false.

Is the intended use of ImGui that I call NewFrame() next and then look at WantCaptureMouse - and if it is false process the above event again, this time passing it to my application?

From the documentation I did get the impression that you want people to do:

io.AddMousePosEvent(x, y);
io.AddMouseButtonEvent(0, true);
if (!io.WantCaptureMouse)
{
  application.process(x, y, 0, true);
}

but that would not work in this case, as WantCaptureMouse is an old value that was not updated by the calls to io.Add*Event.

ocornut commented 2 years ago

You are right but generally speaking:

“ In that case this is a theoretical issue. But I am a perfectionist; “

That’s a good enough reason for me to drop this issue. We have enough real practical issues to be focusing on theorical issues. This hasn’t changed in many years and no one mentioned a practical issue with this. So this is mostly an attention hog.

Even if we called the function to update the hovered window on a MousePos event, we wouldn’t be able to do it reliably for accumulating queued events which are likely in your case of “very slow framerate”, as previously queued events would typically alter the UI state. I don’t think there is an easy and viable solution to this.

CarloWood commented 2 years ago

I'd still like to get an answer to my last comment; how am I expected to process it? Is the last code block the intended way?

ocornut commented 2 years ago

Yes that last block of code is correct.

CarloWood commented 2 years ago

:-/

Oh well, I'll use this for now then.

  case EventType::button_release:
  case EventType::button_press:
    if (m_use_imgui)
    {
      m_imgui.on_mouse_move(x, y);
      if (input_event->button <= 2)
      {
        m_imgui.update_modifiers(input_event->modifier_mask.imgui());
        m_imgui.on_mouse_click(input_event->button, active);
      }
      if (m_imgui.want_capture_mouse())     // Inaccurate; this value corresponds to the mouse position during the *previous* frame.
        break;                              // So it is possible that events are lost or duplicated. FIXME
    }
ocornut commented 2 years ago

That seems correct.

[opinionated] apart from the zero-value added wrapper [/opinionated]

Let us know if you stumble on noticeable real world issues with this. Note that we will eventually work out some mitigation or fix to this either way - related to support for touch devices which are affected with this.

CarloWood commented 2 years ago

zero-value wrapper? I don't understand what code (line) you are referring to :/

ocornut commented 2 years ago

[opinionated] apart from the zero-value added wrapper [/opinionated] zero-value wrapper? I don't understand what code (line) you are referring to :/

That's entirely an opinionated stance unrelated to the issue, but it seems like none of your wrapper function m_imgui.want_capture_mouse(), m_imgui.on_mouse_click() are adding actually value to the software, it's just meticulous wrapping that I like to refer as "zero-value". You are interfacing with a very specific piece of software (Dear ImGui) it's unlikely any of that code is going to magically be reusable for another piece of software. You are just making your life more complicated under the a false perception of correctness.

CarloWood commented 2 years ago

I assume you mean that I am using m_imgui object instead of calling ImGui API directly.

I did that from an Object Oriented point of view, but now you mention it - it seems indeed not add anything, since the called functions do not use any private member of that class; because imgui uses a GLOBAL variable for it's "private" context lol.

CarloWood commented 2 years ago

I wrote that class before I realized that imgui's context was global and ran into the problems when using multiple windows. I fixed that now, and might change this API as well. It seems indeed to add nothing.

CarloWood commented 2 years ago

Oh I just realize what it adds... I did it partly to confine #include to a single TU. If I call ImGui API directly there all kinds of global stuff from imgui has to be included there as well. But that is not enough reason (it would be if this was a header though).