macOS event dispatching broken

[TheJJ]

#828 figured out that for a successful mac run the event dispatching for mac needs to be changed.
This has to be fixed properly.

[Birch-san]

Useful references…

• The pull request #756 GUI: Also try a separate-shared-context mode where the magic commit was first considered
• The magic commit

#756 (comment):

@Birch-san, we also discussed in this PR some changes to the input handling (in outdated comments): libopenage/gui/guisys/private/gui_application_impl.cpp and libopenage/gui/guisys/private/gui_event_queue_impl.cpp.

Dropped that commit because it was unexplainable. I've put it here:

VelorumS@d577f56

• undo final change of gui-gl-context pull request, to prevent SIGSEGV #828 (comment):

[ChipmunkV] that magic partially disables input handling, so it needs some work: #819 (comment)

• make compiler issues on os x Yosemite. #819 (comment):

[Birch-san] @ChipmunkV, I know you've said that the commit was removed because it was unexplainable. But where did you get the idea to do that? Did you read something somewhere?

[ChipmunkV] @Birch-san, I knew that event loops in Qt use event dispatchers that have OS-specific implementation. Then you just disable the input handling part and try.

I think that the issue is easily fixable by implementing and installing a custom dispatcher with QCoreApplication::setEventDispatcher().

[Birch-san]

I actually had a stab at ChipmunkV's idea (installing a custom dispatcher):

master...Birch-san:set-event-dispatcher

I have no idea what I'm doing. At the time of writing: my custom event dispatcher compiles, and openage runs, but we still have a white screen followed by a black screen.
Edit: I just added some qWarning() lines to check whether any functions of my event dispatcher are being invoked. They are never invoked. setEventDispatcher() needs to be installed before you init the QCoreApplication — and I thought I'd done that — but apparently this isn't the correct way to install an event dispatcher.

A good reference for how to build a custom event dispatcher is here (MIT License, peper0@GitHub):

https://github.com/peper0/qtasio/blob/master/qasioeventdispatcher.cpp
https://github.com/peper0/qtasio/blob/master/qasioeventdispatcher.h

Note: peper0's code includes private headers such as:

#include "private/qabstracteventdispatcher_p.h"
#include "private/qtimerinfo_unix_p.h"

We're not supposed to do that. It has the following message:

This file is not part of the Qt API. It exists purely as an
implementation detail. This header file may change from version to
version without notice, or even be removed.

We mean it.

I think we can live without those private headers anyway. qabstracteventdispatcher_p.h is basically empty. qtimerinfo_unix_p.h is more substantial, but whatever. We may not implement event dispatch in exactly the same way, so not sure how useful it is to us.

[Birch-san]

I also considered "maybe a custom event dispatcher is overkill. could we just filter out events instead?"

master...Birch-san:event-flags

I believe no matter what combination of events that I filtered out: I still ended up with a black screen, and the animated graph never appeared. These are the notes that I took at the time:

WARN QFlags(0x1|0x2|0x4) = no graph
WARN QFlags(0x2|0x4) = no graph
WARN QFlags(0x4) = no graph
WARN QFlags(0x1|0x2) = no graph
WARN QFlags(0x1) = no graph
WARN QFlags(0x2) = no graph
WARN QFlags(0x1|0x4) =

Maybe I should've also tried filters on gui_event_queue_impl.cpp, rather than just on gui_application_impl.cpp.

[Birch-san]

There's also this:

• QCoreApplication::installNativeEventFilter
• QAbstractNativeEventFilter::nativeEventFilter

I wonder if the event dispatch problem is just with some platform-specific events. So maybe we could see some change if we filtered them out.

[Birch-san]

Btw, the magic commit is no panacea. This is a video of what openage looks like with the magic commit:

https://www.youtube.com/watch?v=9GmwKreWg6I

Ignore the blue tint — that's an endianness problem that is now fixed.
Rather, notice that loads of interface elements disappear/reappear all the time.

