Skip to content

fix GL context teardown regression from QOpenGLWidget to QOpenGLWindow #510

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.

Sign up for GitHub

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

Merged
nschimme merged 1 commit into from
Apr 8, 2026
Merged

fix GL context teardown regression from QOpenGLWidget to QOpenGLWindow #510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/display/mapcanvas.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,6 @@ MapCanvas::~MapCanvas()
if (pmc == this) {
pmc = nullptr;
}

cleanupOpenGL();
}

MapCanvas *MapCanvas::getPrimary()
Expand Down
9 changes: 9 additions & 0 deletions src/display/mapcanvas_gl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ void MapCanvas::cleanupOpenGL()
// note: m_batchedMeshes co-owns textures created by MapCanvasData,
// and it also owns the lifetime of some OpenGL objects (e.g. VBOs).
m_batches.resetExistingMeshesAndIgnorePendingRemesh();
m_weather.cleanup();
m_textures.destroyAll();
getGLFont().cleanup();
getOpenGL().cleanup();
Expand Down Expand Up @@ -306,6 +307,14 @@ void MapCanvas::initializeGL()
this->updateTextures();
m_frameManager.requestUpdate();
});

// Clean up GL resources while the context is still current.
// The destructor is too late — Qt destroys the context before ~MapCanvas() runs.
connect(context(),
&QOpenGLContext::aboutToBeDestroyed,
this,
&MapCanvas::cleanupOpenGL,
Qt::DirectConnection);
}

/* Direct means it is always called from the emitter's thread */
Expand Down
1 change: 0 additions & 1 deletion src/mainwindow/mainwindow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@ static void addApplicationFont()

MainWindow::~MainWindow()
{
forceNewFile();
mmqt::rdisconnect(this);
async_tasks::cleanup();
delete m_listener;
Expand Down
1 change: 1 addition & 0 deletions src/opengl/OpenGLTypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,7 @@ class NODISCARD UniqueMesh final
~UniqueMesh() = default;
DEFAULT_MOVES_DELETE_COPIES(UniqueMesh);

void reset() { m_mesh.reset(); }
void render(const GLRenderState &rs) const { deref(m_mesh).render(rs); }
NODISCARD explicit operator bool() const { return m_mesh != nullptr; }
};
Expand Down
8 changes: 8 additions & 0 deletions src/opengl/Weather.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,14 @@ GLWeather::~GLWeather()
m_gl.getUboManager().unregisterRebuildFunction(Legacy::SharedVboEnum::WeatherBlock);
}

void GLWeather::cleanup()
{
m_simulation.reset();
m_particles.reset();
m_atmosphere.reset();
m_timeOfDay.reset();
}

void GLWeather::updateFromGame()
{
switch (m_observer.getWeather()) {
Expand Down
2 changes: 1 addition & 1 deletion src/opengl/Weather.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ class NODISCARD GLWeather final
GameObserver &observer,
FrameManager &frameManager);
~GLWeather();

void cleanup();
DELETE_CTORS_AND_ASSIGN_OPS(GLWeather);

public:
Expand Down
7 changes: 7 additions & 0 deletions src/opengl/legacy/Legacy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include <QDebug>
#include <QFile>
#include <QMessageLogContext>
#include <QOpenGLContext>
#include <QOpenGLExtraFunctions>
#include <QOpenGLTexture>

Expand Down Expand Up @@ -354,6 +355,12 @@ UboManager &Functions::getUboManager()
/// This only exists so we can detect errors in contexts that don't support \c glDebugMessageCallback().
void Functions::checkError()
{
// glGetError() returns GL_INVALID_OPERATION indefinitely when called without a current context
// (e.g. on NVIDIA drivers), causing an infinite loop. Skip the check if no context is current.
if (QOpenGLContext::currentContext() == nullptr) {
return;
}

#define CASE(x) \
case (x): \
qCritical() << "OpenGL error" << #x; \
Expand Down
17 changes: 13 additions & 4 deletions src/opengl/legacy/VBO.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

#include "VBO.h"

#include <QDebug>

namespace Legacy {
bool LOG_VBO_ALLOCATIONS = false;
bool LOG_VBO_STATIC_UPLOADS = false;
Expand All @@ -25,8 +27,11 @@ void VBO::reset()
if (LOG_VBO_ALLOCATIONS) {
qInfo() << this << "Freeing VBO" << vbo;
}
auto sharedFunctions = std::exchange(m_weakFunctions, {}).lock();
deref(sharedFunctions).glDeleteBuffers(1, &vbo);
if (auto sharedFunctions = std::exchange(m_weakFunctions, {}).lock()) {
sharedFunctions->glDeleteBuffers(1, &vbo);
} else {
qCritical() << "Legacy::Functions is no longer valid, leaking VBO" << vbo;
Comment on lines +30 to +33

@sourcery-ai sourcery-ai Bot Apr 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): After logging the leak, consider clearing or marking the VBO handle to avoid repeated leak reports or accidental reuse.

When m_weakFunctions has expired, we log the leak but leave vbo unchanged. If reset() (or a destructor calling it) runs again on the same instance, we’ll log the same leak repeatedly and still hold a stale handle. Consider setting vbo = 0 after logging (and doing the same for shader programs) to prevent repeated critical logs and reduce the risk of future code treating the stale handle as valid.

Suggested implementation:

        if (auto sharedFunctions = std::exchange(m_weakFunctions, {}).lock()) {
            sharedFunctions->glDeleteBuffers(1, &vbo);
        } else {
            qCritical() << "Legacy::Functions is no longer valid, leaking VBO" << vbo;
            vbo = 0;
        }

Apply the same pattern to any similar teardown/reset logic for shader programs (or other GL objects) in this or related files: after logging that the GL functions are unavailable and that a resource will be leaked, explicitly reset the corresponding handle (e.g. program ID) to 0 to avoid repeated leak logs and accidental reuse.

}
}
assert(m_weakFunctions.lock() == nullptr);
}
Expand Down Expand Up @@ -77,8 +82,12 @@ void Program::reset()
if (LOG_VBO_ALLOCATIONS) {
qInfo() << this << "Freeing Shader Program" << program;
}
auto sharedFunctions = std::exchange(m_weakFunctions, {}).lock();
deref(sharedFunctions).glDeleteProgram(program);
if (auto sharedFunctions = std::exchange(m_weakFunctions, {}).lock()) {
sharedFunctions->glDeleteProgram(program);
} else {
qCritical() << "Legacy::Functions is no longer valid, leaking shader program"
<< program;
}
}
assert(m_weakFunctions.lock() == nullptr);
}
Expand Down
Loading