[macOS] Replace custom main loop with [NSApp run] and CFRunLoop observer.#104397
[macOS] Replace custom main loop with [NSApp run] and CFRunLoop observer.#104397Repiteo merged 1 commit intogodotengine:masterfrom
[NSApp run] and CFRunLoop observer.#104397Conversation
stuartcarnie
left a comment
There was a problem hiding this comment.
Not sure how much of this is still experimentation, but I added a few comments to suggest some alternatives that utilise AppKit's native application lifecycle events.
Could ask what problem this is trying to solve, as perhaps I can help test?
Just not sufficiently tested. But so far I have not encountered any issues with it. The specific issue it's fixing, is the main loop blocking when you open native menu, and this part seems to be working fine. |
2f2ae02 to
eb50822
Compare
stuartcarnie
left a comment
There was a problem hiding this comment.
When I run this with the following arguments:
--screen 1 --editor --rendering-driver metal
The application thinks it is trying to open a file named metal, so it calls:
- (void)application:(NSApplication *)sender openFiles:(NSArray<NSString *> *)filenames {
which launches a new instance of Godot, and that too passes the args, so it launches another, ad nauseam:
Moved init for |
This comment was marked as resolved.
This comment was marked as resolved.
|
I notice with this change, when running with the |
I guess some places in the editor that are blocking main thread still need manual event processing. |
Yes, removing the AppKit message pump from void DisplayServerMacOS::process_events() {
ERR_FAIL_COND(!Thread::is_main_thread());
while (true) {
NSEvent *event = [NSApp
nextEventMatchingMask:NSEventMaskAny
untilDate:[NSDate distantPast]
inMode:NSDefaultRunLoopMode
dequeue:YES];
if (event == nil) {
break;
}
[NSApp sendEvent:event];
}then the window pops up immediately again. |
|
Removing that logic from I can see Windows polls the message pump: godot/platform/windows/display_server_windows.cpp Lines 3679 to 3682 in 10f6c01 and so does Linux, (at least for Wayland) as it processes events on a separate thread to ensure they are continuously consumed. I think that would be a fairly significant and breaking change, but adding that back should be fine. |
|
Yes, I have re-added it back, it's dequeuing events so should not interfere with the same processing done by the |
Great! I'll do a bit more testing, but I think that should resolve any concerns I had. I did find a few other places where this would caused a problem, as it wouldn't have been pumping events, like this confirmation dialog loop that doesn't break: Lines 5635 to 5641 in efc3e3a |
There was a problem hiding this comment.
I haven't observed any issues, and the changes look good. I've created a patch with some recommended improvements:
main_loop_initalizedis equivalent tomain_loop != nullptr, so I removed it- only start the
pre_wait_observerifmain_loopis initialised, so we don't need to check ifmain_loopis initialised via themain_loop_initializedin thepre_wait_observer_cbfunction - If
Main::iteratein thepre_wait_observer_cb, we would setshould_terminateto true and then outside theif, remove the observer. I created aterminatefunction that ensures the callback is removed, so we don't need to checkshould_terminateevery call to thepre_wait_observer_cbfunction, which was redundant
The end result is fewer if branches that make the pre_wait_observer_cb callback code a little easier to follow.
Subject: [PATCH] Runloop improvements
---
Index: platform/macos/os_macos.mm
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/platform/macos/os_macos.mm b/platform/macos/os_macos.mm
--- a/platform/macos/os_macos.mm (revision d4b0e608811dde1eff6f3ae57aa3625dd6091fd2)
+++ b/platform/macos/os_macos.mm (date 1742684604000)
@@ -49,31 +49,20 @@
void OS_MacOS::pre_wait_observer_cb(CFRunLoopObserverRef p_observer, CFRunLoopActivity p_activiy, void *p_context) {
OS_MacOS *os = static_cast<OS_MacOS *>(OS::get_singleton());
- if (os->main_loop_initalized && !os->should_terminate) {
- @autoreleasepool {
- @try {
- if (DisplayServer::get_singleton()) {
- static_cast<DisplayServerMacOS *>(DisplayServer::get_singleton())->process_events(); // Get rid of pending events.
- }
- os->joypad_apple->process_joypads();
+ @autoreleasepool {
+ @try {
+ if (DisplayServer::get_singleton()) {
+ DisplayServer::get_singleton()->process_events(); // Get rid of pending events.
+ }
+ os->joypad_apple->process_joypads();
- if (Main::iteration()) {
- os->should_terminate = true;
- }
- } @catch (NSException *exception) {
- ERR_PRINT("NSException: " + String::utf8([exception reason].UTF8String));
- }
+ if (Main::iteration()) {
+ os->terminate();
+ }
+ } @catch (NSException *exception) {
+ ERR_PRINT("NSException: " + String::utf8([exception reason].UTF8String));
}
}
-
- if (os->should_terminate) {
- CFRunLoopRemoveObserver(CFRunLoopGetCurrent(), static_cast<OS_MacOS *>(OS::get_singleton())->pre_wait_observer, kCFRunLoopCommonModes);
- CFRelease(os->pre_wait_observer);
-
- // Note: use "stop" instead of "terminate" to ensure control is returned to "OS_MacOS::run".
- [NSApp stop:nil];
- [NSApp abortModal];
- }
CFRunLoopWakeUp(CFRunLoopGetCurrent()); // Prevent main loop from sleeping.
}
@@ -839,13 +828,15 @@
return PREFERRED_TEXTURE_FORMAT_S3TC_BPTC;
}
-bool OS_MacOS::run() {
+void OS_MacOS::run() {
[NSApp run];
- if (main_loop && main_loop_initalized) {
+ if (main_loop) {
main_loop->finalize();
+ @autoreleasepool {
+ Main::cleanup();
+ }
}
- return main_loop_initalized;
}
void OS_MacOS::start_main() {
@@ -864,32 +855,38 @@
@autoreleasepool {
main_loop->initialize();
}
- main_loop_initalized = true;
- } else {
- should_terminate = true;
+ pre_wait_observer = CFRunLoopObserverCreate(kCFAllocatorDefault, kCFRunLoopBeforeWaiting, true, 0, &pre_wait_observer_cb, nullptr);
+ CFRunLoopAddObserver(CFRunLoopGetCurrent(), pre_wait_observer, kCFRunLoopCommonModes);
+ return;
}
} else {
set_exit_code(EXIT_FAILURE);
- should_terminate = true;
}
- } else {
- if (err == ERR_HELP) { // Returned by --help and --version, so success.
- set_exit_code(EXIT_SUCCESS);
- } else {
- set_exit_code(EXIT_FAILURE);
- }
- should_terminate = true;
- }
-
- pre_wait_observer = CFRunLoopObserverCreate(kCFAllocatorDefault, kCFRunLoopBeforeWaiting, true, 0, &pre_wait_observer_cb, nullptr);
- CFRunLoopAddObserver(CFRunLoopGetCurrent(), pre_wait_observer, kCFRunLoopCommonModes);
+ } else if (err == ERR_HELP) { // Returned by --help and --version, so success.
+ set_exit_code(EXIT_SUCCESS);
+ } else {
+ set_exit_code(EXIT_FAILURE);
+ }
+
+ terminate();
+}
+
+void OS_MacOS::terminate() {
+ if (pre_wait_observer) {
+ CFRunLoopRemoveObserver(CFRunLoopGetCurrent(), pre_wait_observer, kCFRunLoopCommonModes);
+ CFRelease(pre_wait_observer);
+ pre_wait_observer = nil;
+ }
+
+ should_terminate = true;
+ [NSApp stop:nil];
+ [NSApp abortModal];
}
OS_MacOS::OS_MacOS(const char *p_execpath, int p_argc, char **p_argv) {
execpath = p_execpath;
argc = p_argc;
argv = p_argv;
-
if (is_sandboxed()) {
// Load security-scoped bookmarks, request access, remove stale or invalid bookmarks.
NSArray *bookmarks = [[NSUserDefaults standardUserDefaults] arrayForKey:@"sec_bookmarks"];
Index: platform/macos/godot_main_macos.mm
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/platform/macos/godot_main_macos.mm b/platform/macos/godot_main_macos.mm
--- a/platform/macos/godot_main_macos.mm (revision d4b0e608811dde1eff6f3ae57aa3625dd6091fd2)
+++ b/platform/macos/godot_main_macos.mm (date 1742684555000)
@@ -64,11 +64,7 @@
// We must override main when testing is enabled.
TEST_MAIN_OVERRIDE
- if (os.run()) {
- @autoreleasepool {
- Main::cleanup();
- }
- }
+ os.run();
return os.get_exit_code();
}
Index: platform/macos/os_macos.h
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/platform/macos/os_macos.h b/platform/macos/os_macos.h
--- a/platform/macos/os_macos.h (revision d4b0e608811dde1eff6f3ae57aa3625dd6091fd2)
+++ b/platform/macos/os_macos.h (date 1742684565000)
@@ -46,7 +46,6 @@
id delegate = nullptr;
bool should_terminate = false;
- bool main_loop_initalized = false;
JoypadApple *joypad_apple = nullptr;
@@ -59,7 +58,7 @@
CrashHandler crash_handler;
- CFRunLoopObserverRef pre_wait_observer;
+ CFRunLoopObserverRef pre_wait_observer = nil;
MainLoop *main_loop = nullptr;
@@ -72,6 +71,8 @@
static _FORCE_INLINE_ String get_framework_executable(const String &p_path);
static void pre_wait_observer_cb(CFRunLoopObserverRef p_observer, CFRunLoopActivity p_activiy, void *p_context);
+ void terminate();
+
protected:
virtual void initialize_core() override;
virtual void initialize() override;
@@ -139,7 +140,7 @@
virtual String get_system_ca_certificates() override;
virtual OS::PreferredTextureFormat get_preferred_texture_format() const override;
- bool run(); // Runs macOS native event loop.
+ void run(); // Runs macOS native event loop.
void start_main(); // Initializes and runs Godot main loop.
bool os_should_terminate() const { return should_terminate; }
int get_cmd_argc() const { return argc; }|
Changed to the draft for now, since there seems to some issues with closing the windows (sometimes it only reacts to close button after a second click, and window stay open for excessively long time).
I guess related to |
I'll test and experiment more – hadn't noticed and issues, but perhaps I was pressing ⌘+Q rather than hitting close button |
|
Added following changes:
|
|
Will give it a try in my morning and let you know |
stuartcarnie
left a comment
There was a problem hiding this comment.
Looks great, @bruvzg – tried various ways to break it without issue. Nice work 👏🏻
|
Thanks! |
Replaces custom main loop with standard
[NSApp run]loop, andCFRunLoopobserver hook. Should prevent main loop from getting block while using native menu/native popups.Might fix window activation issues when running from console, and required to properly support native macOS menus in #76829.