renaudbedard / fez-1.12-issues

A public repository for FEZ 1.12 testers to log issues
9 stars 0 forks source link

Automatic borderless windowed #122

Closed vrubleg closed 8 years ago

vrubleg commented 8 years ago

vrubleg:

BTW, borderless window with native resolution sometimes looks like this: http://veg.by/z/2016-07-20-16-42-40-ee331be2.png It happens when I'm changing fullscreen mode to the borderless windowed mode in the settings just after starting of the fez.exe. Next changing back to fullscreen and to borderless window again works as intended. An idea. Windowed mode could hide border if the window uses native resolution. Maybe in this case borderless windowed mode will not be needed as a separate mode =) Just a little bit of magic.

flibitijibibo:

That offset's probably the result of the border getting removed after we've set the window position. If you switch to another res in borderless, then set the native res while still in borderless, it should center properly. The borderless on windowed fullscreen is a good thought though - I'll have to sit and think on that.

flibitijibibo commented 8 years ago

Dealt with this for the next update - when borderless is enabled and the window size is equal to the display size, we do an extra SetWindowPos call to center the window.

vrubleg commented 8 years ago

It is not fixed in the latest build (27/07).

flibitijibibo commented 8 years ago

Does hitting apply twice without changing any settings affect the window position?

vrubleg commented 8 years ago

Yes. The second apply moves the main window to the right position.

vrubleg commented 8 years ago

What is interesting that the problem occurs only once after game start. For example:

  1. Run fez.exe, open video settings.
  2. Change the Fullscreen to the Borderless Windowed mode, apply. Problem occurs.
  3. Change the Borderless to the Windowed, apply, change to the Fullscreen, apply.
  4. Change the Fullscreen to the Borderless Windowed mode, apply. Problem doesn't occur.

Also the problem occurs on my another PC with Windows 10. So, it is not a rare problem.

Also it interesting what do you think about this:

The borderless on windowed fullscreen is a good thought though - I'll have to sit and think on that.

flibitijibibo commented 8 years ago

Thought about the extra borderless case; I won't be doing the forced borderless on windowed fullscreen, since it's an implicit behavior for exactly one scenario done behind the user's back. It comes off as more of a bug than something intentional. Plus, someone that wants a borderless window can simply ask for it... that's why we wrote that option in!

As for the existing issue, this is less of an issue with FEZ as it is an issue with window managers having absolutely zero respect for what is requested by applications. In short, we can't really fight the WM when it knowingly breaks something behind our backs. And it does break this behind our backs... a fun test:

/* Have a window at 1920x1200 on a 1920x1200 display */
SDL.SDL_SetWindowBordered(window, SDL_FALSE);
int pos = SDL_WINDOWPOS_CENTERED_DISPLAY(
    SDL_GetWindowDisplayIndex(window)
);
SDL_SetWindowPosition(window, pos, pos);

int x, y;
SDL_GetWindowPosition(window, &x, &y);
printf("%d %d\n", x, y);

This block will print 0,0 every time, no matter what we do, no matter how many times we do it, no matter what it actually looks like on screen. You can do it a million times in a loop, min/max/reset/hide/show/add-then-remove-border as much as you want, Windows just Doesn't Care(TM). The only time it finally works is when the WM feels like doing it, which happens to be that exact scenario on Windows. No amount of code will ever fix it, the user has to negotiate with the OS with whatever ritual is asked of them.

If you don't think so, you can try stepping through this in SDL for Windows...

https://hg.libsdl.org/SDL/file/7cbfd97f1430/src/video/windows/SDL_windowswindow.c#l452 https://hg.libsdl.org/SDL/file/7cbfd97f1430/src/video/windows/SDL_windowswindow.c#l81 https://hg.libsdl.org/SDL/file/7cbfd97f1430/src/video/windows/SDL_windowswindow.c#l532

... but your easiest path is to simply trick the OS into actually respecting your request, or figure out a way to make the fullscreen mode work for you.

vrubleg commented 8 years ago

When Borderless Windowed mode is enabled and native resolution is chosen, the game always looks like this: http://veg.by/z/2016-07-20-16-42-40-ee331be2.png (notice free space on the top of the screenshot) after restarting.

