Skip to content

Commit

Permalink
Platform: delay GLFW event callback setup to first main loop iteration.
Browse files Browse the repository at this point in the history
A more robust fix for the Windows-specific issue, and fixing a similar
macOS issue as well.
  • Loading branch information
mosra committed Apr 9, 2020
1 parent ad9b9f5 commit 8af1f6a
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 33 deletions.
10 changes: 6 additions & 4 deletions doc/changelog.dox
Original file line number Diff line number Diff line change
Expand Up @@ -430,10 +430,12 @@ See also:
- Fixed broken @ref Platform::EmscriptenApplication mouse event coordinates
when `-s DISABLE_DEPRECATED_FIND_EVENT_TARGET_BEHAVIOR=1` is enabled (see
[mosra/magnum#408](https://github.com/mosra/magnum/issues/408))
- Due to some Windows-specific issue with GLFW,
@ref Platform::GlfwApplication::viewportEvent() could get fired already
during class construction, potentially causing crashes. This is now worked
around by ignoring the very first event.
- Due to Windows- and macOS-specific issues in GLFW,
@ref Platform::GlfwApplication::viewportEvent() and/or
@ref Platform::GlfwApplication::drawEvent() could get fired already during
class construction, potentially causing crashes or
`pure virtual method call` aborts. To prevent these issues, event callback
setup is delayed to the first time the application main loop is entered.
- In 2019.01 @ref Magnum/Platform/Sdl2Application.h went through an include
cleanup, removing 50k lines; but unfortunately we forgot to add back
@cpp #include <SDL_main.h> @ce, causing iOS builds to fail to initialize.
Expand Down
50 changes: 25 additions & 25 deletions src/Magnum/Platform/GlfwApplication.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,18 +58,6 @@ enum class GlfwApplication::Flag: UnsignedByte {
TextInputActive = 1 << 1,
#ifdef CORRADE_TARGET_APPLE
HiDpiWarningPrinted = 1 << 2
#elif defined(CORRADE_TARGET_WINDOWS)
/* On Windows, GLFW fires a viewport event already when creating the
window, which means viewportEvent() gets called even before the
constructor exits. That's not a problem if the window is created
implicitly (because derived class vtable is not setup yet and so the
call goes into the base class no-op viewportEvent()), but when calling
create() / tryCreate() from user constructor, this might lead to crashes
as things touched by viewportEvent() might not be initialized yet. To
fix this, we ignore the first ever viewport event. This behavior was not
observed on Linux or macOS (and thus ignoring the first viewport event
there may be harmful), so keeping this Windows-only. */
FirstViewportEventIgnored = 1 << 2
#endif
};

Expand Down Expand Up @@ -370,9 +358,6 @@ bool GlfwApplication::tryCreate(const Configuration& configuration) {
CORRADE_IGNORE_DEPRECATED_POP
#endif

/* Set callbacks */
setupCallbacks();

return true;
}

Expand Down Expand Up @@ -561,9 +546,6 @@ bool GlfwApplication::tryCreate(const Configuration& configuration, const GLConf
CORRADE_IGNORE_DEPRECATED_POP
#endif

/* Set callbacks */
setupCallbacks();

/* Make the final context current */
glfwMakeContextCurrent(_window);

Expand Down Expand Up @@ -600,13 +582,6 @@ void GlfwApplication::setupCallbacks() {
#endif
(_window, [](GLFWwindow* const window, const int w, const int h) {
auto& app = *static_cast<GlfwApplication*>(glfwGetWindowUserPointer(window));
#ifdef CORRADE_TARGET_WINDOWS
/* See the flag for details */
if(!(app._flags & Flag::FirstViewportEventIgnored)) {
app._flags |= Flag::FirstViewportEventIgnored;
return;
}
#endif
#ifdef MAGNUM_TARGET_GL
ViewportEvent e{app.windowSize(), {w, h}, app.dpiScaling()};
#else
Expand Down Expand Up @@ -728,6 +703,31 @@ int GlfwApplication::exec() {
bool GlfwApplication::mainLoopIteration() {
CORRADE_ASSERT(_window, "Platform::GlfwApplication::mainLoopIteration(): no window opened", {});

/*
If callbacks are not set up yet, do that. Can't be done inside
tryCreate() because:
1. On Windows, GLFW fires a viewport event already when creating the
window, which means viewportEvent() gets called even before the
constructor exits. That's not a problem if the window is created
implicitly (because derived class vtable is not setup yet and so
the call goes into the base class no-op viewportEvent()), but when
calling create() / tryCreate() from user constructor, this might
lead to crashes as things touched by viewportEvent() might not be
initialized yet. To fix this, we ignore the first ever viewport
event. This behavior was not observed on Linux or macOS (and thus
ignoring the first viewport event there may be harmful), so keeping
this Windows-only.
2. On macOS, GLFW might sometimes (hard to reproduce) trigger a draw
event when creating the window. That's even worse than on Windows
because this leads to pure virtual drawEvent() getting called and
the application aborting due to a pure virtual method call in case
GL context is created implicitly by the base class constructor (at
which point the vtable pointers for the derived class are not set
up yet).
*/
if(glfwGetWindowUserPointer(_window) != this) setupCallbacks();

if(_flags & Flag::Redraw) {
_flags &= ~Flag::Redraw;
drawEvent();
Expand Down
4 changes: 0 additions & 4 deletions src/Magnum/Platform/Test/GlfwApplicationTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,6 @@ GlfwApplicationTest::GlfwApplicationTest(const Arguments& arguments): Platform::
conf.setWindowFlags(Configuration::WindowFlag::Resizable);
if(!args.value("dpi-scaling").empty())
conf.setSize({800, 600}, args.value<Vector2>("dpi-scaling"));
/* Creating the window in the constructor exhibits a Windows-specific GLFW
issue where viewportEvent() gets called even before constructor exits;
it's now worked around but the same problem might start occuring on
other platforms as well in the future */
create(conf);

/* For testing resize events */
Expand Down

0 comments on commit 8af1f6a

Please sign in to comment.