diff --git a/.github/workflows/default.yml b/.github/workflows/default.yml index 4a6b02c..f1f45d6 100644 --- a/.github/workflows/default.yml +++ b/.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: diff --git a/CMakeLists.txt b/CMakeLists.txt index f94a142..8a6a65d 100644 --- a/CMakeLists.txt +++ b/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 +# _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) 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) { 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); 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