It also happens when I'm changing fullscreen mode to the borderless windowed mode in the settings just after starting of the fez.exe. Next changing back to fullscreen and to borderless window again works as intended.

I can easily reproduce this problem on my both PCs (Windows 7 x64 and Windows 10 x32).

It can be fixed. For example, Age of Empires II HD always uses Borderless Windowed Fullscreen by default, and it doesn't have such problem. I'm not sure that it is a bug of Windows. I had implemented similar thing years ago for some other game, and I also hadn't encountered such problem. Maybe Fez, FNA, or SDL do something wrong.

I'm sorry, it is not very convenient to debug the game without the source code. I have to prepare proper environment for being able to compile SDL and FNA, get into this code, etc. Maybe @renaudbedard could run the game on Windows and try to reproduce it? It seems that it will be very easy to reproduce. Maybe it will be not so hard to fix.

I'm not planning to use this mode, so I'm not worrying about myself. I'm just reporting a problem that I had encountered during testing. It seems that Borderless Windowed with native resolution is the only useful side of the Borderless Windowed mode, but it is less useful when it works buggy on most systems.

I would like to mention that I'm not a native English speaker. Some things that can be considered polite in Russian may sound rude in English because of cultural differences. So, sorry if something sounds a little bit rough or too categorical.

vrubleg commented 8 years ago

I have an idea. When you're applying new screen mode, sometimes it is possible to see that value of the Screen Mode changes for a moment to some other value, and just after it returns back to the chosen value. Maybe the game does some unexpected steps while changing the mode and it causes the problem?

vrubleg commented 8 years ago

General Windowed mode also uses same strange positioning when native resolution is used. For the first time it looks like this: http://veg.by/z/2016-08-04-15-59-09-ab853126.png (compare it to my screenshot of the fullscreen borderless windowed mode problem) For the second time it looks like this: http://veg.by/z/2016-08-04-16-02-17-d23b343e.png Or like this: http://veg.by/z/2016-08-04-16-03-16-352b3b76.png

flibitijibibo commented 8 years ago

Yeah, that's the WM fighting us - it originally does what we want, then it pushes us into "acceptable" areas. Your best bet at fixing this is looking at the FULLSCREEN_DESKTOP implementations and seeing if it's feasible to merge it into other modes.

Of course, if this ends up being too hard to implement we could just remove it...

vrubleg commented 8 years ago

I have tested this code:

RECT rect = { 0, 0, 1920, 1200 }; // My native resolution
AdjustWindowRectEx(&rect, GetWindowLong(hwnd, GWL_STYLE), FALSE, GetWindowLong(hwnd, GWL_EXSTYLE));
SetWindowPos(hwnd, HWND_TOP, rect.left, rect.top, rect.right - rect.left, rect.bottom - rect.top, NULL);

And it works perfectly for a borderless window, and for a window with a caption. I have tried it in my test application, not in Fez, just to check if WM will change position of the window to something different that wasn't asked by the program.

flibitijibibo commented 8 years ago

Here are the AdjustWindowRectEx calls if you want to fiddle with them:

https://hg.libsdl.org/SDL/file/e8281b14970c/src/video/windows/SDL_windowswindow.c#l105 https://hg.libsdl.org/SDL/file/e8281b14970c/src/video/windows/SDL_windowswindow.c#l586

vrubleg commented 8 years ago

Ok, I'll try to fix it in the SDL code a bit later.

vrubleg commented 8 years ago

I downloaded the source code of the SDL2 and investigated the problem a little bit.

What happens while starting (Resolution = 1920×1200 (native), Windowed mode):

  1. WIN_CreateWindow is called, SDL_Window * window has these values: x=560 y=360 w=800 h=480.
  2. WIN_SetWindowSize is called, SDL_Window * window has these values: x=560 y=360 w=1920 h=1200.
  3. WIN_SetWindowPosition is called, SDLWindow * window has these values: x=0 y=9 w=1920 h=1182. As you can see, y is not 0 here, and h is wrong. (Actually, WIN_SetWindowSize and WIN_SetWindowPosition are the same, they just call WINSetWindowPositionInternal).

Quick dirty fix:

