Editor crashes on minimising.

[ghost]

Godot version

v4.0.3.stable.official [5222a99]

System information

Windows 11, AMD Ryzen 5 3500U with Radeon Vega Mobile Gfx 2.10 GHz

Issue description

The editor crashes when it is minimised if it isn't the foreground window, eg. via Windows key + M. This didn't occur in 4.0.2, but is now occurring in 4.0.3. I'll just mention that /interface/editor/save_on_focus_loss is false because another issue mentions something like this but with that property set to true.

Steps to reproduce

N/A

Minimal reproduction project

N/A

[saierXP]

I can reproduce it in 4.0.3-rc1, but 4.0.2.stable can't.
And can't reproduce in opengl-compatibility mode.
Sometimes it takes several tries to trigger a crash.

Windows 10.0.19044 integrated AMD Radeon(TM) Vega 8 Graphics (31.0.14051.5006) - AMD Ryzen 5 2500U with Radeon Vega Mobile Gfx (8 Threads)

[ghost]

On my system it crashes consistently every single time in VULKAN mode, but you're right, it doesn't occur in OpenGL compatibility mode.

[jpcerrone]

This issue was introduced in 253b487 . I tested with that commit, where Win+M crashes the program, and with the one before it, where Win+M doesn't crash.

The problem occurs inside the Error VulkanContext::prepare_buffers() function. When minimizing the editor, the _update_swap_chain(w) call doesn't recreate the swap chain, and, in the next iteration of the do-while loop, the empty swap chain generates an exception in fpAcquireNextImageKHR(). This happens because of the break statement that has been removed on the breaking commit. Adding the break statement after the _update_swap_chain(w) call seems to fix the issue.
From what I understand of #72859 , adding the break statement shouldn't alter the desired behaviour intended by the MR. @clayjohn do you think that is the case? If so I can create an MR that adds that break statement back.

[clayjohn]

@jpcerrone If we call break, then the function will fail in cases where the swapchain is actually recreated as we need to acquire the next image.

The problem appears to come from this branch which does an early out from _update_swap_chain() in the case of the minimized window:

godot/drivers/vulkan/vulkan_context.cpp

Lines 1818 to 1822 in 577ab3c

	if (window->width == 0 || window->height == 0) {
	free(presentModes);
	// Likely window minimized, no swapchain created.
	return OK;
	}

It looks like our old behaviour was to just keep a reference to the old swapchain image in the case of minimization. Taking a look around it seems the common solution is just to pause rendering in such cases and then not recreate the swapchain until the window is back (SaschaWillems/Vulkan@1f42dbd, https://vulkan-tutorial.com/Drawing_a_triangle/Swap_chain_recreation#page_Handling-minimization)

Perhaps to keep things simple we can just only call _update_swap_chain(w) when width>0 or height>0

[jpcerrone]

That makes sense, I see why we can't call break.

I tried only calling the _update_swap_chain(w) function with width>0 or height>0 from the Error VulkanContext::prepare_buffers() function, but it looks like at that point, the width and height haven't yet been re-calculated after minimizing, that seems to happen inside the _update_swap_chain(w) function itself. So those aren't yet set to zero.

Maybe we could modify the early out from _update_swap_chain() so that it returns a different error code, since it's not actually updating the swap chain, and check for that outside of it? Something like this:

} else if (err == VK_SUBOPTIMAL_KHR) {
	// Swapchain is not as optimal as it could be, but the platform's
	// presentation engine will still present the image correctly.
	print_verbose("Vulkan: Early suboptimal swapchain.");
	// Updating the swap chain can fail if done during a window minimization. In that case, don't generate the next swapchain image.
	Error update_swap_chain_res = _update_swap_chain(w);
        if (update_swap_chain_res == ERROR_CODE_TO_CHECK){ 
	    break;
        }
}

[clayjohn]

Maybe we could modify the early out from _update_swap_chain() so that it returns a different error code, since it's not actually updating the swap chain, and check for that outside of it? Something like this:

Hmmm, maybe. But by the time the check happens, we will have already deleted the old swapchain.

[jpcerrone]

If the window hasn't been minimized the swapchain will have been re-generated by the time the check happens.

[clayjohn]

If the window hasn't been minimized the swapchain will have been re-generated by the time the check happens.

My concern was more about the fact that if we return then break w->current_buffer will be out of date. But I guess that isn't a huge deal. Perhaps we could just set w->current_buffer to 0 and break

Edit: spending more time with the code, it looks like your initial idea is correct. We can just break after calling _update_swap_chain(w). It looks like anywhere that w->current_buffer is used we ensure that there is a valid swapchain first. So in the case of minimizing, w->current_buffer is never used until the swapchain is recreated. In the case that _update_swap_chain(w) is called but the window is not minimized, then w->current_buffer is set to 0 which should be fine since the swapchain is new

[jpcerrone]

Hmmmm, with "initial idea" do you mean this?:

_update_swap_chain(w);
break;

Or checking for an error code like I mentioned in the last message?:

Error update_swap_chain_res = _update_swap_chain(w);
if (update_swap_chain_res == ERROR_CODE_TO_CHECK){ 
     break;
}

If we go with the first one. Wouldn't we be discarding the new swapchain in the case of the window not minimizing? If we break we won't be calliing fpAcquireNextImageKHR() with it as it is doing now.

[clayjohn]

I meant:

_update_swap_chain(w);
break;

fpAcquireNextImageKHR() returns the index of the next valid image in the swapchain and places it in w->current_buffer (which is just an integer). Since we have recreated the swap chain, it should be fine to start from index 0 which is what we do in other parts of the engine. In the case that the swapchain is never recreated (i.e. the window is minimized), we never read from w->current_buffer anyway, so it doesn't matter if we break.

I misunderstood what fpAcquireNextImageKHR() did and thought it was actually returning a pointer to an image. Accordingly, I was very concerned with keeping the pointer up-to-date. But now that I know better, it seems that the simplest solution was fine all along. My apologies for the confusion

[jpcerrone]

Ohh ok! I wasn't fully aware of how fpAcquireNextImageKHR() worked. It's clear now that breaking shouldn't matter in that case. I'll create an MR with that change on it then.