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

Remove dead code from DisplayServerWindows::window_set_size #86029

Merged
merged 1 commit into from
May 30, 2024

Conversation

0x0ACB
Copy link
Contributor

@0x0ACB 0x0ACB commented Dec 11, 2023

The removed code will in fact just recreate the swapchain with the same parameters it is already created with. While
context_vulkan->window_resize(p_window, w, h); is called with the updated parameters,

VkExtent2D swapchainExtent = _compute_swapchain_extent(surfCapabilities, &window->width, &window->height);
will reset it to the current window size since the window has not yet been resized.

Also MoveWindow is a synchronous call, meaning this line here will be executed before the function returns

context_vulkan->window_resize(window_id, window.width, window.height);

MoveWindow sends the WM_WINDOWPOSCHANGING, WM_WINDOWPOSCHANGED, WM_MOVE, WM_SIZE, and WM_NCCALCSIZE messages to the window.

So currently calling window_set_size will recreate the swapchain twice.

@0x0ACB 0x0ACB requested a review from a team as a code owner December 11, 2023 11:41
@YuriSizov YuriSizov requested review from a team, BastiaanOlij, bruvzg and clayjohn December 11, 2023 12:21
@YuriSizov YuriSizov added this to the 4.3 milestone Dec 11, 2023
@akien-mga akien-mga changed the title remove dead code from DisplayServerWindows::window_set_size Remove dead code from DisplayServerWindows::window_set_size Dec 11, 2023
artnaum added a commit to artnaum/godot that referenced this pull request Apr 25, 2024
@clayjohn clayjohn requested a review from DarioSamo May 27, 2024 23:25
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. window_resize() is just a stub for Windows while window_resize is typically called as a consequence of MoveWindow() (it is only not called when the size is not being changed).

This PR will need a rebase, because context_vulkan was renamed. But it will be a trivial rebase

@0x0ACB 0x0ACB force-pushed the resize_fix branch 2 times, most recently from 7b26a20 to f7e8d63 Compare May 28, 2024 05:17
Comment on lines -1844 to -1851
#if defined(GLES3_ENABLED)
if (gl_manager_native) {
gl_manager_native->window_resize(p_window, w, h);
}
if (gl_manager_angle) {
gl_manager_angle->window_resize(p_window, w, h);
}
#endif
Copy link
Member

@bruvzg bruvzg May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GL resize part (even if it's not really doing anything right now) probably should be moved to WM_WINDOWPOSCHANGED instead of removing it, currently it's only calling RD window_set_sized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, to be fair I didn't retest this, but when I initially pushed this PR it worked just fine without. And it still should, the window can be resized externally without ever calling window_set_size so the code to handle resizing needs to be already present somewhere else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the Windows GL manager implementation these functions do nothing, so it should work fine without it. But I think it should call it from WM_WINDOWPOSCHANGED anyway, just in case GL manager ever change, and for consistency with other platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see what you mean. Will add it

@0x0ACB 0x0ACB force-pushed the resize_fix branch 2 times, most recently from a55dcb1 to 24a61a0 Compare May 28, 2024 06:24
Copy link
Contributor

@DarioSamo DarioSamo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested, works fine here with no validation errors!

@akien-mga akien-mga merged commit ffad49f into godotengine:master May 30, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

6 participants