diff -r e8281b14970c src/video/windows/SDL_windowsevents.c
--- a/src/video/windows/SDL_windowsevents.c     Wed Aug 03 22:39:44 2016 +0200
+++ b/src/video/windows/SDL_windowsevents.c     Thu Aug 04 21:49:04 2016 +0300
@@ -743,7 +743,7 @@
             int x, y;
             int w, h;

-            if (data->initializing || data->in_border_change) {
+            if (data->initializing || data->in_border_change || data->expected_resize) {
                 break;
             }

It prevents unexpected changing of the window->h from 1200 to 1182 in the WM_WINDOWPOSCHANGED message handler during execution of the WIN_SetWindowPositionInternal. It solves the problem, but I don't know SDL enough (acutally, this is the first time when I see it), and it needs deeper investigation to understand why it was written this way. I suspect that my fix disables some needed logic =)

Maybe later...

flibitijibibo commented 8 years ago

Excellent, I'm a regular contributor so I'll run this by the maintainers tomorrow.

flibitijibibo commented 8 years ago

Report has been filed:

https://bugzilla.libsdl.org/show_bug.cgi?id=3404

vrubleg commented 8 years ago

I think that it is not a valid fix. My fix prevents raising of SDL_WINDOWEVENT_MOVED and SDL_WINDOWEVENT_RESIZED events when WIN_SetWindowPositionInternal is used, and maybe some programs rely on this behavior.

    case WM_WINDOWPOSCHANGED:
        {
            RECT rect;
            int x, y;
            int w, h;

            if (data->initializing || data->in_border_change) {
                break;
            }

            if (!GetClientRect(hwnd, &rect) || IsRectEmpty(&rect)) {
                break;
            }
            ClientToScreen(hwnd, (LPPOINT) & rect);
            ClientToScreen(hwnd, (LPPOINT) & rect + 1);

            WIN_UpdateClipCursor(data->window);

            x = rect.left;
            y = rect.top;
            SDL_SendWindowEvent(data->window, SDL_WINDOWEVENT_MOVED, x, y);

            w = rect.right - rect.left;
            h = rect.bottom - rect.top;
            SDL_SendWindowEvent(data->window, SDL_WINDOWEVENT_RESIZED, w, h);
        }
        break;

I investigated a little bit more. The reason of the problem is the GetClientRect(hwnd, &rect) inside the WM_WINDOWPOSCHANGED handler. For some reason this GetClientRect returns invalid rect, and then invalid size is passed to the SDL_WINDOWEVENT_MOVED and SDL_WINDOWEVENT_RESIZED event handlers. And these events overwrite window->x, window->y, window->w, and window->h to new recalculated and invalid values. As the result all next calls of the WIN_SetWindowPositionInternal use wrong size and position of the window.

It seems that it can be workarounded from the Fez. It works nice when a window has no border. So you can set IsBorderlessEXT = true; before ApplyWindowChanges, and set IsBorderlessEXT to needed value after.

Also I have some other idea how to avoid the GetClientRect problem itself, using WINDOWPOS structure that is passed with the WM_WINDOWPOSCHANGED message. I'll try it a bit later.

flibitijibibo commented 8 years ago

Made a test binary that adds an IsBorderlessEXT = true before we apply changes:

http://www.flibitijibibo.com/F12.zip

vrubleg commented 8 years ago

Now it works ok when game starts in the borderless windowed mode. But it is still buggy when switching from the fullscreen to bordlerless windowed in the game settings. Mabye there is another ApplyWindowChanges is used?

flibitijibibo commented 8 years ago

As far as I can tell ApplyChanges is called once only.

The issue we're seeing now is that on leaving fullscreen has other implications on window behavior. Here's what it's like in X11 using Cinnamon, for example:

Windowed->Fullscreen->Borderless:

Borderless->Fullscreen->Borderless

For some inexplicable reason, the exact process of Bordered->Borderless allows us to hack the rules of the DE and move past the taskbars, but ONLY that process. You can NOT, for example, go Fullscreen->Bordered->Borderless in immediate succession, even after pumping events between bordered and borderless, and in fact, adding ANY SetWindowBordered calls, true or false, to any spot in ApplyWindowChanges causes the window to just disappear off the desktop for me.

I'm going to be 100% honest: I think the one case scenario where it does what we want is not what the DE wants, and is a bug on their end. I dunno if I want to endure writing code that depends on a bug...?

Point is, this is still at best something we can work around in SDL; FNA's doing the absolute best it can and FEZ is doing exactly the right thing (that is, setting the window to borderless after applying changes and recentering when it's desktop res). We either fix this in SDL or it doesn't get fixed at all, those are our only two options.

