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

Win32 DX11 monitor Work Size desync issue #8415

Open
lailoken opened this issue Feb 19, 2025 · 14 comments
Open

Win32 DX11 monitor Work Size desync issue #8415

lailoken opened this issue Feb 19, 2025 · 14 comments
Labels

Comments

@lailoken
Copy link

Version/Branch of Dear ImGui:

Version 1.91.9, Branch: docking

Back-ends:

imgui_impl_dx11 + imgui_impl_win32

Compiler, OS:

MSVC

Full config/build information:

Dear ImGui 1.91.9 WIP (19184)
--------------------------------
sizeof(size_t): 8, sizeof(ImDrawIdx): 4, sizeof(ImDrawVert): 20
define: __cplusplus=199711
define: IMGUI_DISABLE_OBSOLETE_FUNCTIONS
define: _WIN32
define: _WIN64
define: _MSC_VER=1940
define: _MSVC_LANG=202002
define: IMGUI_HAS_VIEWPORT
define: IMGUI_HAS_DOCK
--------------------------------
io.BackendPlatformName: imgui_impl_win32
io.BackendRendererName: imgui_impl_dx11
io.ConfigFlags: 0x0000C481
 NavEnableKeyboard
 DockingEnable
 ViewportsEnable
 DpiEnableScaleViewports
 DpiEnableScaleFonts
io.ConfigViewportsNoAutoMerge
io.ConfigViewportsNoDefaultParent
io.ConfigDockingWithShift
io.ConfigNavCaptureKeyboard
io.ConfigInputTextCursorBlink
io.ConfigWindowsResizeFromEdges
io.ConfigMemoryCompactTimer = 60.0
io.BackendFlags: 0x00001C0E
 HasMouseCursors
 HasSetMousePos
 PlatformHasViewports
 HasMouseHoveredViewport
 RendererHasVtxOffset
 RendererHasViewports
--------------------------------
io.Fonts: 11 fonts, Flags: 0x00000000, TexSize: 1024,1024
io.DisplaySize: 1795.00,1161.00
io.DisplayFramebufferScale: 1.00,1.00
--------------------------------
style.WindowPadding: 8.00,8.00
style.WindowBorderSize: 1.00
style.FramePadding: 4.00,3.00
style.FrameRounding: 0.00
style.FrameBorderSize: 0.00
style.ItemSpacing: 8.00,4.00
style.ItemInnerSpacing: 4.00,4.00

Details:

I've been seeing some desync issues happening on occasion, and they may or may not all be related to each other.

One of the cases is when working with ImGui over RDP, when switching back one user reported that the mouse showed the hover over controls that were about 20px below where the mouse was (once the window was moved it was correct again).

And another user commented that our hovering app bar was not above the windows taskbar as it usually is, but right over it (and no amount of dragging fixed that).

I believe (perhaps both) these are related to the fact that the monitor layout is different and changes (in several ways). ImGui already handled the WM_DISPLAYCHANGE message, so this confused me. However, something else I saw when switching from and to RDP is that after connecting, Windows goes wild reordering windows (and the task bar) for a few more seconds, thus even though ImGui received the WM_DISPLAYCHANGE shortly after connecting, there may still be additional messages needed.

Indeed, I also saw that resizing the windows task bar did not update ImGui's internal metrics of monitor work sizes real-time as it should, so I added some debugging to see the messages being dispatched.

I'm not sure if it is the correct message, but I also added WM_GETMINMAXINFO to:

    case WM_DISPLAYCHANGE:
    case WM_GETMINMAXINFO:
        bd->WantUpdateMonitors = true;
        return 0;

And this at least solved how ImGui responds to taskbar changes.

I'm not an experienced windows programmer, and if anyone else can see an issue with this, perhaps there is a better message, but I believe this will address that shortfall.

Screenshots/Video:

No response

Minimal, Complete and Verifiable Example code:

No response

@PathogenDavid
Copy link
Contributor

PathogenDavid commented Feb 20, 2025

