-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Add THREADS_ENABLED
macro in order to compile Godot to run on the main thread
#85939
Add THREADS_ENABLED
macro in order to compile Godot to run on the main thread
#85939
Conversation
ebcd020
to
b0ba444
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work!
I haven't tested yet, only did a quick code quality review.
54d1ae2
to
3ad3673
Compare
Nice work! Since this adds a THREADS_ENABLED macro, should probably add a |
In fact, it should have CI for both. I'll try to add them to the github-actions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do another pass later, once the existing comments have been addressed. As a general advice, I'd make all the no-threads versions of everything as slim as possible (commentless, without inline macros, etc.).
31364e7
to
0931a47
Compare
26225b4
to
7c4cb14
Compare
AH. I understand now why it was working for me. I was only testing with release templates, and the "run in browser" build was using an old (working) debug template. |
@akien-mga I'm not able to replicate your issue, even with the debug template build up-to-date. I tried erasing non-threads templates, setting the templates in the export settings. But clicking One-click deploy for me works without hiccups for me, even on Firefox or Chrome Incognito mode. |
I tested the current commit (7c4cb14a5ed02a3b1032728ca726dae9fd58faac) and it seems to be working fine for me. I tested:
Both my custom built and the GH Actions artifact seem to be working fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to go!
Wait I spoke too fast again... The template from GHA still doesn't work for me. It still tries to enable SharedArrayBuffer. And my locally built templates lacked |
Yep, local build with So this PR is not working yet for me. For the record, the One-click deploy mode isn't a good way to test the no-threads builds, as the server we spawn has the required headers for SharedArrayBuffer. So to be extremely clear, here's what I'm testing:
Same issue if I use the export template artifact from GitHub Actions instead of my custom build. |
7c4cb14
to
bd70b8e
Compare
@akien-mga @Smooth-E @popcar2 @wojtekmal I found and fixed the bug. (in platform/web/export/export_plugin.cpp:173) #ifdef THREADS_ENABLED
replaces["$GODOT_THREADS_ENABLED"] = "true";
#else
replaces["$GODOT_THREADS_ENABLED"] = "false";
#endif That code was setting the requirements for threads as I replaced that code with the actual value checked in the editor: if (p_preset->get("variant/thread_support")) {
replaces["$GODOT_THREADS_ENABLED"] = "true";
} else {
replaces["$GODOT_THREADS_ENABLED"] = "false";
} |
Great, that would explain why it worked for me last week when I tested with a Linux editor build with threads disabled, just for fun. I'll retest today and merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested successfully! And I don't spot any other misplaced TOOLS_ENABLED
check where it's meant to check the target config, so we should be good to go finally.
Thanks! Amazing work 🎉 🥇 |
nice work! paving the way for our team to eventually move to 4.X! Thanks @adamscott |
Hi, I came from this post, which said to report errors here.
I tried the catburglar-main-thread-samples on Firefox. It worked fine until completing the (1st?) level, when it stopped responding. That's after getting at least $300 and returning to the entrance.
|
@adamscott |
@JunShiozawa This isn't the samples PR. This one is: #91382 |
Context
This PR adds the
THREADS_ENABLED
macro and theuse_threads=yes|no
threads=yes|no
build option.There could be a way to make the
THREADS_ENABLED
modifications run time instead of build time, but the Web platform would still need two separate build targets, one with threads, the other without, due to emscripten limitations.Has the good side-effect to make web builds compatible with macOS / iOS.
Demo
https://adamscott.github.io/truck-town-demo-main-thread/
https://adamscott.github.io/truck-town-demo-main-thread-nosound/ (runs without sound, in order to test performance)
Progress
Threads
File system
Audio
Rendering
Physics
Core
OS.are_thread_enabled()
functionExport
web_compatible
exportGithub
use_threads=no
Misc
Fixes
Fixes #85938
Known issues
Edit
2024-01-12
use_threads=yes|no
tothreads=yes|no
.Production edit: closes godotengine/godot-roadmap#11