#756 (comment):

[ChipmunkV] A theory about the disappearing buttons: it's probably related to that input handling fix - some events can't pass through if there is not enough real input activity (so the GUI isn't updated).

[VelorumS]

Btw, I've started the event dispatcher too. And probably I've started from that piece of code where it's created (to make sure that it's created before the QApplication).

The gui_dedicated_thread.cpp that you're changing is a dead code for the future case where the GUI event loop is separated from the rendering thread (sorry about that, it's not a good practice to leave stuff "for the future" on the main branch).

The possible places to attach a dispatcher are: libopenage/gui/integration/public/gui_application_with_logger.cpp (the logger is also attached before the QApplication is created) or libopenage/gui/guisys/private/gui_application_impl.cpp.

About the events: the OS-level event loop is handled by SDL, input events for Qt are synthesized by the openage from the SDL events. There shouldn't be any way for the events from OS to get into Qt.

edit: actually, haven't done much at all - https://github.com/ChipmunkV/openage/commits/event-dispatcher

[Birch-san]

I continued to investigate this.

I've put useful stuff into this branch, you can comment out bits that you don't want to use:

master...Birch-san:event-dispatch-2

The main things that you may find yourself commenting out are:

• gui_application_impl.cpp: installation of the custom event dispatcher (because it's no-op)
  • remove app{(QCoreApplication::setEventDispatcher(&this->event_dispatcher), argc), &argv}
  • prefer app{argc, &argv}
• the logging that is performed in my event filters (because they're noisy; depends if that's what you're testing)
  • remove KeyPressEater.cpp qInfo() << "EVENT: " << event->type();
  • remove CocoaNativeEventFilter.cpp qInfo() << "NATIVE EVENT: " << eventType << "; " << [event type];
• gui_application_impl.cpp: installation of the native event filter (if you're testing this on non-macOS for some reason, and can't compile Objective-C):
  • remove this->app.installNativeEventFilter(&this->native_event_filter);

I've demonstrated here:

• how to install an event filter
  • currently I just use this to log events
• how to install a native macOS event filter
  • currently I just use this to log native events
• what a no-op event dispatcher looks like
  • installed using @ChipmunkV's technique

Things that I noticed…

• When GuiApplicationImpl invokes QGuiApplication#processEvents(): it never finishes.
  • well, maybe this is what it's supposed to do?
• When GuiEventQueueImpl invokes QEventLoop#processEvents(): it never finishes.
  • well, maybe this is what it's supposed to do?
• I tried setting a 100ms maxtime on processEvents: this->app.processEvents(QEventLoop::AllEvents, 100);
• this did not help (the function call still never finished).
• Similarly: I tried toggling various event flags (as I mentioned in an earlier comment), one of which isQEventLoop::WaitForMoreEvents. No combination that I tried did anything. I tried on both processEvents() invocations.
• If either of those classes invokes their processEvents(): we will never again see any events logged in our event filter.
  • probably this means we never process any QTimer events. I assume this is why the graphics don't update.
  • we do continue to see native events (mouse move, keypresses) logged in our native event filter
  • maybe I'm interpreting this incorrectly though. perhaps processEvents() isn't meant to pass events through the filter first?
• I tried filtering out all events and all native events. This did not solve any problems.
  • So this means there's no "bad event" that the event dispatcher fails to handle.
• QThread::currentThread() == QCoreApplication::instance()->thread() is always true, as far as I noticed
  • this logic was used by the "magic commit" to ensure that processEvents() was never run on the QCoreApplication's thread
  • in practice: I think it just means that processEvents() is never run, ever.
  • the reason this helps is because processEvents() seems to be broken in some way with the default macOS event dispatcher.

In case you're interested, this is what I observe when I use the no-op event dispatcher:

• If processEvents() is run: we see neither events, nor native events logged in our event filter
• If processEvents() is not run:
  • we see both events and native events logged in our event filter
  • we see the real-time graph, but nothing else:

So, that's various things that didn't work.

I do caution that writing a custom event dispatcher could be very hard. Here's qt5/qtbase/src/corelib/kernel/qeventdispatcher_cf.mm, which I assume is Qt's default event dispatcher implementation for macOS (i.e. it's the only one that I see written in Objective-C).

I see that Qt5 has the following event dispatcher implementations:

qt5/qtbase/src/corelib/kernel/qeventdispatcher_cf_p.h
qt5/qtbase/src/corelib/kernel/qeventdispatcher_cf.mm
qt5/qtbase/src/corelib/kernel/qeventdispatcher_glib_p.h
qt5/qtbase/src/corelib/kernel/qeventdispatcher_glib.cpp
qt5/qtbase/src/corelib/kernel/qeventdispatcher_unix_p.h
qt5/qtbase/src/corelib/kernel/qeventdispatcher_unix.cpp
qt5/qtbase/src/corelib/kernel/qeventdispatcher_win_p.h
qt5/qtbase/src/corelib/kernel/qeventdispatcher_win.cpp
qt5/qtbase/src/corelib/kernel/qeventdispatcher_winrt_p.h
qt5/qtbase/src/corelib/kernel/qeventdispatcher_winrt.cpp

Maybe we could use the UNIX one or the glib one somehow. But I assume the macOS one exists for a reason.

Some of this stuff is private, so it may be non-trivial to copy-paste. Maybe we can construct an instance and delegate to it?

You can have a look at that source like so:

git clone git://code.qt.io/qt/qt5.git
cd qt5
git checkout v5.8.0
cd qtbase
git submodule update --init --recursive .

But seriously I've no idea what I'm doing.

[Birch-san]

https://doc.qt.io/qt-5/qeventloop.html#exec

More sophisticated idle processing schemes can be achieved using processEvents().

Do we definitely need to use QEventLoop::processEvents()? could we switch to something like QEventLoop::exec()? sounds like it's less complex. if it meets our requirements: perhaps using it would randomize the situation enough that the problem disappears.

[VelorumS]

That's where I'm at: VelorumS@13ad763

The most relevant part is libopenage/gui/guisys/private/gui_timer_info.cpp. Need to make it work so the dispatcher can process timers. That will probably be enough.

QGuiApplication::processEvents, QEventLoop::processEvents and QEventLoop::exec call our QEventDispatcherImpl::processEvents on Linux for me. I'll try your code.

[Birch-san]

There's not really much in my code. All it does is:

• install event filters (I'm only using this for logging)
• install a no-op event dispatcher

But it seems you're significantly past no-op. I'll checkout your commit and see whether it does anything interesting on macOS so far.

[Birch-san]

I encountered this at compile-time:

/Users/birch/git/openage/libopenage/gui/guisys/private/gui_timer_info.cpp:119:6: error: use of undeclared identifier 'QTimerInfoList';
      did you mean 'TimerInfoList'?
void QTimerInfoList::repairTimersIfNeeded()
     ^~~~~~~~~~~~~~
     TimerInfoList

Changed it to TimerInfoList, encountered this SIGSEGV when launching the app. Maybe that's expected behaviour, but reporting it just in case macOS run information is useful at this stage.

[Birch-san]

regarding gui_timer_info.cpp: where does your commented-out implementation come from?

Is it from Qt4.8's qeventdispatcher_unix.cpp?

[VelorumS]

That branch is not runnable, but I think that it should crash in some different way.

The dispatcher is not from the qeventdispatcher_unix.cpp. I've made some socket handling with select() because the application was requesting sockets notifiers.

Then it requested to add timers, so I've taken a qtbase/src/corelib/kernel/qtimerinfo_unix.cpp from the Qt5 implementation. Rewriting it to chrono.

I've launched your version, and it seems to do fine on Linux. The processEvents of the custom dispatcher gets called, GUI works (timers don't work). Also, there are two problems I see: the custom event dispatcher has to be created with new, the int events; in the event dispatcher should be initialized

With this version theQGuiApplication::processEvents()and QEventLoop::processEvents() never finishes on macOS?

P. S. I think that all the code for this change has to be platform-independent.

[Birch-san]

When I make the changes that you recommended (Birch-san/openage@event-dispatch-2...Birch-san:event-dispatch-3):

• QEventDispatcherImpl.cpp: int events = 0;
• gui_application_impl.cpp: app{(QCoreApplication::setEventDispatcher(new QEventDispatcherImpl), argc), &argv}

I find that the game actually works exactly like it does with the magic commit. It even works if I remove these guards if (QThread::currentThread() == QCoreApplication::instance()->thread()) return; from gui_application_impl.cpp and from gui_event_queue_impl.cpp.

processEvents() actually finishes now, and runs in a continuous loop:

WARN Finished processing of local event loop for thread:  QThread(0x7f8b5e6a6ba0)
WARN Processing of GUI application events will continue for thread:  QThread(0x7f8b5e6a6ba0)
WARN processFlags()
WARN Finished processing of GUI application events for thread:  QThread(0x7f8b5e6a6ba0)
WARN Processing of event queue will continue for thread:  QThread(0x7f8b5e6a6ba0)
WARN processFlags()

So… perhaps the no-op event dispatcher is significantly better than I realised.

EDIT: now that I think about it, it makes plenty of sense that the no-op dispatcher works exactly like the magic commit. because both of them have the effect of "don't process events".

EDIT 2: actually, looks like installing the no-op event dispatcher does more than just nothing. If I turn it off, then the only thing we see is a black screen with the performance graph, fps and build details. that's even less than we saw with the magic commit. which means "installing a custom event dispatcher + commenting out calls to processEvents()" renders less than "using default event dispatcher + commenting out calls to processEvents()". it's weird that they're not equivalent. (since I'm not calling processEvents(), I'm surprised it makes a difference which event dispatcher I'm using).

I guess there's still the remaining problem of "UI elements keep disappearing" (just like in the magic commit).

[VelorumS]

Ok, then it's int events; that was causing the loop to block.

If the UI elements still keep disappearing then there is probably a rendering bug with the FBO: it's given to the Engine too early. Whole scenegraph nodes are missing. There is a debug mode to see separate elements: QSG_VISUALIZE=overdraw ./run (http://doc.qt.io/qt-5/qtquick-visualcanvas-scenegraph-renderer.html#visualizing-overdraw)

Maybe glFinish() in GuiRendererImpl::render() in libopenage/gui/guisys/private/gui_renderer_impl.cpp will change something.

But we still can't rule out the processEvent-related cause because the UI elements aren't flickering every frame. They suddenly disappear after the event processing.

[Birch-san]

Wow, glFinish() did the trick!

The UI elements no longer disappear.

[Birch-san]

This is where I put glFinish():

Birch-san/openage@event-dispatch-3...Birch-san:event-dispatch-4

[Birch-san]

I did try the QSG_VISUALIZE=overdraw also.

Whenever any UI elements disappeared: the visualization disappeared also. I also some cases where more UI elements were visible in reality, than were visible in the visualization:

[VelorumS]

Currently both GL contexts are in one thread. Linux drivers are okay if there is no GL synchronization used in this case. But macOS drivers are probably using some intermediate per-context GL command queues, so they need sync in this case too.

Edit: or maybe any FBO needs a sync?

[Birch-san]

But macOS drivers are probably using some intermediate per-context GL command queues, so they need sync in this case too.

Is that what the glFinish() does? Or is there some other problem remaining?

[VelorumS]

Is that what the glFinish() does? Or is there some other problem remaining?

Yes, it does that. Just need to read Internet on how people use FBOs with glFinish.

[Birch-san]

Does the "poor man's event dispatcher" do everything we need it to do? it doesn't implement timers, it doesn't implement socket notifiers. the only thing it really does is call QCoreApplication::sendPostedEvents() until there are no events. also there's logic (events, total_events) I'm pretty sure isn't used.

also it looks like it uses glib:

//QCoreApplicationPrivate::sendPostedEvents(0, 0, d->threadData); //unix dispatcher use this
QCoreApplication::sendPostedEvents(); //glib dispatcher call this after every call of "awake".

Not sure whether sendPostedEvents() is definitely implemented using glib, or if they actually meant "default event dispatch mechanism that Qt provides for your platform".

Is it okay for this to use glib? is that fine for Windows? doesn't seem like we had to add any dependencies to make this work.

[Birch-san]

Maybe a less hacky solution (i.e. one that actually implements a full event dispatcher) would be to find a way to explicitly ask for the glib-based event dispatcher described in qt5/qtbase/src/corelib/kernel/qeventdispatcher_glib_p.h,qeventdispatcher_glib.cpp, rather than allowing Qt to give us the default macOS implementation, (qeventdispatcher_cf.mm, qeventdispatcher_cf_p.h). not sure how though.

[VelorumS]

sendPostedEvents() is delivering events posted with QCoreApplication::postEvent(). For me it was always a pure Qt thing. We're using QCoreApplication::postEvent() to manually pass keyboard/mouse input to Qt (in gui_input_impl.cpp).

There is no API for asking for the platform-specific dispatchers. I think that it's more robust to write a cross-platform implementation of the dispatcher and use it on all platforms.

The goal of the event dispatchers is to get input events from the platform, but we're passing SDL input events through the QCoreApplication::postEvent() hack, which means that our real platform is SDL. We can reimplement gui_input_impl.cpp as an event dispatcher or we can make a timers-sockets-only event dispatcher and leave gui_input_impl.cpp as is.

One last thing that bothers me: when we were doing this things without custom event dispatcher:

#ifndef __APPLE__
 	this->app.processEvents();
#endif
...
#ifdef __APPLE__
	if (QThread::currentThread() != QCoreApplication::instance()->thread())
		this->callback_processor.processEvents();
#else
...

so there are no calls to processEvents() anywhere for __APPLE__. What was processing events on macOS? If the GUI kinda worked that way then something was calling QCoreApplication::sendPostedEvents()?

[Birch-san]

I checked which DTrace probes were exposed for the sendPostedEvents() function:

sudo dtrace -p "$(pgrep run)" -ln 'pid$target:QtCore:*sendPostedEvents*:entry {}'

   ID   PROVIDER            MODULE                          FUNCTION NAME
 8037   pid53854            QtCore QCoreApplication::sendPostedEvents(QObject*, int) entry
 8038   pid53854            QtCore QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) entry

I captured each distinct stack (4 frames deep), and how many times that stack occurred:

sudo dtrace -p "$(pgrep run)" -n 'pid$target:QtCore:*sendPostedEvents*:entry { @[ustack(20)] = count(); }'

When we use the poor-man's event dispatcher, and allow all our calls to processEvents(), we get this variety of stack traces.

When we use the magic commit (i.e. where we do not install a custom event dispatcher, but do delete our call to processEvents() in gui_event_queue_impl.cpp and in gui_application_impl.cpp), we get this variety of stack traces.

And just for fun, here's what it looks like if we do both (i.e. where we install a custom event dispather and delete our calls to processEvents()): stack traces.

EDIT: updated Gists to give you 20 stack frames instead of 4

EDIT 2: In each of these cases: I only launched the app for a second or so. I did not necessarily wait for textures to load, nor for game to begin. but in each case I did confirm that the graph was rendering stats on-screen.

[VelorumS]

From looking at the results of the magic commit, we see that a call through the SDL2 (presumably SDL_PollEvent) gets to the QCoreApplicationPrivate::sendPostedEvents. Qt somehow sneaks a handler into cocoa. I haven't expected it to ever do that.

With event dispatcher and allowing calls to processEvents() it looks sane - all is handled by the QEventDispatcherImpl. The other forgotten QEventLoops that are in separate threads (the RecursiveDirectoryWatcher thread to watch for qml files modifications and some internal thread of QtQml) receive an QEventDispatcherUNIX that does the right thing for them.

So we can fix all by adding glFinish() and a custom event dispatcher for all platforms.

Need to just implement the timers in the dispatcher. Can test it by pasting http://doc.qt.io/qt-5/qml-qtqml-timer.html example somewhere in the openage qml code:

Item {
    Timer {
        interval: 500
        running: true
        repeat: true
        onTriggered: time.text = Date().toString()
    }

    Text { id: time }
}

[Birch-san]

I think we may not need to implement our own timer. I was able to integrate the default QTimerInfoList.

Birch-san/openage@event-dispatch-4...Birch-san:event-dispatch-6

I tried putting that timer UI in a few places. First I tried sane places, but it wasn't visible (maybe default font color is black? tried changing that, but didn't help). Next I put it in progressively more horrifying places until it finally showed up:

