Visualizer: remove glfwTerminate() call from destructor (fix #4960) …

…(See PR #4963) [Author: @bchretien] (#6559) * examples: fix headless_rendering.py * ViewControl.cpp: relax floating-point comparison in ConvertFromPinholeCameraParameters() * Visualizer: improve GLTF context handling (fix #4960) --------- Authored-by: Benjamin Chrétien <[email protected]>
isl-org · Dec 29, 2023 · c948c45 · c948c45
1 parent 7931263
commit c948c45
Show file tree

Hide file tree

Showing 3 changed files with 69 additions and 35 deletions.
diff --git a/cpp/open3d/visualization/visualizer/ViewControl.cpp b/cpp/open3d/visualization/visualizer/ViewControl.cpp
@@ -174,13 +174,16 @@ bool ViewControl::ConvertFromPinholeCameraParameters(
         bool allow_arbitrary) {
     auto intrinsic = parameters.intrinsic_;
     auto extrinsic = parameters.extrinsic_;
-    if (!allow_arbitrary && (window_height_ <= 0 || window_width_ <= 0 ||
-                             window_height_ != intrinsic.height_ ||
-                             window_width_ != intrinsic.width_ ||
-                             intrinsic.intrinsic_matrix_(0, 2) !=
-                                     (double)window_width_ / 2.0 - 0.5 ||
-                             intrinsic.intrinsic_matrix_(1, 2) !=
-                                     (double)window_height_ / 2.0 - 0.5)) {
+
+    constexpr double threshold = 1.e-6;
+    if (!allow_arbitrary &&
+        (window_height_ <= 0 || window_width_ <= 0 ||
+         window_height_ != intrinsic.height_ ||
+         window_width_ != intrinsic.width_ ||
+         std::abs(intrinsic.intrinsic_matrix_(0, 2) -
+                  ((double)window_width_ / 2.0 - 0.5)) > threshold ||
+         std::abs(intrinsic.intrinsic_matrix_(1, 2) =
+                          ((double)window_height_ / 2.0 - 0.5)) > threshold)) {
         utility::LogWarning(
                 "[ViewControl] ConvertFromPinholeCameraParameters() failed "
                 "because window height and width do not match.");

diff --git a/cpp/open3d/visualization/visualizer/Visualizer.cpp b/cpp/open3d/visualization/visualizer/Visualizer.cpp
@@ -7,6 +7,8 @@
 
 #include "open3d/visualization/visualizer/Visualizer.h"
 
+#include <memory>
+
 #include "open3d/geometry/TriangleMesh.h"
 
 #if defined(__APPLE__) && defined(BUILD_GUI)
@@ -18,51 +20,63 @@ void unbind();
 
 namespace open3d {
 
-namespace {
+namespace visualization {
 
-class GLFWEnvironmentSingleton {
+/// \brief GLFW context, handled as a singleton.
+class GLFWContext {
 private:
-    GLFWEnvironmentSingleton() { utility::LogDebug("GLFW init."); }
-    GLFWEnvironmentSingleton(const GLFWEnvironmentSingleton &) = delete;
-    GLFWEnvironmentSingleton &operator=(const GLFWEnvironmentSingleton &) =
-            delete;
-
-public:
-    ~GLFWEnvironmentSingleton() {
-        glfwTerminate();
-        utility::LogDebug("GLFW destruct.");
-    }
-
-public:
-    static GLFWEnvironmentSingleton &GetInstance() {
-        static GLFWEnvironmentSingleton singleton;
-        return singleton;
-    }
+    GLFWContext() {
+        utility::LogDebug("GLFW init.");
 
-    static int InitGLFW() {
-        GLFWEnvironmentSingleton::GetInstance();
 #if defined(__APPLE__)
         // On macOS, GLFW changes the directory to the resource directory,
         // which will cause an unexpected change of directory if using a
         // framework build version of Python.
         glfwInitHint(GLFW_COCOA_CHDIR_RESOURCES, GLFW_FALSE);
 #endif
-        return glfwInit();
+        init_status_ = glfwInit();
+    }
+
+    GLFWContext(const GLFWContext &) = delete;
+    GLFWContext &operator=(const GLFWContext &) = delete;
+
+public:
+    ~GLFWContext() {
+        if (init_status_ == GLFW_TRUE) {
+            glfwTerminate();
+            utility::LogDebug("GLFW destruct.");
+        }
+    }
+
+    /// \brief Get the glfwInit status.
+    inline int InitStatus() const { return init_status_; }
+
+    /// \brief Get a shared instance of the GLFW context.
+    static std::shared_ptr<GLFWContext> GetInstance() {
+        static std::weak_ptr<GLFWContext> singleton;
+
+        auto res = singleton.lock();
+        if (res == nullptr) {
+            res = std::shared_ptr<GLFWContext>(new GLFWContext());
+            singleton = res;
+        }
+        return res;
     }
 
     static void GLFWErrorCallback(int error, const char *description) {
         utility::LogWarning("GLFW Error: {}", description);
     }
-};
-
-}  // unnamed namespace
 
-namespace visualization {
+private:
+    /// \brief Status of the glfwInit call.
+    int init_status_ = GLFW_FALSE;
+};
 
 Visualizer::Visualizer() {}
 
 Visualizer::~Visualizer() {
-    glfwTerminate();  // to be safe
+    DestroyVisualizerWindow();
+
 #if defined(__APPLE__) && defined(BUILD_GUI)
     bluegl::unbind();
 #endif
@@ -77,6 +91,7 @@ bool Visualizer::CreateVisualizerWindow(
         const bool visible /* = true*/) {
     window_name_ = window_name;
     if (window_) {  // window already created
+        utility::LogDebug("[Visualizer] Reusing window.");
         UpdateWindowTitle();
         glfwSetWindowPos(window_, left, top);
         glfwSetWindowSize(window_, width, height);
@@ -91,8 +106,10 @@ bool Visualizer::CreateVisualizerWindow(
         return true;
     }
 
-    glfwSetErrorCallback(GLFWEnvironmentSingleton::GLFWErrorCallback);
-    if (!GLFWEnvironmentSingleton::InitGLFW()) {
+    utility::LogDebug("[Visualizer] Creating window.");
+    glfwSetErrorCallback(GLFWContext::GLFWErrorCallback);
+    glfw_context_ = GLFWContext::GetInstance();
+    if (glfw_context_->InitStatus() != GLFW_TRUE) {
         utility::LogWarning("Failed to initialize GLFW");
         return false;
     }
@@ -201,9 +218,17 @@ bool Visualizer::CreateVisualizerWindow(
 }
 
 void Visualizer::DestroyVisualizerWindow() {
+    if (!is_initialized_) {
+        return;
+    }
+
+    utility::LogDebug("[Visualizer] Destroying window.");
     is_initialized_ = false;
     glDeleteVertexArrays(1, &vao_id_);
+    vao_id_ = 0;
     glfwDestroyWindow(window_);
+    window_ = nullptr;
+    glfw_context_.reset();
 }
 
 void Visualizer::RegisterAnimationCallback(

diff --git a/cpp/open3d/visualization/visualizer/Visualizer.h b/cpp/open3d/visualization/visualizer/Visualizer.h
@@ -37,6 +37,8 @@ class Image;
 
 namespace visualization {
 
+class GLFWContext;
+
 /// \class Visualizer
 ///
 /// \brief The main Visualizer class.
@@ -262,6 +264,10 @@ class Visualizer {
     // window
     GLFWwindow *window_ = NULL;
     std::string window_name_ = "Open3D";
+
+    /// \brief Shared GLFW context.
+    std::shared_ptr<GLFWContext> glfw_context_ = nullptr;
+
     Eigen::Vector2i saved_window_size_ = Eigen::Vector2i::Zero();
     Eigen::Vector2i saved_window_pos_ = Eigen::Vector2i::Zero();
     std::function<bool(Visualizer *)> animation_callback_func_ = nullptr;