From 92cc9eef1e90c41c3850781066143f7e48178dcd Mon Sep 17 00:00:00 2001
From: Giorge Koulin <g.koulin@live.com>
Date: Wed, 2 Oct 2024 09:21:32 +0100
Subject: [PATCH 1/2] tidy up on shutdown to allow multiple `init`/`shutdown`
 cycles per process

- use `std::uique_ptr<Engine>` to manage global engine lifetime. This means that we no longer need `bool state::initialized`, we can just use the engine ptr to infer initialized state.
- Hide window.
- Clear `contextStack`.
- Reset cached `lazy` properties. Had to convert from namespace to struct to allow default initialisation reset.
---
 include/polyscope/context.h               |  1 -
 include/polyscope/polyscope.h             |  3 --
 include/polyscope/render/engine.h         |  2 +-
 src/polyscope.cpp                         | 57 ++++++++++++++---------
 src/render/initialize_backend.cpp         |  2 +-
 src/render/managed_buffer.cpp             |  6 +--
 src/render/mock_opengl/mock_gl_engine.cpp |  2 +-
 src/render/opengl/gl_engine_egl.cpp       |  3 +-
 src/render/opengl/gl_engine_glfw.cpp      |  6 ++-
 src/state.cpp                             |  1 -
 10 files changed, 47 insertions(+), 36 deletions(-)