Oops, I think I knew about this oversight but forgot to report it :( Thanks for the reminder!

WM_GETMINMAXINFO isn't the right message for this as that'll constantly refresh the monitor layout as the window is moved and resized.

My private backend looks for WM_SETTINGCHANGE with wParam == SPI_SETWORKAREA, IE:

case WM_SETTINGCHANGE:
    if (wParam != SPI_SETWORKAREA)
        break;
case WM_DISPLAYCHANGE:
    bd->WantUpdateMonitors = true;
    return 0;

Can you confirm that fixes things for you?

@lailoken
Copy link
Author

Thanks, this seems to work (with a small change)

Initially I did not see any reference to WM_SETTINGCHANGE when resizing my app bar. This is my debug log for reference, I have now also logged the wParam field to see when there is a possible SPI_SETWORKAREA:

Sub  WndProc: WM_WININICHANGE (0x001a) 0x2f 0x0000000000000000 - (0000021451E20F30)
Main WndProc: WM_WININICHANGE (0x001a) 0x0000000000000000
Sub  WndProc: WM_WINDOWPOSCHANGING (0x0046) 0x0 0x00000020856fe6f0 - (0000021451E20F30)
Sub  WndProc: WM_WINDOWPOSCHANGED (0x0047) 0x0 0x00000020856fe6f0 - (0000021451E20F30)
Sub  WndProc: WM_MOVE (0x0003) 0x0 0x0000000002dd0dc8 - (0000021451E20F30)

I notice that:

#define WM_WININICHANGE                 0x001A
#if(WINVER >= 0x0400)
#define WM_SETTINGCHANGE                WM_WININICHANGE
#endif /* WINVER >= 0x0400 */
  • It's there for compatibility, new applications should emit WM_SETTINGCHANGE as per the manual pages.

But I think a more backward compatible version would be to use WM_WININICHANGE for ImGui's purposes:

case WM_WININICHANGE:
    if (wParam != SPI_SETWORKAREA)
        break;
case WM_DISPLAYCHANGE:
    bd->WantUpdateMonitors = true;
    return 0;

This fixes the workarea issues, and may or may not fix the other offset issues users have reported (although I have been unable to reproduce their issues to test).

This is a good addition then nonetheless.

@ocornut ocornut reopened this Feb 20, 2025
@ocornut
Copy link
Owner

ocornut commented Feb 20, 2025

Reopening as this should be in our backend.

@PathogenDavid
Copy link
Contributor

But I think a more backward compatible version would be to use WM_WININICHANGE for ImGui's purposes:

WINVER 0x0400 is Windows NT 4.0 so probably not compatibility worth worrying about. WM_DISPLAYCHANGE itself already needs WINVER >= 0x0400, so the backend already doesn't support Windows 98 😁

@lailoken
Copy link
Author

Yes, WM_SETTINGCHANGE should be good then. 👍

@ocornut ocornut changed the title Win32 DX11 display Work Size desync issue Win32 DX11 monitor Work Size desync issue Feb 21, 2025
@ocornut
Copy link
Owner

ocornut commented Feb 21, 2025

Thank you both for investigating! I have pushed change ea59440 (attributed to David).

TL;DR: work area change may trigger a WM_SETTINGCHANGE's SPI_SETWORKAREA message.

    case WM_DISPLAYCHANGE:
        bd->WantUpdateMonitors = true;
        return 0;
++  case WM_SETTINGCHANGE:
++      if (wParam == SPI_SETWORKAREA)
++         bd->WantUpdateMonitors = true;
++      return 0;
    }

I was curious and noticed neither GLFW neither SDL3 used WM_SETTINGCHANGE for that purpose.
GLFW has a monitor callback so presumably it might want to do the same thing as work area is part of monitor? (@dougbinks ?)
https://github.com/glfw/glfw/blob/e7ea71be039836da3a98cea55ae5569cb5eb885c/src/win32_init.c#L340

        case WM_DISPLAYCHANGE:
            _glfwPollMonitorsWin32();
            break;

For SDL3 it's harder for me to tell. From my point of view which is the same one of an app using SDL, I'm waiting for the following messages to refresh my list of display and work area:

case SDL_EVENT_DISPLAY_ORIENTATION:
case SDL_EVENT_DISPLAY_ADDED:
case SDL_EVENT_DISPLAY_REMOVED:
case SDL_EVENT_DISPLAY_MOVED:
case SDL_EVENT_DISPLAY_CONTENT_SCALE_CHANGED:
{
    bd->WantUpdateMonitors = true;
    return true;
}

