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

[3.x] [macOS] Disable live resize in multithreaded rendering mode. #81442

Merged
merged 1 commit into from
Sep 18, 2023

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Sep 8, 2023

Fixes #81402

Irrelevant for 4.x / Vulkan, 4.x / OpenGL multithreaded mode is crashing in the rasterizer init.

@akien-mga
Copy link
Member

CC @lekoder if you can test.

@akien-mga akien-mga modified the milestones: 3.x, 3.6 Sep 8, 2023
@akien-mga akien-mga added the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Sep 8, 2023
@lekoder
Copy link
Contributor

lekoder commented Sep 9, 2023

Unfortunately, the test machine is provided to me remotely, and I cannot compile Godot on it. Could you provide a pre-compiled 3.5.2 with this patch for me to test?

@akien-mga
Copy link
Member

You can download CI artifacts from any PR in the Checks tab: https://github.com/godotengine/godot/pull/81442/checks

@lekoder
Copy link
Contributor

lekoder commented Sep 10, 2023

It seems to fix the crashing issue. However, 3.6 beta breaks other things in the project - could we get this backported to 3.5.2 (3.5.3?).

@Calinou
Copy link
Member

Calinou commented Sep 10, 2023

It seems to fix the crashing issue. However, 3.6 beta breaks other things in the project - could we get this backported to 3.5.2 (3.5.3?).

This PR already has a cherrypick:3.5 label, so it will be cherry-picked to the 3.5 branch at some point after it's merged.

@bruvzg bruvzg marked this pull request as ready for review September 18, 2023 12:56
@bruvzg bruvzg requested a review from a team as a code owner September 18, 2023 12:56
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.

To be clear, this means game windows using this mode won't be able to resize at runtime, and will require a restart for size change to be taken into account.

Since this seems to be an OpenGL driver bug which Apple is no longer maintaining, it seems to be an acceptable tradeoff to avoid a crash. If anyone wants to look into ways to make window resizing work without crashing in multithreaded mode, feel free to.

@akien-mga akien-mga merged commit 86dee6e into godotengine:3.x Sep 18, 2023
13 checks passed
@akien-mga
Copy link
Member

Thanks!

@lekoder
Copy link
Contributor

lekoder commented Sep 18, 2023

@akien-mga I was under the impression that window resizing worked for me just fine; it just didn't resize the viewport as you dragged the window in realtime, but only after you released the mouse.

@akien-mga
Copy link
Member

Right, that's maybe what bruvzg meant. If so that sounds even more like a fine tradeoff.

@bruvzg
Copy link
Member Author

bruvzg commented Sep 18, 2023

it just didn't resize the viewport as you dragged the window in realtime, but only after you released the mouse.

That's exactly what this PR is disabling (only for multithreaded mode), it was working, but something changed in the macOS, OpenGL is not thread safe by default and enabling thread safe mode will significantly impact performance, so it's not an option, and I have no idea if there's any other way to fix it.

@bruvzg bruvzg deleted the gl_mt branch September 18, 2023 14:14
@akien-mga
Copy link
Member

Cherry-picked for 3.5.3.

@akien-mga akien-mga removed the cherrypick:3.5 Considered for cherry-picking into a future 3.5.x release label Sep 24, 2023
@akien-mga akien-mga changed the title [macOS, 3.x] Disable live resize in multithreaded rendering mode. [3.x] [macOS] Disable live resize in multithreaded rendering mode. Jan 25, 2024
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.

4 participants