Inconsistent grabbing behavior in demos (click and drag property)

paccerdk commented 9 months ago

There seems to be an issue with the implementation of grabbing behavior across several demos, particularly when this behavior is combined with relative movement (dragging an input value).

The problem primarily arises due to how the grab state (nk_context input.mouse.grab/ungrab) is always reset, even if no event happens, leading to grab/ungrab potentially being missed in event handling, especially noticeable with frameworks like SDL.

Issue Details

The issue occurs with the following sequence of events:

Main Loop, Cycle 1: A mouse down event is registered, invoking nk_input_button which in turn causes nk_property_* to set the grab state to 1 (nk_true) later in main loop.
Main Loop, Cycle 2: No new event is registered (mouse remains pressed), causing the event loop to be skipped, then nk_input_end is called, which resets the grab state to 0 without being handled.

Similarly, The same problem happens with the ungrab state

Impact Details

This is a problem for frameworks that do not automatically generate an event following a mouse down/up action. For instance, the SDL implementation requires two consecutive events for proper grabbing behavior. A scenario where the mouse is pressed down and then moved with more than 1 cycle in-between fails to maintain the grab/ungrab state, unlike when multiple events happen, such as a mouse move together with mouse down event.
Its an even bigger problem when a grab event gets handled, but the ungrab event doesn't, causing the mouse cursor to disappear permanently and staying in relative mode with the SDL demos

Solution

I wouldn't mind providing a pull request, but i'm not sure about the ideal way to implement the solution, maybe someone more familiar with the framework might be able to suggest a more integrated solution than mine?

EDIT: see better solution in https://github.com/Immediate-Mode-UI/Nuklear/issues/608#issuecomment-1933193129

Old solution

A simple way of solving it would be something like the following: ``` bool grab_handled = false; while (running) { /* Input */ SDL_Event evt; nk_input_begin(ctx); while (SDL_PollEvent(&evt)) { if (evt.type == SDL_QUIT) goto cleanup; nk_sdl_handle_event(&evt); grab_handled = true; } if (grab_handled) { nk_input_end(ctx); grab_handled = false; } ... ``` This approach only resets grab / ungrab when its been handled.

I believe the following issues are related: These two has @vurtun mentioning that here is a problem: https://github.com/vurtun/nuklear/issues/203 https://github.com/vurtun/nuklear/issues/631

https://github.com/Immediate-Mode-UI/Nuklear/issues/556 https://github.com/Immediate-Mode-UI/Nuklear/issues/512

RobLoach commented 9 months ago

Which demo are you experiencing this on? The SDL one? To my knowledge, nk_input_end() should be called right after all input states are set.

paccerdk commented 9 months ago

I've experienced the issues with SDL on Linux, but as explained above, this problem is not exclusive to SDL and applies to all demos that manage grab/ungrab state changes in this way.

The demos that handle the grab state changes within the event loop (which may get skipped), can/will lead to missed grab state changes.

I've made an overview of the affected demos:

Demos Incorrectly Handling Grab/Ungrab State (These demos handle grab/ungrab state within the event handling loop):

sdl_opengl2
sdl_opengl3
sdl_opengles2
sdl_renderer
sfml_opengl2
sfml_opengl3
x11
x11_opengl2
x11_opengl3
x11_rawfb
x11_xft