And it doesn't look like WM_SETTINGCHANGE's SPI_SETWORKAREA has any particular effect:
https://github.com/libsdl-org/SDL/blob/3293eb1a162d99914c7737b8e7a3ea4d7a97e841/src/video/windows/SDL_windowsevents.c#L2173

   case WM_DISPLAYCHANGE:
    {
        // Reacquire displays if any were added or removed
        WIN_RefreshDisplays(SDL_GetVideoDevice());
    } break;

So even if SDL_GetDisplayUsableBounds() value are not going through a cache (it's not the case for WIN_GetDisplayUsableBounds() at least) i am not sure if user is expected to poll this or if it is worth adding a specific event if there's none, or bundling into an existing event.

Sorry for pinging cc @slouken you might find this useful.

ocornut pushed a commit that referenced this issue Feb 21, 2025
@ocornut ocornut closed this as completed Feb 21, 2025
@dougbinks
Copy link
Contributor

I was curious and noticed neither GLFW neither SDL3 used WM_SETTINGCHANGE for that purpose.
GLFW has a monitor callback so presumably it might want to do the same thing as work area is part of monitor

GLFW doesn't have a work area callback, only a work area query. Maximized windows automatically get resized by the OS with WM_SIZE, and mouse coordinates do not depend on monitor work area. As far as I can see changing the work area doesn't cause issues for ImGui on GLFW, though I've only tested by auto-hiding the Taskbar so far.

So at the moment there's no reason to add WM_SETTINGCHANGE event handling to GLFW unless an issue crops up.

@ocornut
Copy link
Owner

ocornut commented Feb 21, 2025

To clarify, the patch we applied was specifically for this sub-issue:

I also saw that resizing the windows task bar did not update ImGui's internal metrics of monitor work sizes real-time as it should, so I added some debugging to see the messages being dispatched.

And I brain-farted that there was a wider issue in this topic (which, to clarify, has nothing to do with GLFW or SDL) so it was hasty to me to close, reopening.

GLFW doesn't have a work area callback, only a work area query

Indeed. But as glfwGetMonitorWorkarea() takes monitor as input it seems to be a property of the monitor, from user's point of view.

@dougbinks
Copy link
Contributor

dougbinks commented Feb 21, 2025

Indeed. But as glfwGetMonitorWorkarea() takes monitor as input it seems to be a property of the monitor, from user's point of view.

I agree it's a property of a monitor, but the GLFW monitor callback is for connect/disconnect events rather than property changes. At the moment we have a number of monitor properties which can be polled, but we don't have events for them changing. Adding callbacks for these property changing is a good idea, but it's a pretty large endeavour given the number of properties and OSs to cover.

I've added an issue to track this:
glfw/glfw#2691

@ocornut
Copy link
Owner

ocornut commented Feb 21, 2025

Just to clarify i can totally do without. We could rework backends to update work area once a frame and call it a day. I merely wanted to tag you for your thoughts. But you are right it may not be glfw nor sdl job to notify of those changes.

@lailoken
Copy link
Author

I just did a text and noticed that our Linux SDL backend did not adjust workarea on Gnome launcher resizing either.
Not a big issue for us, but I think it may also need to be addressed at some point.

@lailoken
Copy link
Author

I've kind of given up on the fidelity of SDL work-area metrics, need to double-check and fall back in my app to compensate for invalid data.

This ticket case-in-point:
libsdl-org/SDL#11407 (comment)

@ocornut
Copy link
Owner

ocornut commented Feb 21, 2025

I think it’s an issue on our end that we can fix in the backend, as no one gave a specific suggestion that those values changing was tied to monitor/display changing.
EDIT Never mind, your issue point to something different.

ocornut added a commit that referenced this issue Feb 21, 2025
…every frame, as the later may change regardless of monitor changes. (#8415)
@ocornut
Copy link
Owner

ocornut commented Feb 21, 2025

Pushed 1a7b594 to update monitors and work area every frames with GLFW, SDL2, SDL3 backends.
Confirmed that it made work area correct when resizing the Windows task-bar.

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

No branches or pull requests

4 participants