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_;