diff --git a/include/polyscope/context.h b/include/polyscope/context.h
index 051cbf0a..607fd280 100644
--- a/include/polyscope/context.h
+++ b/include/polyscope/context.h
@@ -51,7 +51,6 @@ struct Context {
   // === General globals from polyscope.h
   // ======================================================
 
-  bool initialized = false;
   std::string backend = "";
   std::map<std::string, std::map<std::string, std::unique_ptr<Structure>>> structures;
   std::map<std::string, std::unique_ptr<Group>> groups;
diff --git a/include/polyscope/polyscope.h b/include/polyscope/polyscope.h
index 633db17a..0086fe3f 100644
--- a/include/polyscope/polyscope.h
+++ b/include/polyscope/polyscope.h
@@ -69,9 +69,6 @@ bool windowRequestsClose();
 // === Global variables ===
 namespace state {
 
-// has polyscope::init() been called?
-extern bool& initialized;
-
 // what backend was set on initialization
 extern std::string& backend;
 
diff --git a/include/polyscope/render/engine.h b/include/polyscope/render/engine.h
index 8c66931a..8a5960ef 100644
--- a/include/polyscope/render/engine.h
+++ b/include/polyscope/render/engine.h
@@ -670,7 +670,7 @@ class Engine {
 
 // The global render engine
 // Gets initialized by initializeRenderEngine() in polyscope::init();
-extern Engine* engine;
+extern std::unique_ptr<Engine> engine;
 
 // The backend type of the engine, as initialized above
 extern std::string engineBackendName;
diff --git a/src/polyscope.cpp b/src/polyscope.cpp
index b26dec14..b7862bca 100644
--- a/src/polyscope.cpp
+++ b/src/polyscope.cpp
@@ -162,17 +162,16 @@ void init(std::string backend) {
 
   view::invalidateView();
 
-  state::initialized = true;
   state::doDefaultMouseInteraction = true;
 }
 
 void checkInitialized() {
-  if (!state::initialized) {
+  if (!isInitialized()) {
     exception("Polyscope has not been initialized");
   }
 }
 
-bool isInitialized() { return state::initialized; }
+bool isInitialized() { return render::engine != nullptr; }
 
 void pushContext(std::function<void()> callbackFunction, bool drawDefaultUI) {
 
@@ -890,7 +889,7 @@ void mainLoopIteration() {
 
 void show(size_t forFrames) {
 
-  if (!state::initialized) {
+  if (!isInitialized()) {
     exception("must initialize Polyscope with polyscope::init() before calling polyscope::show().");
   }
   unshowRequested = false;
@@ -934,6 +933,8 @@ bool windowRequestsClose() {
   return false;
 }
 
+void resetLazy();
+
 void shutdown() {
 
   // TODO should we make an effort to destruct everything here?
@@ -941,7 +942,11 @@ void shutdown() {
     writePrefsFile();
   }
 
+  render::engine->hideWindow();
   render::engine->shutdownImGui();
+  render::engine.reset();
+  contextStack.clear();
+  resetLazy();
 }
 
 bool registerStructure(Structure* s, bool replaceIfPresent) {
@@ -1179,7 +1184,7 @@ void refresh() {
 }
 
 // Cached versions of lazy properties used for updates
-namespace lazy {
+struct Lazy {
 TransparencyMode transparencyMode = TransparencyMode::None;
 int transparencyRenderPasses = 8;
 int ssaaFactor = 1;
@@ -1187,8 +1192,10 @@ bool groundPlaneEnabled = true;
 GroundPlaneMode groundPlaneMode = GroundPlaneMode::TileReflection;
 ScaledValue<float> groundPlaneHeightFactor = 0;
 int shadowBlurIters = 2;
-float shadowDarkness = .4;
-} // namespace lazy
+float shadowDarkness = .4f;
+};
+static Lazy lazy{};
+
 
 void processLazyProperties() {
 
@@ -1203,49 +1210,53 @@ void processLazyProperties() {
 
 
   // transparency mode
-  if (lazy::transparencyMode != options::transparencyMode) {
-    lazy::transparencyMode = options::transparencyMode;
+  if (lazy.transparencyMode != options::transparencyMode) {
+    lazy.transparencyMode = options::transparencyMode;
     render::engine->setTransparencyMode(options::transparencyMode);
   }
 
   // transparency render passes
-  if (lazy::transparencyRenderPasses != options::transparencyRenderPasses) {
-    lazy::transparencyRenderPasses = options::transparencyRenderPasses;
+  if (lazy.transparencyRenderPasses != options::transparencyRenderPasses) {
+    lazy.transparencyRenderPasses = options::transparencyRenderPasses;
     requestRedraw();
   }
 
   // ssaa
-  if (lazy::ssaaFactor != options::ssaaFactor) {
-    lazy::ssaaFactor = options::ssaaFactor;
+  if (lazy.ssaaFactor != options::ssaaFactor) {
+    lazy.ssaaFactor = options::ssaaFactor;
     render::engine->setSSAAFactor(options::ssaaFactor);
   }
 
   // ground plane
-  if (lazy::groundPlaneEnabled != options::groundPlaneEnabled || lazy::groundPlaneMode != options::groundPlaneMode) {
-    lazy::groundPlaneEnabled = options::groundPlaneEnabled;
+  if (lazy.groundPlaneEnabled != options::groundPlaneEnabled || lazy.groundPlaneMode != options::groundPlaneMode) {
+    lazy.groundPlaneEnabled = options::groundPlaneEnabled;
     if (!options::groundPlaneEnabled) {
       // if the (depecated) groundPlaneEnabled = false, set mode to None, so we only have one variable to check
       options::groundPlaneMode = GroundPlaneMode::None;
     }
-    lazy::groundPlaneMode = options::groundPlaneMode;
+    lazy.groundPlaneMode = options::groundPlaneMode;
     render::engine->groundPlane.prepare();
     requestRedraw();
   }
-  if (lazy::groundPlaneHeightFactor.asAbsolute() != options::groundPlaneHeightFactor.asAbsolute() ||
-      lazy::groundPlaneHeightFactor.isRelative() != options::groundPlaneHeightFactor.isRelative()) {
-    lazy::groundPlaneHeightFactor = options::groundPlaneHeightFactor;
+  if (lazy.groundPlaneHeightFactor.asAbsolute() != options::groundPlaneHeightFactor.asAbsolute() ||
+      lazy.groundPlaneHeightFactor.isRelative() != options::groundPlaneHeightFactor.isRelative()) {
+    lazy.groundPlaneHeightFactor = options::groundPlaneHeightFactor;
     requestRedraw();
   }
-  if (lazy::shadowBlurIters != options::shadowBlurIters) {
-    lazy::shadowBlurIters = options::shadowBlurIters;
+  if (lazy.shadowBlurIters != options::shadowBlurIters) {
+    lazy.shadowBlurIters = options::shadowBlurIters;
     requestRedraw();
   }
-  if (lazy::shadowDarkness != options::shadowDarkness) {
-    lazy::shadowDarkness = options::shadowDarkness;
+  if (lazy.shadowDarkness != options::shadowDarkness) {
+    lazy.shadowDarkness = options::shadowDarkness;
     requestRedraw();
   }
 };
 
+void resetLazy() {
+  lazy = {};
+}
+
 void updateStructureExtents() {
 
   if (!options::automaticallyComputeSceneExtents) {
diff --git a/src/render/initialize_backend.cpp b/src/render/initialize_backend.cpp
index 95e20772..07537dd4 100644
--- a/src/render/initialize_backend.cpp
+++ b/src/render/initialize_backend.cpp
@@ -10,7 +10,7 @@ namespace polyscope {
 namespace render {
 
 // Storage for the global engine pointer
-Engine* engine = nullptr;
+std::unique_ptr<Engine> engine;
 
 // Backend we initialized with; written once below
 std::string engineBackendName = "";
diff --git a/src/render/managed_buffer.cpp b/src/render/managed_buffer.cpp
index ecba28e6..ef4ea74b 100644
--- a/src/render/managed_buffer.cpp
+++ b/src/render/managed_buffer.cpp
@@ -299,7 +299,7 @@ std::shared_ptr<render::AttributeBuffer> ManagedBuffer<T>::getRenderAttributeBuf
 
   if (!renderAttributeBuffer) {
     ensureHostBufferPopulated(); // warning: the order of these matters because of how hostBufferPopulated works
-    renderAttributeBuffer = generateAttributeBuffer<T>(render::engine);
+    renderAttributeBuffer = generateAttributeBuffer<T>(render::engine.get());
     renderAttributeBuffer->setData(data);
   }
   return renderAttributeBuffer;
@@ -312,7 +312,7 @@ std::shared_ptr<render::TextureBuffer> ManagedBuffer<T>::getRenderTextureBuffer(
   if (!renderTextureBuffer) {
     ensureHostBufferPopulated(); // warning: the order of these matters because of how hostBufferPopulated works
 
-    renderTextureBuffer = generateTextureBuffer<T>(deviceBufferType, render::engine);
+    renderTextureBuffer = generateTextureBuffer<T>(deviceBufferType, render::engine.get());
 
     // templatize this?
     switch (deviceBufferType) {
@@ -377,7 +377,7 @@ ManagedBuffer<T>::getIndexedRenderAttributeBuffer(ManagedBuffer<uint32_t>& indic
 
   // We don't have it. Create a new one and return that.
   ensureHostBufferPopulated();
-  std::shared_ptr<render::AttributeBuffer> newBuffer = generateAttributeBuffer<T>(render::engine);
+  std::shared_ptr<render::AttributeBuffer> newBuffer = generateAttributeBuffer<T>(render::engine.get());
   indices.ensureHostBufferPopulated();
   std::vector<T> expandData = gather(data, indices.data);
   newBuffer->setData(expandData); // initially populate
diff --git a/src/render/mock_opengl/mock_gl_engine.cpp b/src/render/mock_opengl/mock_gl_engine.cpp
index 6b33aef2..b728dcf6 100644
--- a/src/render/mock_opengl/mock_gl_engine.cpp
+++ b/src/render/mock_opengl/mock_gl_engine.cpp
@@ -38,7 +38,7 @@ MockGLEngine* glEngine = nullptr; // alias for engine pointer
 
 void initializeRenderEngine() {
   glEngine = new MockGLEngine();
-  engine = glEngine;
+  engine.reset(glEngine);
   glEngine->initialize();
   engine->allocateGlobalBuffersAndPrograms();
 }
diff --git a/src/render/opengl/gl_engine_egl.cpp b/src/render/opengl/gl_engine_egl.cpp
index 76a957a0..19c91429 100644
--- a/src/render/opengl/gl_engine_egl.cpp
+++ b/src/render/opengl/gl_engine_egl.cpp
@@ -118,7 +118,8 @@ void initializeRenderEngine_egl() {
 
   glEngineEGL = new GLEngineEGL(); // create the new global engine object
 
-  engine = glEngineEGL; // we keep a few copies of this pointer with various types
+  // we keep a few copies of this pointer with various types
+  engine.reset(glEngineEGL); // owning
   glEngine = glEngineEGL;
 
   // initialize
diff --git a/src/render/opengl/gl_engine_glfw.cpp b/src/render/opengl/gl_engine_glfw.cpp
index 4eede7aa..26bdb6bb 100644
--- a/src/render/opengl/gl_engine_glfw.cpp
+++ b/src/render/opengl/gl_engine_glfw.cpp
@@ -22,9 +22,13 @@ extern GLEngine* glEngine;            // defined in gl_engine.h
 
 void initializeRenderEngine_glfw() {
 
+  if (engine) {
+    exception("ERROR: engine is already initialised");
+  }
   glEngineGLFW = new GLEngineGLFW(); // create the new global engine object
 
-  engine = glEngineGLFW; // we keep a few copies of this pointer with various types
+  // we keep a few copies of this pointer with various types
+  engine.reset(glEngineGLFW); // owning
   glEngine = glEngineGLFW;
 
   // initialize
diff --git a/src/state.cpp b/src/state.cpp
index d57c5ffa..692aacb4 100644
--- a/src/state.cpp
+++ b/src/state.cpp
@@ -10,7 +10,6 @@ namespace state {
 Context globalContext;
 
 // Map all of the named global variables as references to the context struct
-bool& initialized = globalContext.initialized;
 std::string& backend = globalContext.backend;
 std::map<std::string, std::map<std::string, std::unique_ptr<Structure>>>& structures = globalContext.structures;
 std::map<std::string, std::unique_ptr<Group>>& groups = globalContext.groups;

From fc74f12f55336d94e086af783e985ec87bc80c5c Mon Sep 17 00:00:00 2001
From: Giorge Koulin <g.koulin@live.com>
Date: Wed, 2 Oct 2024 09:21:32 +0100
Subject: [PATCH 2/2] test multiple `init`/`shutdown` cycles

In `PolyscopeTest` fixture, initialise polyscope at the start of each test and shutdown at the end. This will test multiple shutdown cycles during the lifetime of the fixture.
---
 test/include/polyscope_test.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/test/include/polyscope_test.h b/test/include/polyscope_test.h
index c5b4a6e9..380a8891 100644
--- a/test/include/polyscope_test.h
+++ b/test/include/polyscope_test.h
@@ -32,7 +32,6 @@ class PolyscopeTest : public ::testing::Test {
     polyscope::options::enableRenderErrorChecks = true;
     polyscope::options::errorsThrowExceptions = true;
     polyscope::options::hideWindowAfterShow = false;
-    polyscope::init(testBackend);
   }
 
   // Per-test-suite tear-down.
@@ -46,10 +45,14 @@ class PolyscopeTest : public ::testing::Test {
   */
 
   // You can define per-test set-up logic as usual.
-  // virtual void SetUp() { ... }
+  void SetUp() override {
+    polyscope::init(testBackend);
+  }
 
   // You can define per-test tear-down logic as usual.
-  // virtual void TearDown() { ... }
+  void TearDown() override {
+    polyscope::shutdown();
+  }
 
   // Some expensive resource shared by all tests.
   // static T* shared_resource_;