napframework / nap

NAP Framework source code
https://nap-framework.tech
Mozilla Public License 2.0
410 stars 23 forks source link

setFullScreen broken on Ubuntu 22.x #33

Closed lshoek closed 2 months ago

lshoek commented 4 months ago

Likely has to do with SDL2. Seems like setting the SDL_WINDOW_FULLSCREEN_DESKTOP flag can cause my system to crash. The error that pops up continuously says that the creation of a swapchain the size of my display failed (1920x1080 -> 2560x1440).

My current workaround is to resize a borderless window to the display size and position it at the top left corner of the display coordinates. Arguably this is what (desktop) fullscreen mode does anyway, but the SDL call is still broken.

Additionally prevent fullscreen requests on Linux:

#ifdef __linux__
                nap::Logger::warn("Fullscreen is disabled on Linux");
#elif
                for (auto* window : mMultiRenderWindow->getWindows())
                    window->toggleFullscreen();
#endif
cklosters commented 4 months ago

Thanks for logging this. I am aware of this issue - which is related to SDL2. NAP always uses the SDL_WINDOW_FULLSCREEN_DESKTOP flag instead of SDL_WINDOW_FULLSCREEN, which results in a regular window being scaled to the bounds of the display with no decorations. Doing that by hand (manually) works well (setting size and position, hiding the border), so I don't know why this call fails.

I remember that I (we) solved it using a hack a long time ago, but forgot the details. I hope this won't happen with SDL3, which is still in development.

cklosters commented 2 months ago

I just merged a change that updates SDL2 to the latest supported platform on Windows (x86-64) and Linux (x86-64, armhf & arm64). This fixes a long outstanding DPI scaling issue with X11 - fractional scaling is now supported. It could be that this issue is also resolved.

cklosters commented 2 months ago

Ok, interesting! Calling RenderWindow::toggleFullscreen on Windows - after the recent SDL2 update - now errors out as well. After a bit of debugging with the vulkan validation layer I figured it has to do with calling SDL::setFullscreen(mSDLWindow, value); out of process, ie: directly when the event is received before before GUIAppEventHandler::process and therefore render.

SDL directly scales the window but hasn't emitted a resize event, causing the swapchain to acquire an image that can't be displayed because it's not of the correct size, triggering the vulkan warning that swapchain and image sizes don't match.

Caching and handling the fullscreen toggle at the beginning of RenderWindow::beginRecording fixes this issue:

    VkCommandBuffer RenderWindow::beginRecording()
    {
        if (mToggleFullscreen)
        {
            bool cur_state = SDL::getFullscreen(mSDLWindow);
            setFullscreen(!cur_state);
            mToggleFullscreen = false;
            mRecreateSwapchain = true;
        }

            // Recreate the entire swapchain when the framebuffer (size or format) no longer matches the existing swapchain .

I might come up with a better solution, but this will propbably fix the issue on Linux as well.

cklosters commented 2 months ago

I might come up with a better solution, but this will propbably fix the issue on Linux as well.

I pushed the fix here: https://github.com/napframework/nap/tree/fullscreen_fix

lshoek commented 2 months ago

I just tested this on Linux (Ubuntu 22.04 LTS) with two windows on two displays and it works like a charm. Also with SDL_WINDOW_FULLSCREEN instead of SDL_WINDOW_FULLSCREEN_DESKTOP in fact. This is really cool as it does not not resize the swap chain image to the display resolution but retains the original resolution as set in the resource and stretches it across the screen. We should consider making this as a feature in nap::RenderWindow. Great find and thanks!

cklosters commented 2 months ago

Good to know, thanks for the validation.

As it turns out, the main culprit was inside RenderWindow::beginRendering(), where it would use the current window bounds instead of the swap chain extend for setting up the render pass, causing the subsequent vulkan render calls to fail when (out of session) the window was resized, because the render-window could be larger than the allocated surface.

By making sure the render area matches the assigned swap-chain0extent we know for sure the render pass will succeed, but updating the display queue could fail because the surface is out of sync with the swap-chain , but that's ok: we take that as a hint for re-creating the swapchain.

I've pushed the latest changes and am validating the build right now.

cklosters commented 2 months ago

The builds pass and i have validated the change on various machines