vrubleg commented 8 years ago

I had tried to implement the idea with using WINDOWPOS structure that is passed with the WM_WINDOWPOSCHANGED message. It seems that it helps in all cases. It have to do the same thing that it was in the original code, but without using of the GetClientRect which causes the problem. As far as I can see, this fix shouldn't break any code that may rely on SDL_WINDOWEVENT_MOVED and SDL_WINDOWEVENT_RESIZED events during WIN_SetWindowPositionInternal execution.

diff -r e8281b14970c src/video/windows/SDL_windowsevents.c
--- a/src/video/windows/SDL_windowsevents.c     Wed Aug 03 22:39:44 2016 +0200
+++ b/src/video/windows/SDL_windowsevents.c     Fri Aug 05 18:56:39 2016 +0300
@@ -742,16 +742,31 @@
             RECT rect;
             int x, y;
             int w, h;
-
-            if (data->initializing || data->in_border_change) {
+            DWORD style, exstyle;
+            BOOL menu;
+            LPWINDOWPOS wndpos = (LPWINDOWPOS) lParam;
+
+            if (data->initializing || data->in_border_change || wndpos->flags & (SWP_NOMOVE | SWP_NOSIZE)) {
                 break;
             }

-            if (!GetClientRect(hwnd, &rect) || IsRectEmpty(&rect)) {
+            style = GetWindowLong(hwnd, GWL_STYLE);
+            menu = (style & WS_CHILDWINDOW) ? FALSE : (GetMenu(hwnd) != NULL);
+            exstyle = GetWindowLong(hwnd, GWL_EXSTYLE);
+
+            SetRectEmpty(&rect);
+            if (!AdjustWindowRectEx(&rect, style, menu, exstyle)) {
                 break;
             }
-            ClientToScreen(hwnd, (LPPOINT) & rect);
-            ClientToScreen(hwnd, (LPPOINT) & rect + 1);
+
+            rect.left = wndpos->x - rect.left;
+            rect.top = wndpos->y - rect.top;
+            rect.right = wndpos->cx - wndpos->x - rect.right;
+            rect.bottom = wndpos->cy - wndpos->y - rect.bottom;
+
+            if (IsRectEmpty(&rect)) {
+                break;
+            }

             WIN_UpdateClipCursor(data->window);

The result code looks like this:

    case WM_WINDOWPOSCHANGED:
        {
            RECT rect;
            int x, y;
            int w, h;
            DWORD style, exstyle;
            BOOL menu;
            LPWINDOWPOS wndpos = (LPWINDOWPOS) lParam;

            if (data->initializing || data->in_border_change || wndpos->flags & (SWP_NOMOVE | SWP_NOSIZE)) {
                break;
            }

            style = GetWindowLong(hwnd, GWL_STYLE);
            menu = (style & WS_CHILDWINDOW) ? FALSE : (GetMenu(hwnd) != NULL);
            exstyle = GetWindowLong(hwnd, GWL_EXSTYLE);

            SetRectEmpty(&rect);
            if (!AdjustWindowRectEx(&rect, style, menu, exstyle)) {
                break;
            }

            rect.left = wndpos->x - rect.left;
            rect.top = wndpos->y - rect.top;
            rect.right = wndpos->cx - wndpos->x - rect.right;
            rect.bottom = wndpos->cy - wndpos->y - rect.bottom;

            if (IsRectEmpty(&rect)) {
                break;
            }

            WIN_UpdateClipCursor(data->window);

            x = rect.left;
            y = rect.top;
            SDL_SendWindowEvent(data->window, SDL_WINDOWEVENT_MOVED, x, y);

            w = rect.right - rect.left;
            h = rect.bottom - rect.top;
            SDL_SendWindowEvent(data->window, SDL_WINDOWEVENT_RESIZED, w,
                                h);
        }
        break;

Please remove our previous workaround, it will not be needed with fixed version of the SDL2.

flibitijibibo commented 8 years ago

Interesting - About to try and apply the patch now, noticed that menu goes unused... does it still work when you remove the 2 menu lines? EDIT: Woops forgot to add the parameter to Adjust...

diff -r 19c6504a549d src/video/windows/SDL_windowsevents.c
--- a/src/video/windows/SDL_windowsevents.c Thu May 12 12:44:39 2016 -0400
+++ b/src/video/windows/SDL_windowsevents.c Fri Aug 05 11:26:06 2016 -0400
@@ -742,16 +742,31 @@
             RECT rect;
             int x, y;
             int w, h;
-            
-            if (data->initializing || data->in_border_change) {
+            DWORD style, exstyle;
+            BOOL menu;
+            LPWINDOWPOS wndpos = (LPWINDOWPOS) lParam;
+
+            if (data->initializing || data->in_border_change || wndpos->flags & (SWP_NOMOVE | SWP_NOSIZE)) {
                 break;
             }

-            if (!GetClientRect(hwnd, &rect) || IsRectEmpty(&rect)) {
+            style = GetWindowLong(hwnd, GWL_STYLE);
+            exstyle = GetWindowLong(hwnd, GWL_EXSTYLE);
+            menu = (style & WS_CHILDWINDOW) ? FALSE : (GetMenu(hwnd) != NULL);
+
+            SetRectEmpty(&rect);
+            if (!AdjustWindowRectEx(&rect, style, menu, exstyle)) {
                 break;
             }
-            ClientToScreen(hwnd, (LPPOINT) & rect);
-            ClientToScreen(hwnd, (LPPOINT) & rect + 1);
+
+            rect.left = wndpos->x - rect.left;
+            rect.top = wndpos->y - rect.top;
+            rect.right = wndpos->cx - wndpos->x - rect.right;
+            rect.bottom = wndpos->cy - wndpos->y - rect.bottom;
+            
+            if (IsRectEmpty(&rect)) {
+                break;
+            }

             WIN_UpdateClipCursor(data->window);
vrubleg commented 8 years ago

My mistake. It have to be used in the AdjustWindowRectEx, like this: AdjustWindowRectEx(&rect, style, menu, exstyle). I have fixed my previous post also.

vrubleg commented 8 years ago

Found a bug in my fix. It just skips everything in most cases, so it doesn't work as intended. I'll fix it again soon =)

flibitijibibo commented 8 years ago

Neato. I'll leave the rest of this up to you, as WinAPI is where I'm the least experienced... if you want you can update that Bugzilla thread with patches as you figure out what to do.

vrubleg commented 8 years ago

I had misunderstood documentation about the WM_WINDOWPOSCHANGED, so my previous almost always skips this logic, which is wrong. I had fixed it, and as the result I had the same problem as it was with the GetClientRect. Values are changed somewhere before WM_WINDOWPOSCHANGED. MSDN says that it is WM_WINDOWPOSCHANGING. Default handler of this message checks minimum and maximum window sizes and fixes them when it is needed. We have to return 0 if we would like to prevent it.

So, the final working fix is simple:

diff -r e8281b14970c src/video/windows/SDL_windowsevents.c
--- a/src/video/windows/SDL_windowsevents.c     Wed Aug 03 22:39:44 2016 +0200
+++ b/src/video/windows/SDL_windowsevents.c     Fri Aug 05 20:58:01 2016 +0300
@@ -737,6 +737,13 @@
         break;
 #endif /* WM_GETMINMAXINFO */

+    case WM_WINDOWPOSCHANGING:
+
+        if (data->expected_resize) {
+            returnCode = 0;
+        }
+        break;
+
     case WM_WINDOWPOSCHANGED:
         {
             RECT rect;

Earlier fixes have to be forgotten and erased from your memory =)

vrubleg commented 8 years ago

https://hg.libsdl.org/SDL/rev/3b789a491509

flibitijibibo commented 8 years ago

We'll push this in the next update!

renaudbedard commented 7 years ago

Should be in this latest update :)

vrubleg commented 7 years ago

@renaudbedard also please don't forget this old ticket =) https://github.com/renaudbedard/fez-1.12-issues/issues/120#issuecomment-250940319

renaudbedard commented 7 years ago

Yep that's taken care of as well!