From d6f5c4f14f5dac7e94b7646fa713d986269368e8 Mon Sep 17 00:00:00 2001 From: Ruixiang Du Date: Sun, 6 Sep 2026 15:18:41 +0800 Subject: [PATCH 1/4] build: make the OpenGL and glm dependencies portable to macOS Three Linux assumptions kept quickviz from compiling on macOS. None are fundamental -- the viewer's GL usage is already portable (it requests a 3.2 core + forward-compatible context on Apple, and the shaders are #version 330, under the 4.1 cap macOS enforces). - object_feedback_handler.hpp included unguarded, unlike mesh.cpp and opengl_capability_checker.cpp which already select glad. macOS ships OpenGL as a framework and has no GL/ directory, so this was a hard compile error. Guarded the same way, and taught the existing non-glad branches to use on Apple so VIEWER_WITH_GLAD=OFF works there too. - glad.h includes , but the vendored copy sat in include/glad/. On Linux this resolved only because Mesa happens to install /usr/include/KHR/khrplatform.h -- an undeclared dependency on a system package. Moved it to include/KHR/, glad's own standard layout, so the vendored copy is used on every platform. - glm was never referenced by CMake at all; it resolved implicitly because Ubuntu's libglm-dev installs into /usr/include. Added find_package(glm) so other prefixes work, and defined GLM_ENABLE_EXPERIMENTAL, which glm >= 1.0 requires for the GTX extensions the renderables use. Linux behaviour is unchanged; the KHR move makes it depend on less. Verified: builds clean on macOS 15 (arm64) with no external shims and no manual compile flags. The GUI itself is still unexercised -- quickviz has no automated suite and the samples need a display. --- CMakeLists.txt | 21 +++++++++++++++++++ .../feedback/object_feedback_handler.hpp | 7 +++++++ src/scene/src/renderable/mesh.cpp | 3 +++ src/viewer/src/opengl_capability_checker.cpp | 2 ++ .../glad/include/{glad => KHR}/khrplatform.h | 0 5 files changed, 33 insertions(+) rename third_party/glad/include/{glad => KHR}/khrplatform.h (100%) diff --git a/CMakeLists.txt b/CMakeLists.txt index f94a142..7a504d5 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -120,6 +120,27 @@ else () message(STATUS "quickviz test will not be built") endif () +## 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) + ## Add source directory add_subdirectory(src) add_subdirectory(sample) diff --git a/src/scene/include/scene/feedback/object_feedback_handler.hpp b/src/scene/include/scene/feedback/object_feedback_handler.hpp index 702dac6..1be51c2 100644 --- a/src/scene/include/scene/feedback/object_feedback_handler.hpp +++ b/src/scene/include/scene/feedback/object_feedback_handler.hpp @@ -19,7 +19,14 @@ #include #include +#ifdef VIEWER_WITH_GLAD +#include +#elif defined(__APPLE__) +// macOS ships OpenGL as a framework; there is no GL/ include directory. +#include +#else #include +#endif #include "scene/feedback/visual_feedback_system.hpp" diff --git a/src/scene/src/renderable/mesh.cpp b/src/scene/src/renderable/mesh.cpp index 373be03..b671092 100644 --- a/src/scene/src/renderable/mesh.cpp +++ b/src/scene/src/renderable/mesh.cpp @@ -11,6 +11,9 @@ #ifdef VIEWER_WITH_GLAD #include +#elif defined(__APPLE__) +#include +#include #else #include #include diff --git a/src/viewer/src/opengl_capability_checker.cpp b/src/viewer/src/opengl_capability_checker.cpp index 736e298..0b59194 100644 --- a/src/viewer/src/opengl_capability_checker.cpp +++ b/src/viewer/src/opengl_capability_checker.cpp @@ -16,6 +16,8 @@ #ifdef VIEWER_WITH_GLAD #include "glad/glad.h" +#elif defined(__APPLE__) +#include #else #include #endif diff --git a/third_party/glad/include/glad/khrplatform.h b/third_party/glad/include/KHR/khrplatform.h similarity index 100% rename from third_party/glad/include/glad/khrplatform.h rename to third_party/glad/include/KHR/khrplatform.h From d41aa2c81fbcda07b8218c495996ef7664dd225c Mon Sep 17 00:00:00 2001 From: Ruixiang Du Date: Sun, 6 Sep 2026 16:12:21 +0800 Subject: [PATCH 2/4] ci: add a macOS (arm64) lane, and fix what it took to get tests running The lane itself is AppleClang + libc++ on macos-14 with BUILD_TESTING=ON. Two build-plumbing problems had to be fixed before any test could run; neither is macOS-specific, both were latent. - gtest_discover_tests enumerates cases by RUNNING each test binary at BUILD time, 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 exceeds that. Discovery then times out and CMake DELETES the executable, which surfaces much later as a baffling _NOT_BUILT ctest failure. Switched to PRE_TEST discovery, which removes the build-time dependency on process startup. - OpenCV and PCL export their whole dependency prefix onto the include path. Where a system googletest exists there, its headers were found ahead of the vendored copy while the vendored libgtest.a still linked -- an ABI mismatch appearing only as undefined testing::internal symbols. The vendored include dirs are now prepended. KNOWN FAILURE on this lane: PCLLoaderTest.InvalidFileError. PCL 1.15 (what Homebrew ships) parses a garbage .pcd into a zero-point cloud and returns success, so LoadPCDInternal's `== -1` check never fires and no CorruptedFileException is thrown. That is a loader robustness gap, not a platform issue, and it will reach the Ubuntu lanes when they move past PCL 1.14. Left for a separate decision rather than changing point-cloud loading semantics here. Otherwise verified on macOS 15 (arm64): 119/121 pass, the second failure being the aggregate target that reruns the first. --- .github/workflows/default.yml | 34 ++++++++++++++++++++++++++++++++++ CMakeLists.txt | 28 ++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/.github/workflows/default.yml b/.github/workflows/default.yml index 4a6b02c..ba210aa 100644 --- a/.github/workflows/default.yml +++ b/.github/workflows/default.yml @@ -76,6 +76,40 @@ 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. + # + # KNOWN FAILURE: PCLLoaderTest.InvalidFileError. PCL 1.15 (what Homebrew + # ships) parses a garbage .pcd into a zero-point cloud and returns success, + # so LoadPCDInternal's `== -1` check never fires and no CorruptedFileException + # is thrown. This is a loader robustness gap rather than a platform issue and + # will reach the Ubuntu lanes when they move past PCL 1.14. + 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 + run: ctest -C $BUILD_TYPE --output-on-failure + minimal-build: runs-on: ${{ matrix.os }} strategy: diff --git a/CMakeLists.txt b/CMakeLists.txt index 7a504d5..8a6a65d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -120,6 +120,17 @@ 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 +# _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 @@ -141,6 +152,23 @@ endif () # 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) From 27107fe69dcb6cd1ec08dcb8eb8a42405af75305 Mon Sep 17 00:00:00 2001 From: Ruixiang Du Date: Sun, 6 Sep 2026 16:20:43 +0800 Subject: [PATCH 3/4] fix(pcl_bridge): validate the header, not just the reader's return code LoadPCDInternal/LoadPLYInternal treated a load as successful whenever PCL did not return -1. PCL's readers do not reliably signal a malformed file that way: given arbitrary non-point-cloud data, PCL 1.15 logs "number of points is zero", defaults HEIGHT, reports that x/y/z were not matched, and returns 0. Garbage therefore loaded as an empty cloud and CorruptedFileException was never thrown. The header is the reliable signal. Every PCD/PLY readable into a PCL point type declares x, y and z, so their absence means the file is not a point cloud. Crucially this stays true for a legitimately EMPTY cloud -- POINTS 0 with a full FIELDS line -- which must keep loading, so the check keys on the header rather than on the point count. Applied to all four load paths: the LoadToPCL templates and the renderer-facing LoadPCD/LoadPLY, which already had the fields in hand. AnalyzeFields is deliberately untouched: it answers "what fields does this file have", and all-false is a legitimate answer to that question. Tests: InvalidPLYFileError closes the gap where the fixture built a garbage .ply that nothing asserted on, and EmptyButValidPCDLoads pins the distinction the fix depends on -- a malformed file and a valid empty one both yield zero points, so only the header separates them. Not a macOS-specific fix. Ubuntu 22.04/24.04 ship PCL 1.12/1.14 and have not hit it yet; they will on the next PCL bump. Verified on macOS 15 (arm64): 123/123 tests pass, up from 121 with 2 failing. --- .github/workflows/default.yml | 8 ++-- src/pcl_bridge/src/pcl_loader.cpp | 38 +++++++++++++++++-- .../test/unit_test/test_pcl_loader.cpp | 34 +++++++++++++++++ 3 files changed, 71 insertions(+), 9 deletions(-) diff --git a/.github/workflows/default.yml b/.github/workflows/default.yml index ba210aa..d800a98 100644 --- a/.github/workflows/default.yml +++ b/.github/workflows/default.yml @@ -81,11 +81,9 @@ jobs: # 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. # - # KNOWN FAILURE: PCLLoaderTest.InvalidFileError. PCL 1.15 (what Homebrew - # ships) parses a garbage .pcd into a zero-point cloud and returns success, - # so LoadPCDInternal's `== -1` check never fires and no CorruptedFileException - # is thrown. This is a loader robustness gap rather than a platform issue and - # will reach the Ubuntu lanes when they move past PCL 1.14. + # 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 diff --git a/src/pcl_bridge/src/pcl_loader.cpp b/src/pcl_bridge/src/pcl_loader.cpp index 7f3e091..06757b1 100644 --- a/src/pcl_bridge/src/pcl_loader.cpp +++ b/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}; diff --git a/src/pcl_bridge/test/unit_test/test_pcl_loader.cpp b/src/pcl_bridge/test/unit_test/test_pcl_loader.cpp index 92bc113..a22b289 100644 --- a/src/pcl_bridge/test/unit_test/test_pcl_loader.cpp +++ b/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(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(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); From 680c0b213bd7ee23a754d53aea1b7950af3db5c3 Mon Sep 17 00:00:00 2001 From: Ruixiang Du Date: Sun, 6 Sep 2026 19:58:24 +0800 Subject: [PATCH 4/4] ci: settle the two macOS-lane failures - ThreadSafeQueueTest.PopTimeout asserted that a 50 ms wait returns within 100 ms. That is a claim about how promptly the OS reschedules a blocked thread, not about the queue, and a loaded runner overshoots it -- CI measured 143 ms. The lower bound is the real contract and is unchanged; the upper bound now has a wide margin and guards only against waiting forever. This was latent on Linux too. - Benchmarks are excluded from the macOS lane. They open a real GL window and the hosted macOS runner is headless. Unlike Linux, GLFW initialises fine there (Cocoa is always present) and fails only later at window creation ("NSGL: Failed to find a suitable pixel format"), so the process aborts rather than degrading. The correctness suites already skip GUI tests on their own. Linux keeps running benchmarks. Verified on macOS 15 (arm64): 123/123 tests pass with -LE benchmark. --- .github/workflows/default.yml | 7 ++++++- src/core/test/unit_test/test_thread_safe_queue.cpp | 8 +++++++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/.github/workflows/default.yml b/.github/workflows/default.yml index d800a98..f1f45d6 100644 --- a/.github/workflows/default.yml +++ b/.github/workflows/default.yml @@ -106,7 +106,12 @@ jobs: - name: Test working-directory: build shell: bash - run: ctest -C $BUILD_TYPE --output-on-failure + # 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 }} diff --git a/src/core/test/unit_test/test_thread_safe_queue.cpp b/src/core/test/unit_test/test_thread_safe_queue.cpp index 72271d3..87e8450 100644 --- a/src/core/test/unit_test/test_thread_safe_queue.cpp +++ b/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) {