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

Use surface extents for swap chain extents. #98780

Closed

Conversation

tdaven
Copy link
Contributor

@tdaven tdaven commented Nov 2, 2024

Fixes #98779

This PR just uses the calculated surface extents instead of the current extents which are sometimes the special value UINT32_MAX.

@tdaven tdaven requested a review from a team as a code owner November 2, 2024 23:08
@@ -2991,7 +2989,7 @@ Error RenderingDeviceDriverVulkan::swap_chain_resize(CommandQueueID p_cmd_queue,
swap_create_info.minImageCount = desired_swapchain_images;
swap_create_info.imageFormat = swap_chain->format;
swap_create_info.imageColorSpace = swap_chain->color_space;
swap_create_info.imageExtent = native_display_size;
swap_create_info.imageExtent = extent;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Line 2901 would handle the case of UINT32_MAX extents from GetPhysicalDeviceSurfaceCapabilitiesKHR that means "indicating that the surface size will be determined by the extent of a swapchain targeting the surface". This would be stored to native_display_size but then not updated when the others are handled.

Since native_display_size wasn't used anywhere else it was simple to just use the calculated extents instead.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

I confirm this fixes the linked issue for me.

Still needs an OK from the @godotengine/rendering team.
CC @darksylinc

@darksylinc
Copy link
Contributor

Hold off the merge for a little bit. I need to check if it's correct.

The main issue is that on Android we will often get swap( width, height ) (because the swapchain is rotated 90°) and eyeballing the patch it seems that will break. I need to look into this further.

@Alex2782
Copy link
Member

Alex2782 commented Nov 5, 2024

Last week a forum user reported this error, Android Editor swappy=yes. But with swappy=no it stopped occurring.
(I couldn't reproduce this error) at line 2886

image

@darksylinc
Copy link
Contributor

Hi

Last week a forum user reported this error, Android Editor swappy=yes. But with swappy=no it stopped occurring.

That problem is unrelated to this PR, please open a new ticket providing all the requested info so we can make a blocklist to disable swappy for that particular device(s).

darksylinc pushed a commit to darksylinc/godot that referenced this pull request Nov 5, 2024
Wayland in particular sets surface_capabilities.currentExtent.width to
the special value 0xFFFFFFFF, which is valid per spec.
Fixes godotengine#98779

It may also fix misc issues when resizing on all platforms.

Superseedes PR godotengine#98780 , thanks to user tdaven for the original patch.
PR godotengine#98780 would break Android support as it did not account that width
and height might need to be swapped.

Replaced manual swap by Godot's SWAP(), which indicates intention much
easier.
@darksylinc
Copy link
Contributor

This PR was fine, but I opened #98852 as there were a few details I detected while reviewing it.

@akien-mga
Copy link
Member

Superseded by #98852. Thanks for the contribution!

@akien-mga akien-mga closed this Nov 5, 2024
@akien-mga akien-mga removed this from the 4.4 milestone Nov 5, 2024
@tdaven tdaven deleted the fix-swapchain-resize-extents branch December 11, 2024 02:39
WhalesState pushed a commit to WhalesState/blazium that referenced this pull request Mar 9, 2025
Wayland in particular sets surface_capabilities.currentExtent.width to
the special value 0xFFFFFFFF, which is valid per spec.
Fixes godotengine#98779

It may also fix misc issues when resizing on all platforms.

Superseedes PR godotengine#98780 , thanks to user tdaven for the original patch.
PR godotengine#98780 would break Android support as it did not account that width
and height might need to be swapped.

Replaced manual swap by Godot's SWAP(), which indicates intention much
easier.
WhalesState pushed a commit to WhalesState/blazium that referenced this pull request Mar 9, 2025
Wayland in particular sets surface_capabilities.currentExtent.width to
the special value 0xFFFFFFFF, which is valid per spec.
Fixes godotengine#98779

It may also fix misc issues when resizing on all platforms.

Superseedes PR godotengine#98780 , thanks to user tdaven for the original patch.
PR godotengine#98780 would break Android support as it did not account that width
and height might need to be swapped.

Replaced manual swap by Godot's SWAP(), which indicates intention much
easier.
WhalesState pushed a commit to WhalesState/blazium that referenced this pull request Mar 9, 2025
Wayland in particular sets surface_capabilities.currentExtent.width to
the special value 0xFFFFFFFF, which is valid per spec.
Fixes godotengine#98779

It may also fix misc issues when resizing on all platforms.

Superseedes PR godotengine#98780 , thanks to user tdaven for the original patch.
PR godotengine#98780 would break Android support as it did not account that width
and height might need to be swapped.

Replaced manual swap by Godot's SWAP(), which indicates intention much
easier.
WhalesState pushed a commit to WhalesState/blazium that referenced this pull request Mar 10, 2025
Wayland in particular sets surface_capabilities.currentExtent.width to
the special value 0xFFFFFFFF, which is valid per spec.
Fixes godotengine#98779

It may also fix misc issues when resizing on all platforms.

Superseedes PR godotengine#98780 , thanks to user tdaven for the original patch.
PR godotengine#98780 would break Android support as it did not account that width
and height might need to be swapped.

Replaced manual swap by Godot's SWAP(), which indicates intention much
easier.
WhalesState pushed a commit to WhalesState/blazium that referenced this pull request Mar 13, 2025
Wayland in particular sets surface_capabilities.currentExtent.width to
the special value 0xFFFFFFFF, which is valid per spec.
Fixes godotengine#98779

It may also fix misc issues when resizing on all platforms.

Superseedes PR godotengine#98780 , thanks to user tdaven for the original patch.
PR godotengine#98780 would break Android support as it did not account that width
and height might need to be swapped.

Replaced manual swap by Godot's SWAP(), which indicates intention much
easier.
WhalesState pushed a commit to WhalesState/blazium that referenced this pull request Mar 13, 2025
Wayland in particular sets surface_capabilities.currentExtent.width to
the special value 0xFFFFFFFF, which is valid per spec.
Fixes godotengine#98779

It may also fix misc issues when resizing on all platforms.

Superseedes PR godotengine#98780 , thanks to user tdaven for the original patch.
PR godotengine#98780 would break Android support as it did not account that width
and height might need to be swapped.

Replaced manual swap by Godot's SWAP(), which indicates intention much
easier.
WhalesState pushed a commit to WhalesState/blazium that referenced this pull request Mar 15, 2025
Wayland in particular sets surface_capabilities.currentExtent.width to
the special value 0xFFFFFFFF, which is valid per spec.
Fixes godotengine#98779

It may also fix misc issues when resizing on all platforms.

Superseedes PR godotengine#98780 , thanks to user tdaven for the original patch.
PR godotengine#98780 would break Android support as it did not account that width
and height might need to be swapped.

Replaced manual swap by Godot's SWAP(), which indicates intention much
easier.
WhalesState pushed a commit to WhalesState/blazium that referenced this pull request Mar 16, 2025
Wayland in particular sets surface_capabilities.currentExtent.width to
the special value 0xFFFFFFFF, which is valid per spec.
Fixes godotengine#98779

It may also fix misc issues when resizing on all platforms.

Superseedes PR godotengine#98780 , thanks to user tdaven for the original patch.
PR godotengine#98780 would break Android support as it did not account that width
and height might need to be swapped.

Replaced manual swap by Godot's SWAP(), which indicates intention much
easier.
WhalesState pushed a commit to WhalesState/blazium that referenced this pull request Mar 20, 2025
Wayland in particular sets surface_capabilities.currentExtent.width to
the special value 0xFFFFFFFF, which is valid per spec.
Fixes godotengine#98779

It may also fix misc issues when resizing on all platforms.

Superseedes PR godotengine#98780 , thanks to user tdaven for the original patch.
PR godotengine#98780 would break Android support as it did not account that width
and height might need to be swapped.

Replaced manual swap by Godot's SWAP(), which indicates intention much
easier.
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.

Failure to create swap chain since swappy additions (Linux Wayland).
5 participants