Merge pull request #366 from djeada/copilot/fix-segfault-at-exit

Fix segfault at exit using smart pointers and explicit OpenGL cleanup

app/core/game_engine.cpp

@@ -108,8 +108,8 @@
 #include <utility>
 #include <vector>
 
-GameEngine::GameEngine()
-    : m_selectedUnitsModel(new SelectedUnitsModel(this, this)) {
+GameEngine::GameEngine(QObject *parent)
+    : QObject(parent), m_selectedUnitsModel(new SelectedUnitsModel(this, this)) {
 
   Game::Systems::NationRegistry::instance().initializeDefaults();
   Game::Systems::TroopCountRegistry::instance().initialize();
@@ -295,6 +295,51 @@ GameEngine::~GameEngine() {
   qInfo() << "AudioSystem shut down";
 }
 
+void GameEngine::cleanupOpenGLResources() {
+  qInfo() << "Cleaning up OpenGL resources...";
+  
+  // Check if we have a valid OpenGL context
+  // If not, skip OpenGL cleanup and just reset the pointers
+  // Qt will handle OpenGL resource cleanup when the context is destroyed
+  QOpenGLContext *context = QOpenGLContext::currentContext();
+  const bool hasValidContext = (context != nullptr);
+  
+  if (!hasValidContext) {
+    qInfo() << "No valid OpenGL context, skipping OpenGL cleanup";
+  }
+  
+  // Shutdown renderer and all OpenGL-dependent resources
+  // Only call shutdown if we have a valid context
+  if (m_renderer && hasValidContext) {
+    m_renderer->shutdown();
+    qInfo() << "Renderer shut down";
+  }
+  
+  // Clear render passes that reference renderer resources
+  m_passes.clear();
+  
+  // Reset all renderer-dependent unique_ptrs
+  // These will call destructors which may try to access OpenGL
+  // If no valid context, the destructors should be defensive
+  m_ground.reset();
+  m_terrain.reset();
+  m_biome.reset();
+  m_river.reset();
+  m_riverbank.reset();
+  m_bridge.reset();
+  m_fog.reset();
+  m_stone.reset();
+  m_plant.reset();
+  m_pine.reset();
+  m_firecamp.reset();
+  
+  // Reset renderer and resources
+  m_renderer.reset();
+  m_resources.reset();
+  
+  qInfo() << "OpenGL resources cleaned up";
+}
+
 void GameEngine::onMapClicked(qreal sx, qreal sy) {
   if (m_window == nullptr) {
     return;

app/core/game_engine.h

@@ -77,9 +77,11 @@ class QQuickWindow;
 class GameEngine : public QObject {
   Q_OBJECT
 public:
-  GameEngine();
+  explicit GameEngine(QObject *parent = nullptr);
   ~GameEngine() override;
 
+  void cleanupOpenGLResources();
+
   Q_PROPERTY(QObject *selectedUnitsModel READ selectedUnitsModel NOTIFY
                  selectedUnitsChanged)
   Q_PROPERTY(bool paused READ paused WRITE setPaused)

main.cpp

@@ -15,6 +15,7 @@
 #include <QTextStream>
 #include <QUrl>
 #include <cstdio>
+#include <memory>
 #include <qglobal.h>
 #include <qguiapplication.h>
 #include <qnamespace.h>
@@ -264,22 +265,28 @@ auto main(int argc, char *argv[]) -> int {
   QGuiApplication app(argc, argv);
   qInfo() << "QGuiApplication created successfully";
 
+  // Use unique_ptr with custom deleter for Qt objects
+  // This ensures proper cleanup order and prevents segfaults
+  std::unique_ptr<LanguageManager> language_manager;
+  std::unique_ptr<GameEngine> game_engine;
+  std::unique_ptr<QQmlApplicationEngine> engine;
+
   qInfo() << "Creating LanguageManager...";
-  auto *language_manager = new LanguageManager();
+  language_manager = std::make_unique<LanguageManager>(&app);
   qInfo() << "LanguageManager created";
 
   qInfo() << "Creating GameEngine...";
-  auto *game_engine = new GameEngine();
+  game_engine = std::make_unique<GameEngine>(&app);
   qInfo() << "GameEngine created";
 
   qInfo() << "Setting up QML engine...";
-  QQmlApplicationEngine engine;
+  engine = std::make_unique<QQmlApplicationEngine>();
   qInfo() << "Adding context properties...";
-  engine.rootContext()->setContextProperty("language_manager",
-                                           language_manager);
-  engine.rootContext()->setContextProperty("game", game_engine);
+  engine->rootContext()->setContextProperty("language_manager",
+                                           language_manager.get());
+  engine->rootContext()->setContextProperty("game", game_engine.get());
   qInfo() << "Adding import path...";
-  engine.addImportPath("qrc:/StandardOfIron/ui/qml");
+  engine->addImportPath("qrc:/StandardOfIron/ui/qml");
   qInfo() << "Registering QML types...";
   qmlRegisterType<GLView>("StandardOfIron", 1, 0, "GLView");
 
@@ -289,18 +296,18 @@ auto main(int argc, char *argv[]) -> int {
 
   qInfo() << "Loading Main.qml...";
   qInfo() << "Loading Main.qml...";
-  engine.load(QUrl(QStringLiteral("qrc:/StandardOfIron/ui/qml/Main.qml")));
+  engine->load(QUrl(QStringLiteral("qrc:/StandardOfIron/ui/qml/Main.qml")));
 
   qInfo() << "Checking if QML loaded...";
-  if (engine.rootObjects().isEmpty()) {
+  if (engine->rootObjects().isEmpty()) {
     qWarning() << "Failed to load QML file";
     return -1;
   }
   qInfo() << "QML loaded successfully, root objects count:"
-          << engine.rootObjects().size();
+          << engine->rootObjects().size();
 
   qInfo() << "Finding QQuickWindow...";
-  auto *root_obj = engine.rootObjects().first();
+  auto *root_obj = engine->rootObjects().first();
   auto *window = qobject_cast<QQuickWindow *>(root_obj);
   if (window == nullptr) {
     qInfo() << "Root object is not a window, searching children...";
@@ -361,6 +368,22 @@ auto main(int argc, char *argv[]) -> int {
 
   int const result = QGuiApplication::exec();
 
+  // Explicitly destroy in correct order to prevent segfault
+  qInfo() << "Shutting down...";
+  
+  // Destroy QML engine first (destroys OpenGL context)
+  engine.reset();
+  qInfo() << "QML engine destroyed";
+  
+  // Then destroy game engine
+  // OpenGL cleanup in destructors will be skipped if no valid context
+  game_engine.reset();
+  qInfo() << "GameEngine destroyed";
+  
+  // Finally destroy language manager
+  language_manager.reset();
+  qInfo() << "LanguageManager destroyed";
+
 #ifdef Q_OS_WIN
   // Check if we crashed during OpenGL initialization
   if (g_opengl_crashed) {

render/gl/backend.cpp

@@ -18,6 +18,7 @@
 #include "ground/stone_gpu.h"
 #include "mesh.h"
 #include "render_constants.h"
+#include <QOpenGLContext>
 #include "shader.h"
 #include "state_scopes.h"
 #include "texture.h"
@@ -46,12 +47,26 @@ const QVector3D k_grid_line_color(0.22F, 0.25F, 0.22F);
 Backend::Backend() = default;
 
 Backend::~Backend() {
-  m_cylinderPipeline.reset();
-  m_vegetationPipeline.reset();
-  m_terrainPipeline.reset();
-  m_characterPipeline.reset();
-  m_waterPipeline.reset();
-  m_effectsPipeline.reset();
+  // Manually destroy pipelines before base class destructor runs
+  // This prevents access to base class OpenGL functions after context is destroyed
+  if (QOpenGLContext::currentContext() == nullptr) {
+    // No valid context - manually release the unique_ptrs without calling their destructors
+    // This is safe because the OS will reclaim all OpenGL resources when process exits
+    (void)m_cylinderPipeline.release();
+    (void)m_vegetationPipeline.release();
+    (void)m_terrainPipeline.release();
+    (void)m_characterPipeline.release();
+    (void)m_waterPipeline.release();
+    (void)m_effectsPipeline.release();
+  } else {
+    // Valid context - normal cleanup
+    m_cylinderPipeline.reset();
+    m_vegetationPipeline.reset();
+    m_terrainPipeline.reset();
+    m_characterPipeline.reset();
+    m_waterPipeline.reset();
+    m_effectsPipeline.reset();
+  }
 }
 
 void Backend::initialize() {

render/gl/backend/cylinder_pipeline.cpp

@@ -5,6 +5,7 @@
 #include "../render_constants.h"
 #include "gl/shader_cache.h"
 #include <GL/gl.h>
+#include <QOpenGLContext>
 #include <algorithm>
 #include <cstddef>
 #include <qopenglext.h>
@@ -170,6 +171,20 @@ void CylinderPipeline::initializeCylinderPipeline() {
 }
 
 void CylinderPipeline::shutdownCylinderPipeline() {
+  // Check if we have a valid OpenGL context before cleanup
+  // If not, skip OpenGL calls - resources will be freed by Qt/OS
+  if (QOpenGLContext::currentContext() == nullptr) {
+    // No valid context, just reset state without OpenGL calls
+    m_cylinderVao = 0;
+    m_cylinderVertexBuffer = 0;
+    m_cylinderIndexBuffer = 0;
+    m_cylinderInstanceBuffer = 0;
+    m_cylinderIndexCount = 0;
+    m_cylinderInstanceCapacity = 0;
+    m_cylinderScratch.clear();
+    return;
+  }
+
   initializeOpenGLFunctions();
 
   m_cylinderPersistentBuffer.destroy();
@@ -323,6 +338,20 @@ void CylinderPipeline::initializeFogPipeline() {
 }
 
 void CylinderPipeline::shutdownFogPipeline() {
+  // Check if we have a valid OpenGL context before cleanup
+  // If not, skip OpenGL calls - resources will be freed by Qt/OS
+  if (QOpenGLContext::currentContext() == nullptr) {
+    // No valid context, just reset state without OpenGL calls
+    m_fogVao = 0;
+    m_fogVertexBuffer = 0;
+    m_fogIndexBuffer = 0;
+    m_fogInstanceBuffer = 0;
+    m_fogIndexCount = 0;
+    m_fogInstanceCapacity = 0;
+    m_fogScratch.clear();
+    return;
+  }
+
   initializeOpenGLFunctions();
 
   if (m_fogInstanceBuffer != 0U) {

render/gl/backend/vegetation_pipeline.cpp

@@ -3,6 +3,7 @@
 #include "gl/shader_cache.h"
 #include <GL/gl.h>
 #include <QDebug>
+#include <QOpenGLContext>
 #include <cmath>
 #include <cstddef>
 #include <cstdint>
@@ -187,6 +188,16 @@ void VegetationPipeline::initializeStonePipeline() {
 }
 
 void VegetationPipeline::shutdownStonePipeline() {
+  // Check if we have a valid OpenGL context before cleanup
+  if (QOpenGLContext::currentContext() == nullptr) {
+    m_stoneVao = 0;
+    m_stoneVertexBuffer = 0;
+    m_stoneIndexBuffer = 0;
+    m_stoneVertexCount = 0;
+    m_stoneIndexCount = 0;
+    return;
+  }
+
   initializeOpenGLFunctions();
   if (m_stoneIndexBuffer != 0U) {
     glDeleteBuffers(1, &m_stoneIndexBuffer);
@@ -281,6 +292,16 @@ void VegetationPipeline::initializePlantPipeline() {
 }
 
 void VegetationPipeline::shutdownPlantPipeline() {
+  // Check if we have a valid OpenGL context before cleanup
+  if (QOpenGLContext::currentContext() == nullptr) {
+    m_plantVao = 0;
+    m_plantVertexBuffer = 0;
+    m_plantIndexBuffer = 0;
+    m_plantVertexCount = 0;
+    m_plantIndexCount = 0;
+    return;
+  }
+
   initializeOpenGLFunctions();
   if (m_plantIndexBuffer != 0U) {
     glDeleteBuffers(1, &m_plantIndexBuffer);
@@ -430,6 +451,16 @@ void VegetationPipeline::initializePinePipeline() {
 }
 
 void VegetationPipeline::shutdownPinePipeline() {
+  // Check if we have a valid OpenGL context before cleanup
+  if (QOpenGLContext::currentContext() == nullptr) {
+    m_pineVao = 0;
+    m_pineVertexBuffer = 0;
+    m_pineIndexBuffer = 0;
+    m_pineVertexCount = 0;
+    m_pineIndexCount = 0;
+    return;
+  }
+
   initializeOpenGLFunctions();
   if (m_pineIndexBuffer != 0U) {
     glDeleteBuffers(1, &m_pineIndexBuffer);
@@ -525,6 +556,16 @@ void VegetationPipeline::initializeFireCampPipeline() {
 }
 
 void VegetationPipeline::shutdownFireCampPipeline() {
+  // Check if we have a valid OpenGL context before cleanup
+  if (QOpenGLContext::currentContext() == nullptr) {
+    m_firecampVao = 0;
+    m_firecampVertexBuffer = 0;
+    m_firecampIndexBuffer = 0;
+    m_firecampVertexCount = 0;
+    m_firecampIndexCount = 0;
+    return;
+  }
+
   initializeOpenGLFunctions();
   if (m_firecampIndexBuffer != 0U) {
     glDeleteBuffers(1, &m_firecampIndexBuffer);

render/gl/persistent_buffer.h

@@ -88,6 +88,16 @@ public:
       return;
     }
 
+    // Check if we have a valid OpenGL context before cleanup
+    if (QOpenGLContext::currentContext() == nullptr) {
+      // No valid context, just reset state without OpenGL calls
+      m_buffer = 0;
+      m_mappedPtr = nullptr;
+      m_capacity = 0;
+      m_totalSize = 0;
+      return;
+    }
+
     initializeOpenGLFunctions();
 
     if (m_mappedPtr != nullptr) {