The timer updates live, so I assume that confirms that the timer events work.

Sometimes (50% of the time?) no QML ever appears:

The first time I noticed that problem was as of commit Birch-san@3c081a8:

Basically I took the implementation of processEvents() that you provided (which implemented socket notifiers), and added timers by referring to qeventdispatcher_unix.cpp:

https://code.woboq.org/qt5/qtbase/src/corelib/kernel/qeventdispatcher_unix.cpp.html
https://code.woboq.org/qt5/qtbase/src/corelib/kernel/qeventdispatcher_unix_p.h.html

However there are parts of its logic which I skipped. Maybe that explains why the QML sometimes fails to appear.

If we take the approach from my branch: we would become dependent on qtimerinfo_unix_p.h:

https://code.woboq.org/qt5/qtbase/src/corelib/kernel/qtimerinfo_unix_p.h.html
https://code.woboq.org/qt5/qtbase/src/corelib/kernel/qtimerinfo_unix.cpp.html

Also it means incorporating some LGPL3 header code from Qt (maybe this is fine; we are GPL3, which I assume is a superset).

I don't see a version of QTimerInfo for any other platform. Is this why you recommended that we write our own cross-platform implementation? that seems like it would be a reasonably big job. especially since it concerns time: we've had experiences with time code failing to compile on macOS (or worse: failing to compile on slightly old versions of macOS). so a cross-platform timerinfo may be difficult.

Trying to figure out where to go from here. I assume that if I copy qeventdispatcher_unix a bit more closely: there's a chance that it'll fix the disappearing QML. and then we'd have a working event dispatcher for macOS (and for other UNIX). but I assume we'd then break Windows. and I don't immediately see anything we could copy to make Windows work. which suggests maybe the easier solution is to have platform-specific event dispatch (for example: just install this event dispatcher for macOS, or just for UNIX).

What are the repercussions of just using the magic commit + glFinish()? would it mean we simply have no event processing? the game seemed to work despite that.

[VelorumS]

I don't see a version of QTimerInfo for any other platform. Is this why you recommended that we write our own cross-platform implementation?

Other platforms are probably using the OS-specific API to implement timers. The qtimerinfo_unix is a POSIX code that I understand. And I also understand how to rewrite it with C++ std::chrono (which is probably a requirement since, as you've mentioned, Windows is only partially POSIX). About the platform-specific event dispatch: it's easier when the code is the same for everybody.

What are the repercussions of just using the magic commit + glFinish()? would it mean we simply have no event processing?

I haven't tested, but I think that the GUI animations will play instantly or won't play at all. To check that, set the intervalof that Timer in QML to something like 5000ms instead of 500 and see if it really updates every 5 seconds.

[TheJJ]

I think this was now fixed by #995, please test.