Demos Correctly Implementing Grab/Ungrab State (These demos handle grab/ungrab state per frame, ensuring the state changes won't get missed):

glfw_opengl2
glfw_opengl3
glfw_opengl4
glfw_vulkan

Demos That Do Not Implement Grabbing:

allegro5
d3d11
d3d12
d3d9
gdi
gdi_native_nuklear
gdip
sdl2surface_rawfb
wayland_rawfb

I guess the correct solution would be to move the grab state handling out of the event handling function, and into its own function which should then be called outside of the event loop.

Here is an example:

NK_API void
nk_sdl_handle_grab(void)
{
    struct nk_context *ctx = &sdl.ctx;
    if (ctx->input.mouse.grab) {
        SDL_SetRelativeMouseMode(SDL_TRUE);
    } else if (ctx->input.mouse.ungrab) {
        int x = (int)ctx->input.mouse.prev.x, y = (int)ctx->input.mouse.prev.y;
        SDL_SetRelativeMouseMode(SDL_FALSE);
        SDL_WarpMouseInWindow(sdl.win, x, y);
    }
}

while (running)
{
    SDL_Event evt;
    nk_input_begin(ctx);
    nk_sdl_handle_grab();
    while (SDL_PollEvent(&evt)) {
        if (evt.type == SDL_QUIT) goto cleanup;
        nk_sdl_handle_event(&evt);
    }
    nk_input_end(ctx);
...

This would make the logic equivalent to the glfw demos

I guess this would be a reasonable approach to use fixing the demos and making a pull request?

RobLoach commented 9 months ago

Thanks for the indepth analysis! If you push up a pull request, I'd be happy to test it out on SDL & Linux

paccerdk commented 9 months ago

I've opened a pull request, fixing the issue with the SDL demos. I've done some testing of the sdl_renderer demo on both Linux and Windows with SDL version 2.28.5 and an older 2.0.18, with #define INCLUDE_ALL, and haven't been able to find any issues. The other SDL demos mirror those changes exactly.

Additionally, the cursor is now correctly returning to its exact original coordinates post-grab, (It wasn't before, which was primarily obvious after long drags). This was achieved by:

  ctx->input.mouse.pos.x = ctx->input.mouse.prev.x;
  ctx->input.mouse.pos.y = ctx->input.mouse.prev.y;

(Full function)

```c NK_API void nk_sdl_handle_grab(void) { struct nk_context *ctx = &sdl.ctx; if (ctx->input.mouse.grab) { SDL_SetRelativeMouseMode(SDL_TRUE); } else if (ctx->input.mouse.ungrab) { SDL_SetRelativeMouseMode(SDL_FALSE); SDL_WarpMouseInWindow(sdl.win, (int)ctx->input.mouse.prev.x, (int)ctx->input.mouse.prev.y); } else if (ctx->input.mouse.grabbed) { ctx->input.mouse.pos.x = ctx->input.mouse.prev.x; ctx->input.mouse.pos.y = ctx->input.mouse.prev.y; } } ```

This approach is the same as the handling found in the GLFW demos: nuklear_glfw_gl2.h#L360

This should fix grabbing for the SDL demos and aligns the SDL demo's logic with the GLFW demos.

I can take a look at the remaining sfml and x11 demos if you're happy with the changes of the pull request?

dumblob commented 9 months ago

Thanks @paccerdk ! Really appreciated.

I have commented in your PR and I think it looks good. So feel free to do the same for sfml and x11 demos. If you will not have the bandwidth, just tell us and we will merge the PR as is and/or extend it to sfml and x11 demos too.

paccerdk commented 9 months ago

Sure, i can probably do it within the week - if not, ill let you know Its possible i might need a little help testing, but ill keep you updated either way

paccerdk commented 9 months ago

I had a look at the SFML demos, but it turns out that grabbing isn't actually implemented.

It looks like it was started but never finished. It tries to call setPosition for the mouse on ungrab, but doesn't perform any kind of grabbing.

I have SFML up and running now, so I had a look at implementing grabbing

code

```c NK_API void nk_sfml_handle_grab(void) { struct nk_context* ctx = &sfml.ctx; if (ctx->input.mouse.grab) { sfml.window->setMouseCursorVisible(false); sfml.window->setMouseCursorGrabbed(true); } else if (ctx->input.mouse.ungrab) { sfml.window->setMouseCursorGrabbed(false); sf::Mouse::setPosition(sf::Vector2i((int)ctx->input.mouse.prev.x, (int)ctx->input.mouse.prev.y), *sfml.window); sfml.window->setMouseCursorVisible(true); } else if (ctx->input.mouse.grabbed) { // does not work well with the way mouse movement is currently handled in the event handler // ctx->input.mouse.pos.x = ctx->input.mouse.prev.x; // ctx->input.mouse.pos.y = ctx->input.mouse.prev.y; } } ```

However, it wasn't as straightforward as I was hoping. This is partially because SFML doesn't have support for relative/delta mouse movement, requiring a bit different logic than some of the other implementations, possibly a changed mouse move event handler.

I'll have another go at it, but if it's too problematic, then it might make sense to either just strip the unfinished implementation from the SFML demos or just leave them as is.

In regards to the X11 demos, I had a quick look, and it seems like it should be straightforward to fix those.

paccerdk commented 8 months ago

Looks like https://github.com/Immediate-Mode-UI/Nuklear/pull/609 was merged, that's probably for the best. I'm still working on fixing the rest of the demos, but i ran into a lot more problems than expected.

I believe i have the SFML demos implemented and fixed now, and so is the majority of the X11 demos, however, there's been a lot of inconsistencies, one of the x11 demos generated an event every single cycle, i fixed that, another one of the x11 demos isn't implemented correctly, and the context is not used like in the other demos.

It's going to take a while more, but i'm working on it when i have some spare time and energy.

Would it be ok to keep this issue open?

I'll update with info and make another pull request when its ready.

Immediate-Mode-UI / Nuklear