fix GL context teardown regression from QOpenGLWidget to QOpenGLWindow - #510
Merged
Conversation
Commit aa07211 migrated MapCanvas from QOpenGLWidget to QOpenGLWindow. Under QOpenGLWidget, the GL context is still current during destruction, so calling cleanupOpenGL() from ~MapCanvas() was safe. Under QOpenGLWindow, the native context is destroyed before C++ destructors run, making any GL call from a destructor operate without a current context. On NVIDIA drivers, glGetError() returns GL_INVALID_OPERATION indefinitely when called without a current context, causing the checkError() loop to spin forever. Two destructor call paths triggered this: 1. MainWindow::~MainWindow() called forceNewFile() -> slot_dataLoaded() -> forceUpdateMeshes(), which created and destroyed GL meshes after the context was gone. 2. GLWeather was not included in cleanupOpenGL(), so its particle meshes were destroyed in ~MapCanvas() after the context was gone. Fix: - Connect QOpenGLContext::aboutToBeDestroyed -> cleanupOpenGL() in initializeGL(), so GL resources are freed while the context is still current. (The call in ~MapCanvas() is removed.) - Add m_weather.cleanup() to cleanupOpenGL(), matching the existing cleanup of m_batches and m_glFont. - Remove forceNewFile() from MainWindow::~MainWindow() to avoid triggering GL mesh creation/destruction during teardown. - Guard checkError() to return early when no context is current, as a safety net against future oversights. - Guard VBO::reset() and Program::reset() to skip GL deletion and log a qCritical when the Functions weak_ptr has expired, consistent with the existing behaviour in VAO::reset().
Reviewer's GuideEnsures all OpenGL resource teardown happens while a valid context is current by wiring cleanup to QOpenGLContext::aboutToBeDestroyed, adding missing weather cleanup, hardening error checking and legacy GL resource reset paths, and avoiding GL-related work during MainWindow destruction. Sequence diagram for OpenGL context teardown and cleanupOpenGL flowsequenceDiagram
participant QOpenGLContext
participant MapCanvas
participant Batches
participant GLWeather
participant Textures
participant GLFont
participant OpenGL
QOpenGLContext->>MapCanvas: aboutToBeDestroyed
MapCanvas->>MapCanvas: cleanupOpenGL
MapCanvas->>Batches: resetExistingMeshesAndIgnorePendingRemesh
MapCanvas->>GLWeather: cleanup
MapCanvas->>Textures: destroyAll
MapCanvas->>GLFont: cleanup
MapCanvas->>OpenGL: cleanup
Class diagram for updated OpenGL cleanup and legacy resource managementclassDiagram
class MapCanvas {
+cleanupOpenGL() void
}
class GLWeather {
+GLWeather(OpenGL gl, GameObserver observer, FrameManager frameManager)
+~GLWeather() void
+cleanup() void
+updateFromGame() void
}
class Legacy_Functions {
+getUboManager() UboManager&
+checkError() void
}
class Legacy_VBO {
-m_weakFunctions
+reset() void
}
class Legacy_Program {
-m_weakFunctions
+reset() void
}
class UniqueMesh {
-m_mesh
+~UniqueMesh() void
+reset() void
+render(GLRenderState rs) void
+operator_bool() bool
}
MapCanvas o-- GLWeather : owns
MapCanvas o-- UniqueMesh : uses
Legacy_VBO --> Legacy_Functions : uses
Legacy_Program --> Legacy_Functions : uses
Legacy_Functions --> UboManager : returns
class UboManager {
+unregisterRebuildFunction(SharedVboEnum enumValue) void
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/opengl/legacy/VBO.cpp" line_range="30-33" />
<code_context>
}
- 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;
+ }
}
</code_context>
<issue_to_address>
**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:
```cpp
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.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+30
to
+33
| if (auto sharedFunctions = std::exchange(m_weakFunctions, {}).lock()) { | ||
| sharedFunctions->glDeleteBuffers(1, &vbo); | ||
| } else { | ||
| qCritical() << "Legacy::Functions is no longer valid, leaking VBO" << vbo; |
There was a problem hiding this comment.
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #510 +/- ##
==========================================
- Coverage 25.06% 25.05% -0.01%
==========================================
Files 510 510
Lines 42275 42286 +11
Branches 4574 4575 +1
==========================================
Hits 10596 10596
- Misses 31679 31690 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit aa07211 migrated MapCanvas from QOpenGLWidget to QOpenGLWindow. Under QOpenGLWidget, the GL context is still current during destruction, so calling cleanupOpenGL() from ~MapCanvas() was safe. Under QOpenGLWindow, the native context is destroyed before C++ destructors run, making any GL call from a destructor operate without a current context.
On NVIDIA drivers, glGetError() returns GL_INVALID_OPERATION indefinitely when called without a current context, causing the checkError() loop to spin forever. Two destructor call paths triggered this:
MainWindow::~MainWindow() called forceNewFile() -> slot_dataLoaded() -> forceUpdateMeshes(), which created and destroyed GL meshes after the context was gone.
GLWeather was not included in cleanupOpenGL(), so its particle meshes were destroyed in ~MapCanvas() after the context was gone.
Fix:
Summary by Sourcery
Ensure OpenGL resources are cleaned up safely when the context is destroyed and prevent error-checking loops during teardown.
Bug Fixes:
Enhancements: