build: macOS (arm64) support, CI lane, and PCL loader validation

.github/workflows/default.yml

@@ -76,6 +76,43 @@ jobs:
         name: test-results-${{ matrix.os }}
         path: ${{runner.workspace}}/build/test_results.xml
 
+  # macOS coverage: AppleClang + libc++ on arm64. The GL usage is already
+  # portable (a 3.2 core + forward-compatible context on Apple, #version 330
+  # shaders, under the 4.1 cap macOS enforces); this lane keeps it that way.
+  # The GUI itself is not exercised -- the samples need a display.
+  #
+  # This lane is also what first exercised the loader against PCL 1.15, whose
+  # readers accept a malformed file and return an empty cloud rather than an
+  # error; the loader now validates the header instead of only the return code.
+  macos:
+    name: macOS (arm64)
+    runs-on: macos-14
+    steps:
+    - uses: actions/checkout@v4
+    - name: Checkout submodules
+      run: git submodule update --init --recursive
+    - name: Install Dependencies
+      # eigen@3 and opencv@4 are keg-only: Homebrew defaults to Eigen 5 and
+      # OpenCV 5, and this project targets the 3.x/4.x APIs.
+      run: brew install glfw cairo fontconfig glm opencv@4 pcl eigen@3 abseil re2 ncurses
+    - name: Configure CMake
+      shell: bash
+      run: >
+        cmake -S . -B build -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DBUILD_TESTING=ON
+        -DCMAKE_PREFIX_PATH="$(brew --prefix eigen@3);$(brew --prefix opencv@4)"
+    - name: Build
+      shell: bash
+      run: cmake --build build -j"$(sysctl -n hw.ncpu)" --config $BUILD_TYPE
+    - name: Test
+      working-directory: build
+      shell: bash
+      # Benchmarks are excluded: they open a real GL window, and the hosted
+      # macOS runner is headless. Unlike Linux, GLFW initialises fine there
+      # (Cocoa is always present) and only fails later at window creation
+      # ("NSGL: Failed to find a suitable pixel format"), so it aborts rather
+      # than degrading. The correctness suites skip GUI tests on their own.
+      run: ctest -C $BUILD_TYPE --output-on-failure -LE benchmark
+
   minimal-build:
     runs-on: ${{ matrix.os }}
     strategy:

CMakeLists.txt

@@ -120,6 +120,55 @@ else ()
   message(STATUS "quickviz test will not be built")
 endif ()
 
+## Test discovery mode
+# gtest_discover_tests defaults to running each test binary at BUILD time to
+# enumerate its cases, with a 5 s timeout. These binaries link GL, cairo and
+# (optionally) OpenCV/PCL, and where those ship as many small shared libraries
+# the dynamic loader alone can exceed that -- discovery then times out and
+# CMake DELETES the executable, which surfaces later as a confusing
+# <target>_NOT_BUILT failure. Enumerating at test time instead removes the
+# build-time dependency on process startup entirely.
+# (CMake >= 3.22; older versions ignore this and keep the previous behaviour.)
+set(CMAKE_GTEST_DISCOVER_TESTS_DISCOVERY_MODE PRE_TEST)
+
+## glm (header-only)
+# Previously found implicitly: Ubuntu's libglm-dev installs into /usr/include,
+# which is on the default search path. Other prefixes (Homebrew, vcpkg, a local
+# install) are not, so locate it properly. Kept at directory scope because that
+# is how it was already consumed -- no target export signatures change.
+find_package(glm QUIET)
+if (glm_FOUND)
+  get_target_property(QUICKVIZ_GLM_INCLUDE_DIRS glm::glm INTERFACE_INCLUDE_DIRECTORIES)
+  if (QUICKVIZ_GLM_INCLUDE_DIRS)
+    include_directories(SYSTEM ${QUICKVIZ_GLM_INCLUDE_DIRS})
+  endif ()
+  message(STATUS "quickviz: glm found (${glm_VERSION})")
+else ()
+  message(STATUS "quickviz: glm not found via find_package -- relying on the "
+                 "default include path")
+endif ()
+# The renderables use GTX extensions (vector_angle, quaternion, transform,
+# norm, component_wise, rotate_vector). glm >= 1.0 hard-errors on these unless
+# the caller opts in.
+add_compile_definitions(GLM_ENABLE_EXPERIMENTAL)
+
+## Vendored googletest must win the include search
+# OpenCV and PCL export their whole dependency prefix (Homebrew's
+# /opt/homebrew/include, say). If a different googletest is installed there,
+# its headers are found ahead of the vendored copy while the vendored
+# libgtest.a is still what links -- an ABI mismatch that appears only as
+# undefined testing::internal symbols. Prepending keeps the vendored headers
+# in front of anything a dependency drags in.
+#
+# Stated here rather than after add_subdirectory(third_party) because that
+# comes last, and a directory-scope include only reaches targets defined
+# after it -- so guard on the path rather than on the gtest target.
+if (BUILD_TESTING AND EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/third_party/googletest/googletest/include")
+  include_directories(BEFORE SYSTEM
+      ${CMAKE_CURRENT_SOURCE_DIR}/third_party/googletest/googletest/include
+      ${CMAKE_CURRENT_SOURCE_DIR}/third_party/googletest/googlemock/include)
+endif ()
+
 ## Add source directory
 add_subdirectory(src)
 add_subdirectory(sample)

src/core/test/unit_test/test_thread_safe_queue.cpp

@@ -101,8 +101,14 @@ TEST_F(ThreadSafeQueueTest, PopTimeout) {
   auto duration = std::chrono::steady_clock::now() - start;
 
   EXPECT_FALSE(result.has_value());
+  // The lower bound is the contract: PopFor must wait at least its timeout.
   EXPECT_GE(duration, std::chrono::milliseconds(45)); // Allow some variance
-  EXPECT_LE(duration, std::chrono::milliseconds(100));
+  // The upper bound only guards against waiting forever. It deliberately has
+  // a wide margin: how promptly a blocked thread is rescheduled is up to the
+  // OS, and a loaded CI runner can overshoot a 50 ms wait severalfold (this
+  // fired at 143 ms against the old 100 ms bound), which says nothing about
+  // the queue.
+  EXPECT_LE(duration, std::chrono::seconds(2));
 }
 
 TEST_F(ThreadSafeQueueTest, MoveConstructor) {

src/pcl_bridge/src/pcl_loader.cpp

@@ -27,6 +27,32 @@
 namespace quickviz {
 namespace pcl_bridge {
 
+namespace {
+
+// PCL's readers do not reliably signal a malformed file through their return
+// code. A file with no recognisable header parses into an empty cloud and
+// still returns success (PCL 1.15 logs "number of points is zero", defaults
+// HEIGHT, reports that fields x/y/z were not matched, and returns 0), so
+// checking only for -1 lets arbitrary non-point-cloud data through as an
+// empty cloud.
+//
+// The header is the reliable signal: every PCD/PLY that can be read into a
+// PCL point type declares x, y and z, and that stays true for a legitimately
+// empty cloud (POINTS 0 with a full FIELDS line), which must keep loading.
+// Their absence means the file is not a point cloud.
+void RequirePointCloudHeader(const PointCloudFields& fields,
+                             const std::string& format,
+                             const std::string& filename) {
+  if (!fields.HasXYZ()) {
+    throw CorruptedFileException(
+        format + " header declares no x/y/z fields, so the file is not a "
+                 "point cloud: " + filename);
+  }
+}
+
+}  // namespace
+
+
 // PointCloudMetadata implementation
 std::string PointCloudMetadata::GetRecommendedPCLType() const {
   if (fields.HasRGBAColor()) {
@@ -68,6 +94,7 @@ PointCloudMetadata PointCloudLoader::LoadPCD(const std::string& filename,
   }
 
   PointCloudFields fields = DetectPCDFields(filename);
+  RequirePointCloudHeader(fields, "PCD", filename);
 
   if (progress_callback) {
     progress_callback(0.2f, "Loading point cloud data...");
@@ -84,6 +111,7 @@ PointCloudMetadata PointCloudLoader::LoadPLY(const std::string& filename,
   }
 
   PointCloudFields fields = DetectPLYFields(filename);
+  RequirePointCloudHeader(fields, "PLY", filename);
 
   if (progress_callback) {
     progress_callback(0.2f, "Loading point cloud data...");
@@ -236,11 +264,12 @@ PointCloudLoader::LoadPCDInternal(const std::string& filename, ProgressCallback
     throw CorruptedFileException("Failed to load PCD file: " + filename);
   }
 
+  PointCloudFields fields = DetectPCDFields(filename);
+  RequirePointCloudHeader(fields, "PCD", filename);
+
   if (progress_callback) {
     progress_callback(0.7f, "Calculating metadata...");
   }
-
-  PointCloudFields fields = DetectPCDFields(filename);
   PointCloudMetadata metadata = CalculateMetadata(filename, "PCD", *cloud, fields);
 
   return {cloud, metadata};
@@ -255,11 +284,12 @@ PointCloudLoader::LoadPLYInternal(const std::string& filename, ProgressCallback
     throw CorruptedFileException("Failed to load PLY file: " + filename);
   }
 
+  PointCloudFields fields = DetectPLYFields(filename);
+  RequirePointCloudHeader(fields, "PLY", filename);
+
   if (progress_callback) {
     progress_callback(0.7f, "Calculating metadata...");
   }
-
-  PointCloudFields fields = DetectPLYFields(filename);
   PointCloudMetadata metadata = CalculateMetadata(filename, "PLY", *cloud, fields);
 
   return {cloud, metadata};

src/pcl_bridge/test/unit_test/test_pcl_loader.cpp

@@ -365,6 +365,40 @@ TEST_F(PCLLoaderTest, InvalidFileError) {
                CorruptedFileException);
 }
 
+// The PLY reader is as permissive as the PCD one about malformed input, and
+// the fixture already produced a garbage .ply that nothing asserted on.
+TEST_F(PCLLoaderTest, InvalidPLYFileError) {
+  EXPECT_THROW(PointCloudLoader::LoadToPCL<pcl::PointXYZ>(test_invalid_ply_),
+               CorruptedFileException);
+}
+
+// Counterpart to the two above: rejecting a malformed file must not also
+// reject a well-formed empty one. Both end up as a zero-point cloud, so the
+// header is the only thing separating them -- this pins that the check keys
+// on the header and not on the point count.
+TEST_F(PCLLoaderTest, EmptyButValidPCDLoads) {
+  std::string filename = test_dir_ / "empty_valid.pcd";
+  {
+    std::ofstream file(filename);
+    file << "# .PCD v0.7 - Point Cloud Data file format\n"
+         << "VERSION 0.7\n"
+         << "FIELDS x y z\n"
+         << "SIZE 4 4 4\n"
+         << "TYPE F F F\n"
+         << "COUNT 1 1 1\n"
+         << "WIDTH 0\n"
+         << "HEIGHT 1\n"
+         << "VIEWPOINT 0 0 0 1 0 0 0\n"
+         << "POINTS 0\n"
+         << "DATA ascii\n";
+  }
+
+  auto result = PointCloudLoader::LoadToPCL<pcl::PointXYZ>(filename);
+  EXPECT_TRUE(result.first->points.empty());
+  EXPECT_EQ(result.second.point_count, 0u);
+  EXPECT_TRUE(result.second.fields.HasXYZ());
+}
+
 TEST_F(PCLLoaderTest, UnsupportedFormatError) {
   EXPECT_THROW(PointCloudLoader::DetectFormat("test.txt"), 
                UnsupportedFormatException);

src/scene/include/scene/feedback/object_feedback_handler.hpp

@@ -19,7 +19,14 @@
 #include <memory>
 
 #include <glm/glm.hpp>
+#ifdef VIEWER_WITH_GLAD
+#include <glad/glad.h>
+#elif defined(__APPLE__)
+// macOS ships OpenGL as a framework; there is no GL/ include directory.
+#include <OpenGL/gl.h>
+#else
 #include <GL/gl.h>
+#endif
 
 #include "scene/feedback/visual_feedback_system.hpp"
 

src/scene/src/renderable/mesh.cpp

@@ -11,6 +11,9 @@
 
 #ifdef VIEWER_WITH_GLAD
 #include <glad/glad.h>
+#elif defined(__APPLE__)
+#include <OpenGL/gl.h>
+#include <OpenGL/glext.h>
 #else
 #include <GL/gl.h>
 #include <GL/glext.h>

src/viewer/src/opengl_capability_checker.cpp

@@ -16,6 +16,8 @@
 
 #ifdef VIEWER_WITH_GLAD
 #include "glad/glad.h"
+#elif defined(__APPLE__)
+#include <OpenGL/gl.h>
 #else
 #include <GL/gl.h>
 #endif