feat: Add Atracsys Visualizer for spryTrack 300 RGB (Visible, IR, Structured Light) and Marker Tracking#1520
Open
artrittelaku-wayland wants to merge 23 commits intonvidia-holoscan:mainfrom
Conversation
Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
- Migrated entire custom namespace to `holoscan::ops` - Unified redundant YAML configurations into `atracsys_visualizer.yaml` - Removed dummy python synthetic GXF generator - Updated `metadata.json` structures to strict PascalCase - Authored detailed README Flowcharts, proprietary SDK acquisition manuals, and GXF Recording guidelines - Formatted gracefully via HoloHub Container linter (`clang-format`, `ruff`, `black`) Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
… runs Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
- Fix cppcheck warnings and CTest regex - Fix geometry10.ini runtime path in YAML - Update author affiliations in metadata - Apply HoloHub linter formatting Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
…Hub PR - Update SPDX copyright headers to Wayland Technologies - Automate Replay dataset downloading via GitHub Releases - Set replayer mode as application default in metadata.json - Refine README with SDK acquisition steps, visual showcase, and default execution guidelines - Add demo GIF to documentation Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
Contributor
Greptile SummaryThis PR introduces the Several issues flagged in prior review rounds have been resolved: the Confidence Score: 4/5Safe to merge for the replayer path; live-camera path has a lingering width/height ROI swap (prior thread) and inconsistent GXF error checking that should be addressed before hardware use. Most previously-reported P0/P1 issues are resolved. The only new finding is a P2 inconsistency in GXF return-value checking in master_source_op.cpp helpers. The unresolved ROI width/height swap (prior thread, not re-raised here) remains a live-camera-only correctness concern on non-square images. Together these warrant a 4 rather than 5. operators/atracsys_camera/master_source_op.cpp — four ensure_*_output_entities helpers with unchecked GXF results; configure_structured_frame_state with swapped ROI bounds (prior thread). Important Files Changed
Sequence DiagramsequenceDiagram
participant Replayer as VideoStreamReplayerOp (×4)
participant LiveCam as AtracsysMasterSourceOp
participant Bayer as BayerDemosaicOp
participant PCF as PointCloudFilterOp
participant MS as AtracsysModeSwitcherOp
participant HV as HolovizOp
alt replayer mode
Replayer->>Bayer: out_visible_base → receiver
Replayer->>MS: output → in_ir_base
Replayer->>MS: output → in_structured_points
Replayer->>MS: output → in_marker_poses
else live_camera mode
LiveCam->>Bayer: out_visible_base → receiver
LiveCam->>MS: out_ir_base → in_ir_base
LiveCam->>MS: out_marker_poses → in_marker_poses
LiveCam->>PCF: out_disparity + out_q_matrix
PCF->>MS: out_structured_points → in_structured_points
MS->>LiveCam: out_hw_cmd → in_hw_cmd
end
Bayer->>MS: transmitter → in_visible_base
MS->>HV: out_base / out_overlay / out_marker_points / out_points / out_mode_text / out_fiducial_text_coords / out_specs
Reviews (16): Last reviewed commit: "Address Atracsys visualizer review feedb..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Atracsys Visualizer C++ application plus two operators: required Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
operators/atracsys_mode_switcher/README.md-1-30 (1)
1-30:⚠️ Potential issue | 🟡 MinorAdd requirements and a minimal usage example.
This README covers the operator purpose and ports, but it is missing explicit requirements and an example showing how an application configures/wires
AtracsysModeSwitcherOp.Suggested README sections
The application is responsible for providing a valid camera calibration and the marker geometry file. + +## Requirements + +- Holoscan SDK 4.0.0 or newer +- A hosting HoloHub application that provides visible, infrared, structured-light, and marker-pose streams +- Camera calibration values and a marker geometry file + +## Example usage + +Configure the operator in the hosting application YAML and connect the selected `out_base`, +overlay, point-cloud, and text outputs to Holoviz receivers. Enable `out_hw_cmd` only when a +live camera source consumes hardware-mode commands.As per coding guidelines, "
{applications,operators,workflows,gxf_extensions}/*/README.md: Include README.md file with purpose, usage, requirements, and examples for all contributions".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_mode_switcher/README.md` around lines 1 - 30, The README for AtracsysModeSwitcherOp is missing explicit requirements and a minimal usage example; update README.md to add a "Requirements" section listing runtime deps (e.g., required camera calibration format, marker geometry file, any Python/ROS/GXF libs or version constraints) and an "Example" section that shows minimal wiring/configuration for AtracsysModeSwitcherOp including how to connect the operator's inputs (in_visible_base, in_ir_base, in_structured_points, in_marker_poses) to sources, how to consume outputs (out_base, out_overlay, out_marker_points, out_points, out_mode_text, out_fiducial_text_coords, out_specs, out_hw_cmd), and a short snippet describing how to set camera calibration and marker geometry parameters so an application can reproduce basic usage.applications/atracsys_visualizer/README.md-30-41 (1)
30-41:⚠️ Potential issue | 🟡 MinorMinor markdownlint issues (MD022/MD031).
Several headings (lines 30, 41, 58, 69) are missing a blank line below, and several fenced code blocks (lines 81–95) are not surrounded by blank lines. Also note trailing whitespace on lines 21, 42, and 43. These are flagged by the repository's
markdownlint-cli2and should be cleaned up for CI lint compliance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/atracsys_visualizer/README.md` around lines 30 - 41, Fix markdownlint issues in README.md: add a single blank line after the "Visual Showcase" and "SDK and Data Acquisition" headings (and any other headings flagged), ensure all fenced code blocks (the block spanning lines ~81–95) are preceded and followed by a blank line, and remove trailing whitespace on the flagged lines (around the demo/description area). Locate and update the "Visual Showcase" and "SDK and Data Acquisition" headings and the fenced code block to satisfy MD022/MD031 and strip trailing spaces to pass markdownlint-cli2.applications/atracsys_visualizer/cpp/CMakeLists.txt-55-64 (1)
55-64:⚠️ Potential issue | 🟡 MinorGate the replay test on dataset availability.
With
HOLOHUB_DOWNLOAD_DATASETS=OFFor an offline configure, this test is still registered but its--datapath may not exist. Skip registration unless the replay dataset is present.Proposed test guard
-if(BUILD_TESTING) +if(BUILD_TESTING AND EXISTS "${HOLOHUB_DATA_DIR}/atracsys_visualizer/ir_base.gxf_entities") add_test(NAME atracsys_visualizer_replay_test COMMAND atracsys_visualizer --source replayer --config ${CMAKE_CURRENT_BINARY_DIR}/atracsys_visualizer_test.yaml --data "${HOLOHUB_DATA_DIR}/atracsys_visualizer" WORKING_DIRECTORY ${CMAKE_BINARY_DIR}) set_tests_properties(atracsys_visualizer_replay_test PROPERTIES PASS_REGULAR_EXPRESSION "Reach end of file or playback count reaches to the limit. Stop ticking.") +elseif(BUILD_TESTING) + message(STATUS "Skipping atracsys_visualizer_replay_test because the replay dataset is unavailable") endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/atracsys_visualizer/cpp/CMakeLists.txt` around lines 55 - 64, The replay test is registered even when the dataset may be missing; wrap the add_test/register logic in a guard that checks for the dataset directory before creating atracsys_visualizer_replay_test. Use CMake's file(EXISTS ...) or if(EXISTS ...) to test "${HOLOHUB_DATA_DIR}/atracsys_visualizer" and only call add_test / set_tests_properties when that path exists (keeping the existing BUILD_TESTING check and the same test name/arguments).operators/atracsys_camera/sdk/s3dk_wrapper.hpp-20-24 (1)
20-24:⚠️ Potential issue | 🟡 MinorAdd missing exception header and validate wrapper function arguments.
This header uses
std::runtime_errorbut does not explicitly include<stdexcept>, relying on transitive includes which is fragile. Additionally, theinitializeDeviceHelpermethod dereferencesdevice_sn,lib, andimage_typewithout null validation, violating the error handling guideline. Add the header and guard against null pointers.Proposed fix
`#include` <map> +#include <stdexcept> `#include` <string> `#include` <s3dk_gpu.hpp>bool initializeDeviceHelper(uint64_t* device_sn, ftkLibrary lib, ImageType3D* image_type) override { + if (device_sn == nullptr || image_type == nullptr || lib == nullptr) { + throw std::invalid_argument("Invalid S3DK device initialization arguments"); + } auto options = enumerate_sdk_options(lib, *device_sn);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/sdk/s3dk_wrapper.hpp` around lines 20 - 24, The header is missing <stdexcept> and initializeDeviceHelper dereferences device_sn, lib, and image_type without null checks; add `#include` <stdexcept> to operators/atracsys_camera/sdk/s3dk_wrapper.hpp and update initializeDeviceHelper to validate its pointer parameters (device_sn, lib, image_type) at entry and throw a descriptive std::runtime_error if any are null, so callers receive a clear error instead of undefined behavior.operators/atracsys_camera/sdk/sdk_wrapper.hpp-57-60 (1)
57-60:⚠️ Potential issue | 🟡 Minor
initExtdiscards vendor diagnostics on failure.When
ftkInitExtreturns a null library pointer, Atracsys typically fills the providedftkBufferwith a human-readable diagnostic (license error, USB enumeration failure, etc.). Returning a blanketFTK_ERR_INV_PTRthrows all of that away, so upstream (AtracsysMasterSourceOp::start/AtracsysDevice::init) can only report a generic "bad pointer" failure. At minimum, log or propagate the buffer contents.🛠️ Suggested fix
ftkError initExt(const char* config, ftkBuffer* buffer) override { ftkLibrary lib = ftkInitExt(config, buffer); - return (lib != nullptr) ? ftkError::FTK_OK : ftkError::FTK_ERR_INV_PTR; + if (lib != nullptr) { + return ftkError::FTK_OK; + } + if (buffer != nullptr && buffer->size > 0) { + HOLOSCAN_LOG_ERROR("ftkInitExt failed: {}", + std::string(reinterpret_cast<const char*>(buffer->data), + buffer->size)); + } + return ftkError::FTK_ERR_INV_PTR; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/sdk/sdk_wrapper.hpp` around lines 57 - 60, initExt currently maps any null return from ftkInitExt to FTK_ERR_INV_PTR and discards the ftkBuffer diagnostics; update initExt to surface the vendor diagnostics by reading or copying the provided ftkBuffer when ftkInitExt returns nullptr and returning a more informative error (or passing the buffer back to caller) so upstream callers (AtracsysMasterSourceOp::start / AtracsysDevice::init) can log/use the message; specifically, in initExt detect lib == nullptr, extract the human-readable text from the ftkBuffer parameter (or attach it to the returned error/exception) and ensure that information is propagated instead of always returning FTK_ERR_INV_PTR.operators/atracsys_mode_switcher/mode_switcher_op.cpp-491-494 (1)
491-494:⚠️ Potential issue | 🟡 Minor
CameraCalibration::valid()only checks image dimensions.
valid()(inoperators/atracsys_mode_switcher/camera_calibration.hpp) returns true as long asimage_width > 0 && image_height > 0;fx/fy/cx/cydefault to 0. A YAML file with dimensions but zero intrinsics therefore passes this guard, andproject_marker_to_overlaythen collapses every fiducial to(cx/W, cy/H) == (0, 0)without any warning. Tightenvalid()to also requirefx > 0 && fy > 0(and reasonablecx/cywithin image bounds), or add an explicit check here.🛡️ Suggested fix (in `camera_calibration.hpp`)
- [[nodiscard]] bool valid() const { return image_width > 0 && image_height > 0; } + [[nodiscard]] bool valid() const { + return image_width > 0 && image_height > 0 && fx > 0.0F && fy > 0.0F && + cx >= 0.0F && cx <= static_cast<float>(image_width) && + cy >= 0.0F && cy <= static_cast<float>(image_height); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_mode_switcher/mode_switcher_op.cpp` around lines 491 - 494, The current guard in AtracsysModeSwitcherOp (mode_switcher_op.cpp) only checks camera_calibration_ && camera_calibration_->valid(), but CameraCalibration::valid() only verifies image_width/image_height so zero intrinsics pass and break project_marker_to_overlay; update CameraCalibration::valid() to also require fx>0 && fy>0 and cx/cy inside [0,image_width/height) (or alternatively add an explicit check in mode_switcher_op.cpp after the existing guard that verifies fx>0 && fy>0 and cx/cy bounds on camera_calibration_ and throw a runtime_error with a clear message if invalid) so invalid intrinsics are caught before calling project_marker_to_overlay.applications/atracsys_visualizer/cpp/atracsys_visualizer.cpp-248-274 (1)
248-274:⚠️ Potential issue | 🟡 MinorWrap
main()in try/catch to produce a clean failure message.Any exception thrown by
parse_arguments(invalid--source),make_camera_calibration(missing/invalid YAML fields),config()/compose()/run()will currently propagate out ofmainand triggerstd::terminate, printing onlyterminate called after throwing…with no user-actionable context. This is flagged by cppcheck (throwInEntryPoint) and makes CI failures hard to diagnose.🛡️ Suggested fix
int main(int argc, char** argv) { - CommandLineOptions options; - if (!parse_arguments(argc, argv, options)) { - return 0; - } - - if (options.config_path.empty()) { - options.config_path = "atracsys_visualizer.yaml"; - } - - if (options.data_path.empty()) { - if (const char* env_path = std::getenv("HOLOSCAN_INPUT_PATH")) { - options.data_path = env_path; - } else { - options.data_path = "data/atracsys_visualizer"; - } - } - - auto app = holoscan::make_application<AtracsysVisualizerApp>(); - app->set_source(options.source); - app->set_data_path(options.data_path); - app->set_config_path(options.config_path); - app->config(options.config_path); - app->run(); - - return 0; + try { + CommandLineOptions options; + if (!parse_arguments(argc, argv, options)) { + return 0; + } + + if (options.config_path.empty()) { + options.config_path = "atracsys_visualizer.yaml"; + } + if (options.data_path.empty()) { + if (const char* env_path = std::getenv("HOLOSCAN_INPUT_PATH")) { + options.data_path = env_path; + } else { + options.data_path = "data/atracsys_visualizer"; + } + } + + auto app = holoscan::make_application<AtracsysVisualizerApp>(); + app->set_source(options.source); + app->set_data_path(options.data_path); + app->set_config_path(options.config_path); + app->config(options.config_path); + app->run(); + return 0; + } catch (const std::exception& ex) { + HOLOSCAN_LOG_ERROR("atracsys_visualizer failed: {}", ex.what()); + return 1; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/atracsys_visualizer/cpp/atracsys_visualizer.cpp` around lines 248 - 274, Wrap the entire body of main in a try/catch: put the existing logic (calls to parse_arguments, holoscan::make_application<AtracsysVisualizerApp>(), app->set_source/set_data_path/set_config_path, app->config, app->run, return 0) inside a try block and add a catch(const std::exception& e) that writes a clear failure message including e.what() to std::cerr and returns non-zero, plus a catch(...) that writes an "unknown exception" message and returns non-zero; this ensures exceptions from parse_arguments, make_camera_calibration, holoscan::make_application, AtracsysVisualizerApp::config/compose/run are handled cleanly.
🧹 Nitpick comments (16)
operators/atracsys_camera/sdk/sdk_interface.hpp (1)
27-49: Consider placingISDKInterfacein theatracsysnamespace.Other additions in this PR (e.g.
hardware_mode_command.hpp) use theatracsys::namespace. Leaving this interface at global scope risks name collisions and is inconsistent with sibling types. Consider wrapping the class innamespace atracsys { ... }and updatingRealSDKWrapperaccordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/sdk/sdk_interface.hpp` around lines 27 - 49, ISDKInterface is declared in global scope but other types use the atracsys namespace; move ISDKInterface into namespace atracsys by wrapping the class declaration in "namespace atracsys { ... }" and update all references accordingly (notably RealSDKWrapper and any uses of ISDKInterface) to qualify with atracsys:: or add appropriate using directives/imports so symbol resolution and include consistency are preserved.cmake/modules/FindS3DK.cmake (2)
22-31: Consider marking cache variables as advanced.Minor polish: hide
S3DK_INCLUDE_DIR/S3DK_LIBRARYfrom the default cmake-gui view viamark_as_advanced(S3DK_INCLUDE_DIR S3DK_LIBRARY)— standard practice for find modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/modules/FindS3DK.cmake` around lines 22 - 31, Mark the cache variables S3DK_INCLUDE_DIR and S3DK_LIBRARY as advanced so they are hidden in the default cmake-gui view; after the find_package_handle_standard_args(...) call (and before using those variables to create the S3DK::S3DK imported target) add a call to mark_as_advanced(S3DK_INCLUDE_DIR S3DK_LIBRARY) so the variables are treated as advanced options.
13-20: Redundant library name alternative.On Unix,
find_libraryautomatically prependsliband appends the platform suffix, solibs3dk_gpuis redundant alongsides3dk_gpu. Harmless, but can be dropped.♻️ Proposed tweak
find_library(S3DK_LIBRARY - NAMES s3dk_gpu libs3dk_gpu + NAMES s3dk_gpu HINTS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/modules/FindS3DK.cmake` around lines 13 - 20, The NAMES list passed to find_library for S3DK_LIBRARY redundantly includes both "s3dk_gpu" and "libs3dk_gpu"; on Unix CMake will auto-prepend "lib", so remove "libs3dk_gpu" from the NAMES list in the find_library(...) call (the invocation that sets S3DK_LIBRARY with NAMES s3dk_gpu libs3dk_gpu, HINTS ${S3DK_ROOT} $ENV{S3DK_ROOT} /opt/s3dk, PATH_SUFFIXES lib lib64) so only "s3dk_gpu" remains.operators/atracsys_camera/point_cloud_filter_op.hpp (1)
26-30: Remove the manual CUDA stream typedef.Lines 26–28 declare
cudaStream_tbut it is never used in this header; the file only referencesholoscan::CudaStreamPool. Manually mirroring CUDA types is unnecessary since Holoscan headers transitively include CUDA runtime types.Proposed cleanup
-extern "C" { -typedef struct CUstream_st* cudaStream_t; -} - `#include` "holoscan/holoscan.hpp"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/point_cloud_filter_op.hpp` around lines 26 - 30, Remove the manual CUDA typedef (the extern "C" block declaring typedef struct CUstream_st* cudaStream_t) since it is unused and Holoscan headers already provide CUDA runtime types; delete those lines and leave only the Holoscan include and usages (e.g., holoscan::CudaStreamPool) so the header no longer defines cudaStream_t redundantly.operators/atracsys_camera/CMakeLists.txt (1)
49-49: Redundantfind_package(CUDAToolkit REQUIRED).
holoscan::core(viafind_package(holoscan ...)above) transitively brings inCUDA::cudartand the otherCUDA::targets, so this explicit call is not strictly needed. Keeping it doesn't hurt, but removing it aligns with the pattern used elsewhere in HoloHub operators.Based on learnings: "In Holoscan operator CMakeLists.txt files, avoid adding a separate find_package(CUDAToolkit ...) because CUDA targets (e.g., CUDA::cuda_driver) are brought in transitively by the holoscan::core linkage through find_package(holoscan ...)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/CMakeLists.txt` at line 49, Remove the redundant explicit find_package(CUDAToolkit REQUIRED) call in CMakeLists.txt since holoscan::core (brought in by find_package(holoscan ...)) already transitively provides the CUDA::* targets; locate and delete the find_package(CUDAToolkit REQUIRED) line so the file follows the same pattern as other HoloHub operators while keeping the holoscan::core linkage intact.operators/atracsys_camera/point_cloud_filter.cu (2)
84-85: Consider a smaller block size (e.g., 16×16).32×32 = 1024 threads/block sits at the hardware maximum, which on many architectures limits occupancy (register pressure per thread + one block per SM). A 16×16 block (256 threads) is the typical sweet spot for memory-bound 2D kernels like this one and usually yields better throughput. Worth benchmarking against the actual 1280×960 disparity maps used by the replay dataset.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/point_cloud_filter.cu` around lines 84 - 85, The kernel launch uses dim3 blocks((width + 31) / 32, (height + 31) / 32) and dim3 threads(32, 32) which creates 1024-thread blocks; change these to a 16×16 block configuration (e.g., dim3 blocks((width + 15) / 16, (height + 15) / 16) and dim3 threads(16, 16)) in point_cloud_filter.cu (update the variables named blocks and threads) to reduce per-block thread count and improve occupancy for this 2D memory-bound kernel, then benchmark against your 1280×960 disparity maps to confirm performance.
26-95: LGTM on the projection logic; two optional polish items for consideration.Kernel correctly handles: non-positive disparity,
W==0singularities, non-finite coordinates, and bounds onZ/|X|/|Y|. Theraw_d / 16.0fconversion matches OpenCV's fixed-point SGBM disparity convention.Two optional improvements:
- The Q matrix is passed as 16 scalar kernel arguments. Consider using a
float4 q[4]parameter to reduce argument-passing overhead and improve call-site readability.- No
cudaGetLastError()after launch inpoint_cloud_filter_op.cpp. Consider adding error checking after the kernel call for safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/point_cloud_filter.cu` around lines 26 - 95, The PR passes the 4x4 Q matrix as 16 scalars into point_cloud_filter_kernel which is verbose and may hurt launch overhead—change point_cloud_filter_kernel signature to accept a float4 q[4] (or a const float4* q) and update its use of q00..q33 to q[0].x/... etc, and update launch_point_cloud_filter to pack h_Q into a float4 array when calling the kernel; also add a CUDA error check after invoking point_cloud_filter_kernel in launch_point_cloud_filter (call cudaGetLastError() and handle/log the error) so kernel launch failures are caught (use the existing d_out_points/stream error-handling pattern).applications/atracsys_visualizer/Dockerfile (1)
38-51: Unused FFmpeg/V4L/x264 dev headers in the builder stage.The OpenCV build explicitly sets
-D WITH_FFMPEG=OFFand-D WITH_GSTREAMER=OFF(lines 78–79), solibavcodec-dev,libavformat-dev,libswscale-dev,libv4l-dev,libx264-dev, andlibxvidcore-devdon't contribute to the built artifacts and just inflate the builder image. Consider dropping them to speed up builds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/atracsys_visualizer/Dockerfile` around lines 38 - 51, The builder stage installs FFmpeg/V4L/x264 dev packages that are unused because OpenCV is configured with -D WITH_FFMPEG=OFF and -D WITH_GSTREAMER=OFF; remove libavcodec-dev, libavformat-dev, libswscale-dev, libv4l-dev, libx264-dev, and libxvidcore-dev from the apt install list in the Dockerfile builder stage to shrink the image and speed builds, and verify no other build steps reference those packages before committing the change.applications/atracsys_visualizer/metadata.json (1)
60-90: Duplicate-DHOLOHUB_BUILD_CPP=ONand duplicate env block inlive_cameramode.Two small cleanups:
-DHOLOHUB_BUILD_CPP=ONis set in bothreplayer.build.cmake_optionsandlive_camera.build.cmake_options; theHOLOHUB_BUILD_CPPoption is a global for the repo — fine to leave, but worth confirming this is accepted by the mode-selection logic (it won't override another mode's setting at runtime).- The
live_camerablock specifies the sameAtracsys_DIR/S3DK_ROOT/OpenCV_DIRvalues under bothbuild.env(lines 86–90) and the mode-levelenv(lines 107–111). Consider consolidating to avoid drift if paths change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/atracsys_visualizer/metadata.json` around lines 60 - 90, The metadata contains duplicated settings: the global cmake flag HOLOHUB_BUILD_CPP appears in both replayer.build.cmake_options and live_camera.build.cmake_options and the Atracsys_DIR / S3DK_ROOT / OpenCV_DIR envs are declared twice (once under live_camera.build.env and again at the live_camera mode-level env); to fix, remove the redundant HOLOHUB_BUILD_CPP entry from the mode-specific cmake_options if the repo accepts a single global setting (leave it in one canonical location such as replayer.build.cmake_options or the global build block), and consolidate the duplicate environment variables by deleting the build.env block (or the mode-level env) so the live_camera mode references a single env definition (update live_camera.build.env or live_camera.env consistently and ensure Atracsys_DIR, S3DK_ROOT, OpenCV_DIR remain present in only one place).operators/atracsys_camera/master_source_op.cpp (2)
694-723: Hoist the magic54-slot scheduler-pattern length into a named constant.
std::string(54, slot_for_mode(mode))on line 696 and the two54uses inconfigured_mixed_pattern(lines 719-721) all encode the vendor's fixed Image-Scheduler-Pattern slot count. Give it a namedstatic constexpr size_t kSchedulerPatternLength = 54;near the top of the anonymous namespace so the intent is documented and future SDK bumps become a one-line change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/master_source_op.cpp` around lines 694 - 723, Introduce a named constant for the scheduler pattern length and replace hard-coded 54s: add a static constexpr size_t kSchedulerPatternLength = 54 in the anonymous namespace near the top of the file, then update AtracsysMasterSourceOp::set_exclusive_pattern to call std::string(kSchedulerPatternLength, slot_for_mode(mode)) and update AtracsysMasterSourceOp::configured_mixed_pattern to reserve/loop/resize using kSchedulerPatternLength instead of 54 (references: set_exclusive_pattern, configured_mixed_pattern, slot_for_mode, enable_visible_/enable_ir_/enable_structured_).
287-299: Consider explicit memory ordering if relying on undocumented AsynchronousCondition semantics.
capture_loop(background thread) writesframe_viaftkGetLastFrame, then signalsasync_cond_->event_state(EVENT_DONE).compute()(scheduler thread) checks state and readsframe_->imageHeader. WhileAsynchronousConditionis designed by Holoscan for exactly this multi-threaded signaling pattern and likely implements proper acquire/release semantics internally, the official documentation does not explicitly guarantee memory ordering. For clarity, consider adding an explicitstd::atomic<bool>fence (e.g.,frame_ready_withmemory_order_release/acquire) to make synchronization explicit and independent of undocumented implementation details.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/master_source_op.cpp` around lines 287 - 299, compute() reads shared pointer frame_ set by capture_loop and currently relies on async_cond_ for signaling; add an explicit std::atomic<bool> flag (e.g., frame_ready_) to make the handoff memory-safe: in capture_loop set frame_ = ftkGetLastFrame(...), then set frame_ready_.store(true, std::memory_order_release) and call async_cond_->event_state(EVENT_DONE) as before; in AtracsysMasterSourceOp::compute() first check frame_ready_.load(std::memory_order_acquire) (or use it together with the existing async_cond_ check) before dereferencing frame_->imageHeader, and reset frame_ready_.store(false, std::memory_order_release) after consuming the frame; ensure frame_ready_ is default-initialized to false (member variable) and update any error/early-return paths to leave the flag consistent.operators/atracsys_camera/sdk/helpers.cpp (1)
29-29: Avoidusing namespace std;in library TUs.This file is part of a reusable operator package (
operators/atracsys_camera) and ends up pulling the entirestdnamespace into scope for anything that ICU-includes this TU's private helpers via other headers. Qualify identifiers explicitly (std::string,std::map,std::hex,std::setw,std::cout, ...) or use targetedusing std::...aliases inside functions.As per coding guidelines: "All code must adhere to Holoscan SDK coding standards for style compliance".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/sdk/helpers.cpp` at line 29, Remove the global "using namespace std;" from helpers.cpp and instead fully qualify standard library symbols (e.g., std::string, std::map, std::hex, std::setw, std::cout) or introduce narrow using declarations scoped inside the specific functions that need them; locate the occurrence of "using namespace std;" in helpers.cpp and replace it with explicit std:: prefixes or function-local "using std::X;" lines to comply with Holoscan SDK style.applications/atracsys_visualizer/cpp/atracsys_visualizer.cpp (1)
103-109: Harden calibration parsing against missing/malformed YAML keys.Every
config["camera_calibration_*"].as<T>()throws a rawYAML::Exceptionif the key is missing or not convertible. For a user-facing config, it is worth producing a single readable error that names the missing field. Consider wrapping each.as<...>()in a helper that catches and rethrows with context ("missing camera_calibration_fx in <path>"). Same applies tocamera_calibration_distortionbeing either missing or not a sequence. This dovetails with themain()try/catch above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/atracsys_visualizer/cpp/atracsys_visualizer.cpp` around lines 103 - 109, The YAML parsing using config["camera_calibration_distortion"].as<std::vector<double>>() can throw raw YAML::Exception and doesn't validate presence/type; update parsing to use a small helper (e.g., a function like parseYamlFieldOrThrow) that checks config["camera_calibration_distortion"] exists and is a sequence, catches YAML::Exception, and rethrows a std::runtime_error with a clear message naming the missing/malformed field (e.g., "missing or invalid camera_calibration_distortion"), then use that helper to obtain the std::vector<double>, validate size against calibration->distortion.size(), and only then copy values into calibration->distortion via the existing loop so all errors are user-friendly and caught by main()'s try/catch.operators/atracsys_mode_switcher/mode_switcher_op.cpp (1)
540-545:ensure_static_entitiesonly checks the first ring slot.The early-return guard (lines 541-543) tests index
[0]of each ring buffer, but the constructor/reset paths clear allkEntityRingSizeentries. If a single call only partially populates the rings (e.g., an exception mid-loop in an earlier invocation), subsequent calls will skip re-creation and later.value()access on a reset optional will throw. Considerstd::all_of(...)across each array or guarding eachforloop with its own "needs rebuild" check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_mode_switcher/mode_switcher_op.cpp` around lines 540 - 545, The early-return in ensure_static_entities currently only inspects index 0 of each ring (hidden_overlay_entities_, hidden_marker_points_entities_, placeholder_points_entities_, blank_base_entities_, mode_text_entities_, fiducial_text_coord_entities_) so partially-populated rings can cause later .value() calls to throw; change the guard to verify all kEntityRingSize slots are populated (e.g., use std::all_of over the range [0, kEntityRingSize) for each array) or, alternatively, compute a per-array needs_rebuild boolean before each creation loop and only skip that specific loop when every slot is set; update ensure_static_entities accordingly to avoid early-return based on a single slot.operators/atracsys_mode_switcher/mode_switcher_op.hpp (1)
51-51: Prefer snake_case for public method name.Holoscan SDK public C++ APIs (and the rest of this operator's overrides
setup/start/stop/compute) are snake_case.setCameraCalibrationbreaks that convention; rename toset_camera_calibrationfor consistency. The only caller (applications/atracsys_visualizer/cpp/atracsys_visualizer.cpp:167) needs a matching update.♻️ Proposed rename
- void setCameraCalibration(std::shared_ptr<CameraCalibration> camera_calibration); + void set_camera_calibration(std::shared_ptr<CameraCalibration> camera_calibration);As per coding guidelines: "All code must adhere to Holoscan SDK coding standards for style compliance".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_mode_switcher/mode_switcher_op.hpp` at line 51, Rename the public method setCameraCalibration to snake_case set_camera_calibration across the operator API: update the declaration in mode_switcher_op.hpp, the corresponding implementation/definition (same-named function) and all call sites (e.g., the caller in atracsys_visualizer.cpp) so signatures match; ensure any forward declarations, include references, and documentation/comments are updated to use set_camera_calibration to maintain Holoscan SDK naming consistency.operators/atracsys_camera/master_source_op.hpp (1)
30-32: Remove redundantcudaStream_tforward declaration; rely on transitive include fromholoscan/holoscan.hpp.Since
holoscan/holoscan.hpptransitively includes CUDA runtime headers that definecudaStream_t, theextern "C"typedef on lines 30–32 is redundant. Delete the forward declaration and use the type from the Holoscan headers, consistent with the approach in other operators in the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/master_source_op.hpp` around lines 30 - 32, Remove the redundant extern "C" forward-declaration of cudaStream_t in master_source_op.hpp and rely on the definition provided by the transitive CUDA includes from holoscan/holoscan.hpp; locate and delete the typedef block that declares "typedef struct CUstream_st* cudaStream_t" so that all uses of cudaStream_t in functions/classes (e.g., any method parameters or members referencing cudaStream_t) use the canonical type from the Holoscan headers, matching the pattern used in other operators.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@applications/atracsys_visualizer/CMakeLists.txt`:
- Around line 26-34: Don't overwrite HOLOHUB_BUILD_OPERATORS with FORCE;
preserve and update it instead. Ensure OP_atracsys_camera is still set from
ATRACSYS_VISUALIZER_ENABLE_LIVE_CAMERA, then initialize HOLOHUB_BUILD_OPERATORS
only if undefined (e.g. if(NOT DEFINED HOLOHUB_BUILD_OPERATORS)
set(HOLOHUB_BUILD_OPERATORS "" CACHE STRING ...)). When enabling
ATRACSYS_VISUALIZER_ENABLE_LIVE_CAMERA append "atracsys_camera" to
HOLOHUB_BUILD_OPERATORS only if it isn't already present (use string or list
checks such as string(FIND) or list(FIND) and then set(HOLOHUB_BUILD_OPERATORS
"${HOLOHUB_BUILD_OPERATORS};atracsys_camera" CACHE STRING ...)). When disabling,
remove "atracsys_camera" from HOLOHUB_BUILD_OPERATORS with string(REPLACE)
(handle cases with leading/trailing/sole entry). Keep OP_atracsys_camera as a
separate CACHE BOOL and do not use FORCE to clobber existing operator lists.
- Around line 36-58: The file(DOWNLOAD) invocation that fetches the
atracsys_visualizer.zip is not pinned to a checksum, so compute the SHA-256 of
the current archive and update the file(DOWNLOAD) call to include EXPECTED_HASH
SHA256=<computed_hash> and enable TLS verification (TLS_VERIFY ON) so the
download is integrity-checked and rejects tampered artifacts; keep the existing
STATUS handling and error paths and update the same download stanza that
references "${CMAKE_CURRENT_BINARY_DIR}/atracsys_visualizer.zip" (within the
HOLOHUB_DOWNLOAD_DATASETS guard and HOLOHUB_DATA_DIR logic).
In `@applications/atracsys_visualizer/Dockerfile`:
- Around line 107-112: The Dockerfile currently creates a symlink named
libopencv_world.so.411 pointing to libopencv_world.so.4.10.0 which can mask ABI
incompatibilities; instead, confirm which SONAME S3DK was built against and make
the symlink match that exact SONAME (e.g., create libopencv_world.so.4.10 or
libopencv_world.so.410 if S3DK was built for 4.10, or only create .411 if you
have verified ABI compatibility), and if the expected OpenCV artifact is missing
fail the build early (do not silently skip) so the S3DK path cannot proceed with
a misconfigured OpenCV; also document the verified S3DK build OpenCV
version/SONAME in the README so consumers know which aliasing (if any) is safe.
In `@operators/atracsys_camera/CMakeLists.txt`:
- Around line 67-73: The current CMakeLists adds
${CMAKE_SOURCE_DIR}/operators/atracsys_mode_switcher as a PUBLIC include which
hard-codes the monorepo layout and propagates an absolute path; instead remove
that explicit path from target_include_directories for target atracsys_camera
and express the dependency by linking against the atracsys_mode_switcher target
(e.g., target_link_libraries(atracsys_camera PUBLIC/PRIVATE
atracsys_mode_switcher)) so atracsys_camera inherits interface include
directories correctly; if only internal compilation needs the headers (not
consumers), make the dependency PRIVATE rather than PUBLIC and keep
OpenCV_INCLUDE_DIRS as appropriate.
In `@operators/atracsys_camera/master_source_op.cpp`:
- Around line 198-256: The code allocates s3dk_ with new (RealS3DKWrapper) in
start() and manually deletes it in stop(), which leaks if start() throws before
stop() runs; change s3dk_ to a std::unique_ptr<RealS3DKWrapper> (or appropriate
wrapper type), replace raw new with std::make_unique in
AtracsysMasterSourceOp::start(), remove the manual delete and nulling in
AtracsysMasterSourceOp::stop(), and update any uses
(s3dk_->initializeDeviceHelper, s3dk_->createStereoParameters,
s3dk_->createDefaultEngine, s3dk_->createGpu3DFrame) to work with the unique_ptr
(use operator->); ensure header member declaration for s3dk_ is updated
accordingly so partial failures no longer leak.
- Around line 497-506: The ROI axis checks and fallback are using height/width
swapped: update the validation in the block that reads the ROI from
gpu_frame_->_ROIbox (variable roi) so x_max is compared against width and y_max
against height, and when resetting invalid ROI set roi.x_max = width and
roi.y_max = height (instead of assigning height/width swapped). Ensure the
roi_invalid expression and the fallback assignments both use width for x and
height for y.
In `@operators/atracsys_camera/point_cloud_filter_op.cpp`:
- Around line 86-101: In compute(), do not silently return when
op_input.receive<T>() yields an empty optional (e.g., q_mat_msg or
in_entity_value); instead surface the missing-input condition by throwing or
returning an explicit error/status so upstream consumers see the failure (for
example throw std::runtime_error or return a non-success Status) and only treat
malformed payloads (like q_mat_msg.value()->size() != 16) as targeted validation
failures with a WARN. Update the checks around q_mat_msg, h_Q, in_entity_value,
in_entity, and disp_tensor so missing optionals are converted to explicit
exceptions/errors rather than simply returning and masking dropped ticks.
- Around line 109-114: The code currently prefers allocating a new stream via
context.allocate_cuda_stream("point_cloud_filter_stream") before asking the
producer for its CUDA stream, which can let the filter run before the producer's
async upload completes; instead call
op_input.receive_cuda_stream("in_disparity", true, cuda_stream) first to capture
the producer's stream (into cuda_stream), then only if that call did not provide
a stream call context.allocate_cuda_stream("point_cloud_filter_stream") and use
its value; update the logic around cuda_stream, maybe_stream, and the
"in_disparity"/"point_cloud_filter_stream" calls accordingly so the filter uses
the producer's stream when available.
In `@operators/atracsys_camera/sdk/atracsys_device.hpp`:
- Around line 37-60: The init() path leaves lib_ non-null if later steps fail,
causing a partially-initialized singleton; ensure all-or-nothing by on any
failure after lib_ is set (after ftkInitExt) calling ftkClose(&lib_), setting
lib_ = nullptr, clearing sn_ (e.g. set to 0 or empty), clearing options_ and
resetting buffer_ before throwing; likewise, make shutdown() clear cached state
by calling ftkClose(&lib_) if lib_, then setting lib_ = nullptr, resetting sn_,
clearing options_, and resetting buffer_ to a safe empty state so subsequent
init() attempts start from a clean slate; reference functions/fields: init(),
shutdown(), lib_, buffer_, sn_, options_, retrieveLastDevice,
ftkEnumerateOptions, ftkClose.
In `@operators/atracsys_camera/sdk/geometry_helper.cpp`:
- Around line 67-74: The code uses OPT_DIR.assign(buffer.data) which assumes NUL
termination of ftkBuffer.data; instead perform a bounded copy using the
buffer.size returned by ftkGetData to avoid overruns—after verifying
ftkGetData(lib, 0uLL, FTK_OPT_DATA_DIR, &buffer) == ftkError::FTK_OK and
buffer.size >= 1u, assign OPT_DIR from the raw bytes with the length (e.g. using
OPT_DIR.assign(buffer.data, buffer.size) or equivalent) so the conversion uses
the provided size; ensure you reference the ftkBuffer returned from ftkGetData
and FTK_OPT_DATA_DIR in your change.
- Around line 101-109: Validate file size before reading: check streampos pos
against std::numeric_limits<uint32_t>::max() and the actual capacity of buffer
(the ftkBuffer instance used by buffer.reset()) before calling
input.read(buffer.data, ...); if pos exceeds uint32_t max or the buffer's
capacity, log an error (e.g., "File too large for buffer") and return false;
only after those checks perform input.read(...) and then set buffer.size =
static_cast<uint32_t>(pos). Also add a brief comment referencing the SDK
behavior of buffer.reset() or consult/document its guaranteed capacity if
relying on it.
In `@operators/atracsys_camera/sdk/helpers.cpp`:
- Around line 102-113: Move the free functions into the project namespace (e.g.,
wrap error() and checkError() in atracsys::sdk) to avoid global-symbol
collisions; keep or rename the throw helper but ensure it's in atracsys::sdk and
referenced by checkError(). Update checkError(ftkLibrary lib, bool
dontWaitForKeyboard, bool) to handle the getter failing: call
ftkGetLastErrorString(lib, sizeof(buf), buf) and if it returns ftkError::FTK_OK
throw with the buffer when non-empty, but if it returns any non-FTK_OK value
throw a std::runtime_error that includes the returned ftkError code (and any
available context) instead of silently proceeding. Ensure you reference the
existing symbols error(), checkError(), ftkGetLastErrorString, and
ftkError::FTK_OK when making the change so callers resolve the
namespace-qualified names.
In `@operators/atracsys_mode_switcher/camera_calibration.hpp`:
- Line 34: The current valid() method only checks image_width/image_height;
tighten it to also validate intrinsic and distortion values by ensuring fx and
fy are finite and >0, cx and cy are finite, and any distortion coefficients
(e.g., the distortion vector used by this class) are finite; use std::isfinite
checks and reject NaN/inf or zero focal lengths so that valid() returns false
for unusable calibration data (update the valid() implementation in the class
that defines image_width, image_height, fx, fy, cx, cy, and distortion).
In `@operators/atracsys_mode_switcher/mode_switcher_op.cpp`:
- Around line 842-862: The loop currently stops at the first non-empty marker
due to the `break;` after decoding a pose, so other markers are ignored; remove
the `break;` in the loop that iterates over `marker_poses_host_` so all poses
with content (checked via `pose_has_content`) are processed, and make sure
`transformed_marker_points_scratch_` is explicitly cleared before starting
marker processing (so successive runs don't accumulate old points) while
continuing to use `decode_pose_transform`, `transform_local_geometry_point`,
`marker_local_geometry_mm_`, and `scene_marker_point_buffer_` to accumulate all
transformed points.
- Around line 318-417: The code uses blocking cudaMemcpy in
add_device_tensor_to_entity and upload_to_tensor which stalls the pipeline;
change both to accept a CUDA stream (obtainable via
context.allocate_cuda_stream()) and use cudaMemcpyAsync with that stream instead
of cudaMemcpy, then set the stream on the produced tensor (e.g.,
tensor->set_cuda_stream(stream)) or propagate it via
op_output.set_cuda_stream(...) when emitting the entity; update callers
create_device_tensor_entity, create_overlay_entity, create_marker_points_entity
(and any other callers) to pass the allocated stream through to
add_device_tensor_to_entity/upload_to_tensor so uploads are asynchronous.
---
Minor comments:
In `@applications/atracsys_visualizer/cpp/atracsys_visualizer.cpp`:
- Around line 248-274: Wrap the entire body of main in a try/catch: put the
existing logic (calls to parse_arguments,
holoscan::make_application<AtracsysVisualizerApp>(),
app->set_source/set_data_path/set_config_path, app->config, app->run, return 0)
inside a try block and add a catch(const std::exception& e) that writes a clear
failure message including e.what() to std::cerr and returns non-zero, plus a
catch(...) that writes an "unknown exception" message and returns non-zero; this
ensures exceptions from parse_arguments, make_camera_calibration,
holoscan::make_application, AtracsysVisualizerApp::config/compose/run are
handled cleanly.
In `@applications/atracsys_visualizer/cpp/CMakeLists.txt`:
- Around line 55-64: The replay test is registered even when the dataset may be
missing; wrap the add_test/register logic in a guard that checks for the dataset
directory before creating atracsys_visualizer_replay_test. Use CMake's
file(EXISTS ...) or if(EXISTS ...) to test
"${HOLOHUB_DATA_DIR}/atracsys_visualizer" and only call add_test /
set_tests_properties when that path exists (keeping the existing BUILD_TESTING
check and the same test name/arguments).
In `@applications/atracsys_visualizer/README.md`:
- Around line 30-41: Fix markdownlint issues in README.md: add a single blank
line after the "Visual Showcase" and "SDK and Data Acquisition" headings (and
any other headings flagged), ensure all fenced code blocks (the block spanning
lines ~81–95) are preceded and followed by a blank line, and remove trailing
whitespace on the flagged lines (around the demo/description area). Locate and
update the "Visual Showcase" and "SDK and Data Acquisition" headings and the
fenced code block to satisfy MD022/MD031 and strip trailing spaces to pass
markdownlint-cli2.
In `@operators/atracsys_camera/sdk/s3dk_wrapper.hpp`:
- Around line 20-24: The header is missing <stdexcept> and
initializeDeviceHelper dereferences device_sn, lib, and image_type without null
checks; add `#include` <stdexcept> to
operators/atracsys_camera/sdk/s3dk_wrapper.hpp and update initializeDeviceHelper
to validate its pointer parameters (device_sn, lib, image_type) at entry and
throw a descriptive std::runtime_error if any are null, so callers receive a
clear error instead of undefined behavior.
In `@operators/atracsys_camera/sdk/sdk_wrapper.hpp`:
- Around line 57-60: initExt currently maps any null return from ftkInitExt to
FTK_ERR_INV_PTR and discards the ftkBuffer diagnostics; update initExt to
surface the vendor diagnostics by reading or copying the provided ftkBuffer when
ftkInitExt returns nullptr and returning a more informative error (or passing
the buffer back to caller) so upstream callers (AtracsysMasterSourceOp::start /
AtracsysDevice::init) can log/use the message; specifically, in initExt detect
lib == nullptr, extract the human-readable text from the ftkBuffer parameter (or
attach it to the returned error/exception) and ensure that information is
propagated instead of always returning FTK_ERR_INV_PTR.
In `@operators/atracsys_mode_switcher/mode_switcher_op.cpp`:
- Around line 491-494: The current guard in AtracsysModeSwitcherOp
(mode_switcher_op.cpp) only checks camera_calibration_ &&
camera_calibration_->valid(), but CameraCalibration::valid() only verifies
image_width/image_height so zero intrinsics pass and break
project_marker_to_overlay; update CameraCalibration::valid() to also require
fx>0 && fy>0 and cx/cy inside [0,image_width/height) (or alternatively add an
explicit check in mode_switcher_op.cpp after the existing guard that verifies
fx>0 && fy>0 and cx/cy bounds on camera_calibration_ and throw a runtime_error
with a clear message if invalid) so invalid intrinsics are caught before calling
project_marker_to_overlay.
In `@operators/atracsys_mode_switcher/README.md`:
- Around line 1-30: The README for AtracsysModeSwitcherOp is missing explicit
requirements and a minimal usage example; update README.md to add a
"Requirements" section listing runtime deps (e.g., required camera calibration
format, marker geometry file, any Python/ROS/GXF libs or version constraints)
and an "Example" section that shows minimal wiring/configuration for
AtracsysModeSwitcherOp including how to connect the operator's inputs
(in_visible_base, in_ir_base, in_structured_points, in_marker_poses) to sources,
how to consume outputs (out_base, out_overlay, out_marker_points, out_points,
out_mode_text, out_fiducial_text_coords, out_specs, out_hw_cmd), and a short
snippet describing how to set camera calibration and marker geometry parameters
so an application can reproduce basic usage.
---
Nitpick comments:
In `@applications/atracsys_visualizer/cpp/atracsys_visualizer.cpp`:
- Around line 103-109: The YAML parsing using
config["camera_calibration_distortion"].as<std::vector<double>>() can throw raw
YAML::Exception and doesn't validate presence/type; update parsing to use a
small helper (e.g., a function like parseYamlFieldOrThrow) that checks
config["camera_calibration_distortion"] exists and is a sequence, catches
YAML::Exception, and rethrows a std::runtime_error with a clear message naming
the missing/malformed field (e.g., "missing or invalid
camera_calibration_distortion"), then use that helper to obtain the
std::vector<double>, validate size against calibration->distortion.size(), and
only then copy values into calibration->distortion via the existing loop so all
errors are user-friendly and caught by main()'s try/catch.
In `@applications/atracsys_visualizer/Dockerfile`:
- Around line 38-51: The builder stage installs FFmpeg/V4L/x264 dev packages
that are unused because OpenCV is configured with -D WITH_FFMPEG=OFF and -D
WITH_GSTREAMER=OFF; remove libavcodec-dev, libavformat-dev, libswscale-dev,
libv4l-dev, libx264-dev, and libxvidcore-dev from the apt install list in the
Dockerfile builder stage to shrink the image and speed builds, and verify no
other build steps reference those packages before committing the change.
In `@applications/atracsys_visualizer/metadata.json`:
- Around line 60-90: The metadata contains duplicated settings: the global cmake
flag HOLOHUB_BUILD_CPP appears in both replayer.build.cmake_options and
live_camera.build.cmake_options and the Atracsys_DIR / S3DK_ROOT / OpenCV_DIR
envs are declared twice (once under live_camera.build.env and again at the
live_camera mode-level env); to fix, remove the redundant HOLOHUB_BUILD_CPP
entry from the mode-specific cmake_options if the repo accepts a single global
setting (leave it in one canonical location such as replayer.build.cmake_options
or the global build block), and consolidate the duplicate environment variables
by deleting the build.env block (or the mode-level env) so the live_camera mode
references a single env definition (update live_camera.build.env or
live_camera.env consistently and ensure Atracsys_DIR, S3DK_ROOT, OpenCV_DIR
remain present in only one place).
In `@cmake/modules/FindS3DK.cmake`:
- Around line 22-31: Mark the cache variables S3DK_INCLUDE_DIR and S3DK_LIBRARY
as advanced so they are hidden in the default cmake-gui view; after the
find_package_handle_standard_args(...) call (and before using those variables to
create the S3DK::S3DK imported target) add a call to
mark_as_advanced(S3DK_INCLUDE_DIR S3DK_LIBRARY) so the variables are treated as
advanced options.
- Around line 13-20: The NAMES list passed to find_library for S3DK_LIBRARY
redundantly includes both "s3dk_gpu" and "libs3dk_gpu"; on Unix CMake will
auto-prepend "lib", so remove "libs3dk_gpu" from the NAMES list in the
find_library(...) call (the invocation that sets S3DK_LIBRARY with NAMES
s3dk_gpu libs3dk_gpu, HINTS ${S3DK_ROOT} $ENV{S3DK_ROOT} /opt/s3dk,
PATH_SUFFIXES lib lib64) so only "s3dk_gpu" remains.
In `@operators/atracsys_camera/CMakeLists.txt`:
- Line 49: Remove the redundant explicit find_package(CUDAToolkit REQUIRED) call
in CMakeLists.txt since holoscan::core (brought in by find_package(holoscan
...)) already transitively provides the CUDA::* targets; locate and delete the
find_package(CUDAToolkit REQUIRED) line so the file follows the same pattern as
other HoloHub operators while keeping the holoscan::core linkage intact.
In `@operators/atracsys_camera/master_source_op.cpp`:
- Around line 694-723: Introduce a named constant for the scheduler pattern
length and replace hard-coded 54s: add a static constexpr size_t
kSchedulerPatternLength = 54 in the anonymous namespace near the top of the
file, then update AtracsysMasterSourceOp::set_exclusive_pattern to call
std::string(kSchedulerPatternLength, slot_for_mode(mode)) and update
AtracsysMasterSourceOp::configured_mixed_pattern to reserve/loop/resize using
kSchedulerPatternLength instead of 54 (references: set_exclusive_pattern,
configured_mixed_pattern, slot_for_mode,
enable_visible_/enable_ir_/enable_structured_).
- Around line 287-299: compute() reads shared pointer frame_ set by capture_loop
and currently relies on async_cond_ for signaling; add an explicit
std::atomic<bool> flag (e.g., frame_ready_) to make the handoff memory-safe: in
capture_loop set frame_ = ftkGetLastFrame(...), then set
frame_ready_.store(true, std::memory_order_release) and call
async_cond_->event_state(EVENT_DONE) as before; in
AtracsysMasterSourceOp::compute() first check
frame_ready_.load(std::memory_order_acquire) (or use it together with the
existing async_cond_ check) before dereferencing frame_->imageHeader, and reset
frame_ready_.store(false, std::memory_order_release) after consuming the frame;
ensure frame_ready_ is default-initialized to false (member variable) and update
any error/early-return paths to leave the flag consistent.
In `@operators/atracsys_camera/master_source_op.hpp`:
- Around line 30-32: Remove the redundant extern "C" forward-declaration of
cudaStream_t in master_source_op.hpp and rely on the definition provided by the
transitive CUDA includes from holoscan/holoscan.hpp; locate and delete the
typedef block that declares "typedef struct CUstream_st* cudaStream_t" so that
all uses of cudaStream_t in functions/classes (e.g., any method parameters or
members referencing cudaStream_t) use the canonical type from the Holoscan
headers, matching the pattern used in other operators.
In `@operators/atracsys_camera/point_cloud_filter_op.hpp`:
- Around line 26-30: Remove the manual CUDA typedef (the extern "C" block
declaring typedef struct CUstream_st* cudaStream_t) since it is unused and
Holoscan headers already provide CUDA runtime types; delete those lines and
leave only the Holoscan include and usages (e.g., holoscan::CudaStreamPool) so
the header no longer defines cudaStream_t redundantly.
In `@operators/atracsys_camera/point_cloud_filter.cu`:
- Around line 84-85: The kernel launch uses dim3 blocks((width + 31) / 32,
(height + 31) / 32) and dim3 threads(32, 32) which creates 1024-thread blocks;
change these to a 16×16 block configuration (e.g., dim3 blocks((width + 15) /
16, (height + 15) / 16) and dim3 threads(16, 16)) in point_cloud_filter.cu
(update the variables named blocks and threads) to reduce per-block thread count
and improve occupancy for this 2D memory-bound kernel, then benchmark against
your 1280×960 disparity maps to confirm performance.
- Around line 26-95: The PR passes the 4x4 Q matrix as 16 scalars into
point_cloud_filter_kernel which is verbose and may hurt launch overhead—change
point_cloud_filter_kernel signature to accept a float4 q[4] (or a const float4*
q) and update its use of q00..q33 to q[0].x/... etc, and update
launch_point_cloud_filter to pack h_Q into a float4 array when calling the
kernel; also add a CUDA error check after invoking point_cloud_filter_kernel in
launch_point_cloud_filter (call cudaGetLastError() and handle/log the error) so
kernel launch failures are caught (use the existing d_out_points/stream
error-handling pattern).
In `@operators/atracsys_camera/sdk/helpers.cpp`:
- Line 29: Remove the global "using namespace std;" from helpers.cpp and instead
fully qualify standard library symbols (e.g., std::string, std::map, std::hex,
std::setw, std::cout) or introduce narrow using declarations scoped inside the
specific functions that need them; locate the occurrence of "using namespace
std;" in helpers.cpp and replace it with explicit std:: prefixes or
function-local "using std::X;" lines to comply with Holoscan SDK style.
In `@operators/atracsys_camera/sdk/sdk_interface.hpp`:
- Around line 27-49: ISDKInterface is declared in global scope but other types
use the atracsys namespace; move ISDKInterface into namespace atracsys by
wrapping the class declaration in "namespace atracsys { ... }" and update all
references accordingly (notably RealSDKWrapper and any uses of ISDKInterface) to
qualify with atracsys:: or add appropriate using directives/imports so symbol
resolution and include consistency are preserved.
In `@operators/atracsys_mode_switcher/mode_switcher_op.cpp`:
- Around line 540-545: The early-return in ensure_static_entities currently only
inspects index 0 of each ring (hidden_overlay_entities_,
hidden_marker_points_entities_, placeholder_points_entities_,
blank_base_entities_, mode_text_entities_, fiducial_text_coord_entities_) so
partially-populated rings can cause later .value() calls to throw; change the
guard to verify all kEntityRingSize slots are populated (e.g., use std::all_of
over the range [0, kEntityRingSize) for each array) or, alternatively, compute a
per-array needs_rebuild boolean before each creation loop and only skip that
specific loop when every slot is set; update ensure_static_entities accordingly
to avoid early-return based on a single slot.
In `@operators/atracsys_mode_switcher/mode_switcher_op.hpp`:
- Line 51: Rename the public method setCameraCalibration to snake_case
set_camera_calibration across the operator API: update the declaration in
mode_switcher_op.hpp, the corresponding implementation/definition (same-named
function) and all call sites (e.g., the caller in atracsys_visualizer.cpp) so
signatures match; ensure any forward declarations, include references, and
documentation/comments are updated to use set_camera_calibration to maintain
Holoscan SDK naming consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ea4f293-1a26-4aa3-990f-568fdf802683
⛔ Files ignored due to path filters (1)
applications/atracsys_visualizer/demo.gifis excluded by!**/*.gif
📒 Files selected for processing (39)
applications/CMakeLists.txtapplications/atracsys_visualizer/CMakeLists.txtapplications/atracsys_visualizer/Dockerfileapplications/atracsys_visualizer/README.mdapplications/atracsys_visualizer/cpp/CMakeLists.txtapplications/atracsys_visualizer/cpp/atracsys_visualizer.cppapplications/atracsys_visualizer/cpp/atracsys_visualizer.yamlapplications/atracsys_visualizer/cpp/atracsys_visualizer_test.yamlapplications/atracsys_visualizer/cpp/geometry10.iniapplications/atracsys_visualizer/metadata.jsoncmake/modules/FindS3DK.cmakeoperators/CMakeLists.txtoperators/atracsys_camera/CMakeLists.txtoperators/atracsys_camera/README.mdoperators/atracsys_camera/master_source_op.cppoperators/atracsys_camera/master_source_op.hppoperators/atracsys_camera/metadata.jsonoperators/atracsys_camera/point_cloud_filter.cuoperators/atracsys_camera/point_cloud_filter.cuhoperators/atracsys_camera/point_cloud_filter_op.cppoperators/atracsys_camera/point_cloud_filter_op.hppoperators/atracsys_camera/sdk/atracsys_device.hppoperators/atracsys_camera/sdk/geometry_helper.cppoperators/atracsys_camera/sdk/geometry_helper.hppoperators/atracsys_camera/sdk/helpers.cppoperators/atracsys_camera/sdk/helpers.hppoperators/atracsys_camera/sdk/s3dk_interface.hppoperators/atracsys_camera/sdk/s3dk_wrapper.hppoperators/atracsys_camera/sdk/sdk_interface.hppoperators/atracsys_camera/sdk/sdk_wrapper.hppoperators/atracsys_mode_switcher/CMakeLists.txtoperators/atracsys_mode_switcher/README.mdoperators/atracsys_mode_switcher/camera_calibration.hppoperators/atracsys_mode_switcher/hardware_mode_command.hppoperators/atracsys_mode_switcher/metadata.jsonoperators/atracsys_mode_switcher/mode_switcher_keyboard.cppoperators/atracsys_mode_switcher/mode_switcher_keyboard.hppoperators/atracsys_mode_switcher/mode_switcher_op.cppoperators/atracsys_mode_switcher/mode_switcher_op.hpp
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
…ix PIE linking, and format docs Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
operators/atracsys_camera/sdk/geometry_helper.cpp (1)
98-114:⚠️ Potential issue | 🟠 MajorReplace the speculative
ftkBuffercapacity assumption with a real guard.The raw
input.read(buffer.data, ...)still assumesbuffer.reset()allocated enough space, and the comment explicitly documents uncertainty rather than enforcing the SDK contract. Please either allocate/validate the buffer capacity before reading or cite the exact SDK guarantee in a precise comment. As per coding guidelines, code contributions must include appropriate error handling and validation for invalid inputs.#!/bin/bash # Inspect available project/SDK wrapper code for ftkBuffer capacity/allocation semantics. rg -n -C4 'struct\s+ftkBuffer|class\s+ftkBuffer|typedef.*ftkBuffer|buffer\.reset\(\)|loadFileInBuffer|ftkBuffer' \ --glob '*.{h,hpp,cpp,cc}'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/sdk/geometry_helper.cpp` around lines 98 - 114, The code assumes buffer.reset() provided sufficient capacity before calling input.read(buffer.data, ...); replace this speculative behavior with an explicit guard: after buffer.reset() check the buffer's available capacity (e.g., a capacity/size/capacityRemaining accessor on ftkBuffer or that buffer.data is non-null and allocated) and if capacity < pos, either request/allocate the required capacity via the SDK's allocator API or log an error and return false; if you choose to rely on an SDK guarantee instead, replace the comment with the exact SDK call/contract name and assert that guarantee before reading (referencing buffer.reset(), ftkBuffer, buffer.data, buffer.size, input.read, and fullFilePath to locate the code).
🧹 Nitpick comments (8)
operators/atracsys_camera/CMakeLists.txt (1)
55-56: Anchor the custom module path to this file, not the top-level project.
${CMAKE_SOURCE_DIR}changes when this operator is consumed from another superbuild, soFindS3DK.cmakemay disappear even though it exists in HoloHub’scmake/modules. Prefer a path relative to thisCMakeLists.txt.♻️ Proposed CMake refactor
-list(APPEND CMAKE_MODULE_PATH "${CMAKE_SOURCE_DIR}/cmake/modules") +list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}/../../cmake/modules") find_package(S3DK REQUIRED)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/CMakeLists.txt` around lines 55 - 56, The custom module path should be anchored to this CMakeLists.txt so FindS3DK.cmake is found when consumed by a superbuild: replace usage of CMAKE_SOURCE_DIR with a path based on CMAKE_CURRENT_LIST_DIR (or CMAKE_CURRENT_SOURCE_DIR) when appending to CMAKE_MODULE_PATH, e.g. compute the correct relative traversal from this file to the repo-level cmake/modules and call list(APPEND CMAKE_MODULE_PATH "<computed-path>/cmake/modules") before find_package(S3DK REQUIRED) so find_package can find FindS3DK.cmake reliably.applications/atracsys_visualizer/README.md (2)
99-104: Clarify the4key behavior.Per
handle_keyboard_request()inmode_switcher_op.cpp, pressing4enables tracking overlayed on whatever the previous base mode was (Visible/IR/Structured). The current bullet "Tracking mode" understates this — worth aligning with the "Visual Showcase" section to avoid confusion (e.g., "Toggle tracking overlay on top of the current base mode").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/atracsys_visualizer/README.md` around lines 99 - 104, The README's "4: Tracking mode" entry is misleading; update the bullet to reflect that pressing `4` toggles a tracking overlay on top of the current base mode (Visible/Infrared/Structured) as implemented in handle_keyboard_request() in mode_switcher_op.cpp—replace "Tracking mode" with a phrase such as "Toggle tracking overlay on top of the current base mode (Visible/Infrared/Structured)" and ensure the wording matches the "Visual Showcase" section.
30-98: Minor: Markdown formatting warnings from markdownlint.markdownlint flags missing blank lines around headings (lines 30, 41, 60, 71) and fenced code blocks (lines 83, 86, 88, 94). These are style-only but trivial to fix to keep CI lint clean.
♻️ Proposed fix (representative)
-## Visual Showcase + +## Visual Showcase + Replay mode (Default): + ```bash ./holohub build atracsys_visualizer --cuda 12 ./holohub run atracsys_visualizer --cuda 12
Or, you can explicitly specify the mode:
</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@applications/atracsys_visualizer/README.mdaround lines 30 - 98, Add
required blank lines around markdown headings and fenced code blocks to satisfy
markdownlint: insert a blank line before and after the headings "## Visual
Showcase", "## SDK and Data Acquisition", "### Getting the Atracsys SDKs", and
"## Recording Your Own Data", and ensure each fenced code block in the "Build
and run" section (the bash snippets around the holohub build/run commands and
the explicit mode example) has a blank line before the openingand after the closing, so the file README.md matches markdownlint's expected spacing
rules.</details> </blockquote></details> <details> <summary>operators/atracsys_camera/sdk/helpers.cpp (1)</summary><blockquote> `29-29`: **Avoid `using namespace std;` at file scope.** At file scope this pulls the entire `std` namespace into subsequent code, which can bite later when headers add new identifiers. Prefer explicit `std::` qualification (the code already qualifies in some places, e.g. `std::runtime_error`, `std::to_string`) and drop this directive. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/sdk/helpers.cpp` at line 29, Remove the file-scope "using namespace std;" directive in helpers.cpp and replace any remaining unqualified std symbols with explicit std:: qualifiers (the file already uses std::runtime_error and std::to_string, so ensure any other identifiers are fully qualified); keep normal includes and let local scopes add using declarations only if necessary, to avoid leaking the entire std namespace into the file. ``` </details> </blockquote></details> <details> <summary>operators/atracsys_camera/master_source_op.cpp (2)</summary><blockquote> `287-290`: **Hardware-mode commands are silently dropped in `mixed` scheduler mode.** When `active_scheduler_mode_ == kMixed`, the received `in_hw_cmd` is consumed but never applied, with no log. In this mode the mode switcher still emits commands, which may make the ignore look like a wiring issue during debugging. A one-time or rate-limited `HOLOSCAN_LOG_DEBUG`/`HOLOSCAN_LOG_INFO` stating "hw_cmd ignored in mixed mode" would make this obvious. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/master_source_op.cpp` around lines 287 - 290, When receiving hardware-mode commands via op_input.receive<std::shared_ptr<atracsys::HardwareModeCommand>>("in_hw_cmd"), the code currently only applies them when active_scheduler_mode_ == SchedulerMode::kExclusive, causing silent drops when active_scheduler_mode_ == SchedulerMode::kMixed; add a one-time or rate-limited log (using HOLOSCAN_LOG_DEBUG or HOLOSCAN_LOG_INFO) in the branch where hw_cmd is present but active_scheduler_mode_ == SchedulerMode::kMixed to state that the hw_cmd was ignored in mixed mode (include enough context like command type or a short identifier), and keep the existing behavior of only calling apply_pending_command(hw_cmd.value()) for SchedulerMode::kExclusive. ``` </details> --- `258-282`: **Optional: handoff of `frame_` between capture thread and `compute()` relies on the AsyncCondition ordering.** The capture thread writes `frame_` (via `sdk_.getLastFrame`) then sets `EVENT_DONE`; `compute()` reads `frame_` after seeing `EVENT_DONE` (implicitly, because the scheduler fires `compute()` on state transition). This pattern is fine as long as `AsynchronousCondition::event_state(...)` provides the usual acquire/release semantics (which Holoscan's implementation does). Worth a brief code comment here documenting the invariant so a future refactor doesn't accidentally break the synchronization. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/master_source_op.cpp` around lines 258 - 282, The capture thread writes to the shared member frame_ via sdk_.getLastFrame(...) and then sets async_cond_->event_state(AsynchronousEventState::EVENT_DONE), while compute() reads frame_ after observing that EVENT_DONE; add a short comment in AtracsysMasterSourceOp (near capture_loop and/or where compute() handles the async condition) documenting this synchronization invariant and that AsynchronousCondition::event_state provides the necessary acquire/release memory ordering, so future refactors know frame_ is hand‑off synchronized by async_cond_->event_state and must preserve those semantics. ``` </details> </blockquote></details> <details> <summary>operators/atracsys_camera/sdk/helpers.hpp (1)</summary><blockquote> `30-34`: **Consider moving the remaining free functions into `atracsys::sdk` for symmetry.** `throwSdkError` / `checkError` were already moved to `atracsys::sdk` (good), but `deviceEnumerator`, `fusionTrackEnumerator`, `optionEnumerator`, and `retrieveLastDevice` remain in the global namespace with very generic names that can collide with other SDK wrappers or third-party code pulled in transitively. Moving them into `atracsys::sdk` (and updating callers in `atracsys_device.hpp`) would contain the symbols and keep the API consistent. Also: `<map>` at line 22 is not used in this header's declarations — it's only needed by `optionEnumerator`'s implementation, which takes `void*`. Safe to drop. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/sdk/helpers.hpp` around lines 30 - 34, The header leaves free functions deviceEnumerator, fusionTrackEnumerator, optionEnumerator, and retrieveLastDevice in the global namespace and keeps an unused <map> include; move these function declarations into the atracsys::sdk namespace (matching throwSdkError/checkError) and update all callers (e.g., in atracsys_device.hpp) to use atracsys::sdk::deviceEnumerator, ::fusionTrackEnumerator, ::optionEnumerator, and ::retrieveLastDevice, and remove the unused `#include` <map> from this header since optionEnumerator’s implementation (which needs <map>) lives in the .cpp. ``` </details> </blockquote></details> <details> <summary>operators/atracsys_mode_switcher/mode_switcher_op.cpp (1)</summary><blockquote> `820-872`: **Redundant scratch-buffer clears + inconsistent indentation; D2H pose copy bypasses the allocated stream.** Two minor things in the marker-pose handling block: 1. `transformed_marker_points_scratch_` and `scene_marker_point_buffer_` are already cleared at lines 824–825 at the top of the block. Lines 833–836 clear and reserve them again immediately, which is redundant (and the indentation on 833–836 is off by one level versus 838+). 2. Line 846 uses blocking `cudaMemcpy` on the default stream while the rest of `compute()` runs on the stream returned by `context.allocate_cuda_stream(...)`. Even though the read must complete before the subsequent host-side pose decoding, doing a default-stream copy can interleave poorly with async work queued on other streams. Using `cudaMemcpyAsync(..., cuda_stream)` followed by `cudaStreamSynchronize(cuda_stream)` keeps ordering explicit. <details> <summary>♻️ Suggested cleanup</summary> ```diff if (poses_tensor) { const uint32_t num_markers = poses_tensor.value()->shape().dimension(0); - transformed_marker_points_scratch_.clear(); - transformed_marker_points_scratch_.reserve(marker_local_geometry_mm_.size() * num_markers); - scene_marker_point_buffer_.clear(); - scene_marker_point_buffer_.reserve(marker_local_geometry_mm_.size() * 3 * num_markers); + transformed_marker_points_scratch_.reserve(marker_local_geometry_mm_.size() * num_markers); + scene_marker_point_buffer_.reserve(marker_local_geometry_mm_.size() * 3 * num_markers); marker_poses_host_.resize(num_markers * 16); const auto storage = poses_tensor.value()->storage_type(); if (storage == nvidia::gxf::MemoryStorageType::kHost || storage == nvidia::gxf::MemoryStorageType::kSystem) { std::memcpy(marker_poses_host_.data(), poses_tensor.value()->pointer(), num_markers * 16 * sizeof(float)); } else { - check_cuda(cudaMemcpy(marker_poses_host_.data(), - poses_tensor.value()->pointer(), - num_markers * 16 * sizeof(float), - cudaMemcpyDeviceToHost), - "Failed to copy marker poses to host for point extraction"); + check_cuda(cudaMemcpyAsync(marker_poses_host_.data(), + poses_tensor.value()->pointer(), + num_markers * 16 * sizeof(float), + cudaMemcpyDeviceToHost, + cuda_stream), + "Failed to copy marker poses to host for point extraction"); + check_cuda(cudaStreamSynchronize(cuda_stream), + "Failed to synchronize pose D2H copy stream"); } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_mode_switcher/mode_switcher_op.cpp` around lines 820 - 872, Remove the redundant clears/reserves and fix their indentation: delete the second clear() / reserve() calls for transformed_marker_points_scratch_ and scene_marker_point_buffer_ that appear just after obtaining num_markers (they duplicate the earlier clears at the top of the block) and ensure remaining reserve calls align with the surrounding indentation in the marker-pose handling block. Replace the blocking cudaMemcpy used to copy poses into marker_poses_host_ with an asynchronous copy on the operator's CUDA stream (use cudaMemcpyAsync with the same cuda stream obtained from context.allocate_cuda_stream(...) or the local cuda_stream variable used elsewhere), then call cudaStreamSynchronize(cuda_stream) (or check_cuda on the sync) before using marker_poses_host_ so host-side decode_pose_transform and pose_has_content operate on fully copied data; keep using check_cuda for error reporting where appropriate and reference poses_tensor, marker_poses_host_, decode_pose_transform, and transform_local_geometry_point when making changes. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@operators/atracsys_camera/CMakeLists.txt:
- Around line 47-49: The CMakeLists currently calls find_package(CUDAToolkit
REQUIRED) which can conflict with Holoscan's provided CUDA targets; remove the
explicit find_package(CUDAToolkit REQUIRED) line so the build relies on
holoscan::core's transitive CUDA targets (e.g., CUDA::cudart, CUDA::cuda_driver)
instead—locate the find_package(CUDAToolkit REQUIRED) entry in the CMakeLists
and delete it, ensuring targets in the rest of the file still reference
holoscan::core and CUDA::cuda_driver as intended.In
@operators/atracsys_camera/master_source_op.cpp:
- Around line 596-614: The null-prerequisite guard in
AtracsysMasterSourceOp::emit_structured_points misses engine_, which is later
passed into s3dk_->computeDispMap; update the initial null-check (the if that
tests s3dk_, gpu_frame_, frame_, frame_->imageHeader, and stereo_params_) to
also check engine_ and log/return the same way so computeDispMap never receives
a null engine_; ensure you reference emit_structured_points, engine_, s3dk_, and
computeDispMap when making the change.- Around line 627-658: The code assumes stereo_params_->Q_32F (Q_mat) contains
16 contiguous float elements but only checks type; update the logic to validate
Q_mat.rows==4 && Q_mat.cols==4 && Q_mat.isContinuous() before
reinterpret_casting and copying into q_msg (or else handle the error path:
log/return or construct a safe 4x4 fallback), and then copy the 16 floats safely
(e.g., use std::memcpy from Q_mat.ptr() or iterate elementwise) prior to
calling op_output.emit("out_q_matrix"); ensure you reference
stereo_params_->Q_32F, Q_mat, q_msg and op_output.emit in the change so the
check is colocated with the existing copy.In
@operators/atracsys_camera/point_cloud_filter_op.cpp:
- Around line 106-122: Validate disp_tensor shape before deriving sizes: check
that disp_tensor.value()->shape().dimension(0) and dimension(1) are positive and
that their product fits into a safe integer type (use size_t or check for
overflow before casting to int/ int32_t). Only after these validations compute
height, width, step, and total_points; if invalid, return or signal an error
rather than proceeding to allocate or launch kernels. Update references to
total_points and ensure_structured_output_entities(context, total_points) to use
the validated, safely-typed count (or reject on overflow/zero) so kernel launch
sizes and step = width * sizeof(int16_t) are never computed from invalid
dimensions.In
@operators/atracsys_camera/sdk/geometry_helper.cpp:
- Around line 67-77: The code currently returns true immediately after
populating OPT_DIR (in the OPT_DIR.empty() branch with ftkGetData), which
prevents resolving fullFilePath and causes loadRigidBody() to call
loadFileInBuffer(""); change the flow so that after successfully filling OPT_DIR
(using ftkGetData and trimming trailing '\0') you do NOT return immediately but
allow execution to continue to the candidate-path resolution that computes
fullFilePath (so callers like loadRigidBody() get a valid path); keep the same
validation of buffer.size and the OPT_DIR assignment/trim but remove the early
return and let the subsequent candidate-path check run.- Around line 22-26: The code uses std::numeric_limits<uint32_t> (in
geometry_helper.cpp) but the header is not included; add#include
to the include block at the top of geometry_helper.cpp alongside the
existing includes (e.g., with , , , ) so
std::numeric_limits is declared and the file compiles cleanly.
Duplicate comments:
In@operators/atracsys_camera/sdk/geometry_helper.cpp:
- Around line 98-114: The code assumes buffer.reset() provided sufficient
capacity before calling input.read(buffer.data, ...); replace this speculative
behavior with an explicit guard: after buffer.reset() check the buffer's
available capacity (e.g., a capacity/size/capacityRemaining accessor on
ftkBuffer or that buffer.data is non-null and allocated) and if capacity < pos,
either request/allocate the required capacity via the SDK's allocator API or log
an error and return false; if you choose to rely on an SDK guarantee instead,
replace the comment with the exact SDK call/contract name and assert that
guarantee before reading (referencing buffer.reset(), ftkBuffer, buffer.data,
buffer.size, input.read, and fullFilePath to locate the code).
Nitpick comments:
In@applications/atracsys_visualizer/README.md:
- Around line 99-104: The README's "4: Tracking mode" entry is misleading;
update the bullet to reflect that pressing4toggles a tracking overlay on top
of the current base mode (Visible/Infrared/Structured) as implemented in
handle_keyboard_request() in mode_switcher_op.cpp—replace "Tracking mode" with a
phrase such as "Toggle tracking overlay on top of the current base mode
(Visible/Infrared/Structured)" and ensure the wording matches the "Visual
Showcase" section.- Around line 30-98: Add required blank lines around markdown headings and
fenced code blocks to satisfy markdownlint: insert a blank line before and after
the headings "## Visual Showcase", "## SDK and Data Acquisition", "### Getting
the Atracsys SDKs", and "## Recording Your Own Data", and ensure each fenced
code block in the "Build and run" section (the bash snippets around the holohub
build/run commands and the explicit mode example) has a blank line before the
openingand after the closing, so the file README.md matches
markdownlint's expected spacing rules.In
@operators/atracsys_camera/CMakeLists.txt:
- Around line 55-56: The custom module path should be anchored to this
CMakeLists.txt so FindS3DK.cmake is found when consumed by a superbuild: replace
usage of CMAKE_SOURCE_DIR with a path based on CMAKE_CURRENT_LIST_DIR (or
CMAKE_CURRENT_SOURCE_DIR) when appending to CMAKE_MODULE_PATH, e.g. compute the
correct relative traversal from this file to the repo-level cmake/modules and
call list(APPEND CMAKE_MODULE_PATH "/cmake/modules") before
find_package(S3DK REQUIRED) so find_package can find FindS3DK.cmake reliably.In
@operators/atracsys_camera/master_source_op.cpp:
- Around line 287-290: When receiving hardware-mode commands via
op_input.receive<std::shared_ptratracsys::HardwareModeCommand>("in_hw_cmd"),
the code currently only applies them when active_scheduler_mode_ ==
SchedulerMode::kExclusive, causing silent drops when active_scheduler_mode_ ==
SchedulerMode::kMixed; add a one-time or rate-limited log (using
HOLOSCAN_LOG_DEBUG or HOLOSCAN_LOG_INFO) in the branch where hw_cmd is present
but active_scheduler_mode_ == SchedulerMode::kMixed to state that the hw_cmd was
ignored in mixed mode (include enough context like command type or a short
identifier), and keep the existing behavior of only calling
apply_pending_command(hw_cmd.value()) for SchedulerMode::kExclusive.- Around line 258-282: The capture thread writes to the shared member frame_ via
sdk_.getLastFrame(...) and then sets
async_cond_->event_state(AsynchronousEventState::EVENT_DONE), while compute()
reads frame_ after observing that EVENT_DONE; add a short comment in
AtracsysMasterSourceOp (near capture_loop and/or where compute() handles the
async condition) documenting this synchronization invariant and that
AsynchronousCondition::event_state provides the necessary acquire/release memory
ordering, so future refactors know frame_ is hand‑off synchronized by
async_cond_->event_state and must preserve those semantics.In
@operators/atracsys_camera/sdk/helpers.cpp:
- Line 29: Remove the file-scope "using namespace std;" directive in helpers.cpp
and replace any remaining unqualified std symbols with explicit std:: qualifiers
(the file already uses std::runtime_error and std::to_string, so ensure any
other identifiers are fully qualified); keep normal includes and let local
scopes add using declarations only if necessary, to avoid leaking the entire std
namespace into the file.In
@operators/atracsys_camera/sdk/helpers.hpp:
- Around line 30-34: The header leaves free functions deviceEnumerator,
fusionTrackEnumerator, optionEnumerator, and retrieveLastDevice in the global
namespace and keeps an unused include; move these function declarations
into the atracsys::sdk namespace (matching throwSdkError/checkError) and update
all callers (e.g., in atracsys_device.hpp) to use
atracsys::sdk::deviceEnumerator, ::fusionTrackEnumerator, ::optionEnumerator,
and ::retrieveLastDevice, and remove the unused#includefrom this header
since optionEnumerator’s implementation (which needs ) lives in the .cpp.In
@operators/atracsys_mode_switcher/mode_switcher_op.cpp:
- Around line 820-872: Remove the redundant clears/reserves and fix their
indentation: delete the second clear() / reserve() calls for
transformed_marker_points_scratch_ and scene_marker_point_buffer_ that appear
just after obtaining num_markers (they duplicate the earlier clears at the top
of the block) and ensure remaining reserve calls align with the surrounding
indentation in the marker-pose handling block. Replace the blocking cudaMemcpy
used to copy poses into marker_poses_host_ with an asynchronous copy on the
operator's CUDA stream (use cudaMemcpyAsync with the same cuda stream obtained
from context.allocate_cuda_stream(...) or the local cuda_stream variable used
elsewhere), then call cudaStreamSynchronize(cuda_stream) (or check_cuda on the
sync) before using marker_poses_host_ so host-side decode_pose_transform and
pose_has_content operate on fully copied data; keep using check_cuda for error
reporting where appropriate and reference poses_tensor, marker_poses_host_,
decode_pose_transform, and transform_local_geometry_point when making changes.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `bdcac87b-6d5a-4b9f-a895-9f3b23d1ef0c` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 0b6458878643d6f2f09d2360115a35774a5dd13e and 0241242a4b5a8cb97819657d9495bf1314b78c6d. </details> <details> <summary>📒 Files selected for processing (16)</summary> * `applications/CMakeLists.txt` * `applications/atracsys_visualizer/CMakeLists.txt` * `applications/atracsys_visualizer/Dockerfile` * `applications/atracsys_visualizer/README.md` * `operators/atracsys_camera/CMakeLists.txt` * `operators/atracsys_camera/master_source_op.cpp` * `operators/atracsys_camera/master_source_op.hpp` * `operators/atracsys_camera/point_cloud_filter_op.cpp` * `operators/atracsys_camera/point_cloud_filter_op.hpp` * `operators/atracsys_camera/sdk/atracsys_device.hpp` * `operators/atracsys_camera/sdk/geometry_helper.cpp` * `operators/atracsys_camera/sdk/helpers.cpp` * `operators/atracsys_camera/sdk/helpers.hpp` * `operators/atracsys_mode_switcher/camera_calibration.hpp` * `operators/atracsys_mode_switcher/mode_switcher_op.cpp` * `operators/atracsys_mode_switcher/mode_switcher_op.hpp` </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * applications/CMakeLists.txt </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
- Wrap ISDKInterface and IS3DKInterface in atracsys namespace - Use std::unique_ptr<RealS3DKWrapper> in master_source_op - Add try/catch block around main in applications/atracsys_visualizer.cpp - Fix break statement logic in mode_switcher_op.cpp for marker poses loop - Resolve cpplint line-length and trailing whitespace warnings - Fix markdownlint formatting issues in atracsys_visualizer README - Remove FORCE from CMakeLists.txt cache variables - Remove unused apt dependencies from Dockerfile Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
artrittelaku-wayland
force-pushed
the
feature/atracsys-visualizer-holohub
branch
from
bf9f775 to
a874bf4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
operators/atracsys_camera/point_cloud_filter_op.cpp (2)
138-148: Expose point-cloud bounding-volume limits as operator parameters.
min_z = 0.0f,max_z = 5000.0f, and themax_x/max_y = 5000.0ffilter extents are hardcoded here. These are scene/rig-dependent (units depend on the Q matrix — typically millimeters for Atracsys), and downstream users will almost certainly want to tune them per deployment without rebuilding. Consider promoting them tospec.param(...)with sensible defaults, consistent with other Holoscan operators in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/point_cloud_filter_op.cpp` around lines 138 - 148, The call to launch_point_cloud_filter hardcodes min_z (0.0f), max_z (5000.0f) and max_x/max_y (5000.0f) which should be operator parameters; add spec.param definitions (e.g., min_z, max_z, max_xy or max_x and max_y) with sensible defaults, read those params in the operator implementation, and pass them instead of the hardcoded literals to launch_point_cloud_filter so downstream users can tune the point-cloud bounding-volume without rebuilding.
81-88: Optional-unwrapping chain assumes every GXF allocation succeeds.
Handle::Create(...),Entity::New(...),add<Tensor>(...), andreshape<float>(...)each return a GXFExpected<T>, and the code dereferences them via.value()without checking — consistent with the project convention of letting failures surface as exceptions. That said, under sustained allocator pressure or device-memory exhaustion,reshape<float>can fail at runtime, which will turn into an opaquebad_optional_accessfrom GXF. Consider adding an explicit check ontensor/reshapewith a descriptive message so operational failures are diagnosable without a debugger.♻️ Proposed refinement
- auto tensor = msg.value().add<nvidia::gxf::Tensor>(kStructuredTensorName); - tensor.value()->reshape<float>(shape, nvidia::gxf::MemoryStorageType::kDevice, alloc.value()); + auto tensor = msg.value().add<nvidia::gxf::Tensor>(kStructuredTensorName); + if (!tensor) { + throw std::runtime_error("PointCloudFilterOp: failed to add structured_points tensor"); + } + const auto reshape_result = tensor.value()->reshape<float>( + shape, nvidia::gxf::MemoryStorageType::kDevice, alloc.value()); + if (!reshape_result) { + throw std::runtime_error("PointCloudFilterOp: failed to reshape structured_points tensor"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/point_cloud_filter_op.cpp` around lines 81 - 88, The code currently unwraps multiple GXF Expected<T> results (Handle::Create, Entity::New, msg.add<nvidia::gxf::Tensor>, and tensor->reshape<float>) via .value() which can throw opaque bad_optional_access on allocation/device failures; update point_cloud_filter_op.cpp to check each Expected before calling .value() (e.g., verify alloc, msg, tensor are valid and capture the Expected returned by reshape<float>()), log a clear descriptive error including the failing operation and any status/message, and early-return or raise a GXF-aware error instead of letting .value() throw; reference the symbols Handle::Create, Entity::New, add<nvidia::gxf::Tensor>, reshape<float>, and structured_output_entities_[structured_output_entity_index_] when locating and changing the code.operators/atracsys_camera/sdk/geometry_helper.cpp (1)
28-28: Avoidusing namespace std;in implementation files.Header-scope
using namespace std;pollutes all subsequent code in this translation unit and can cause ADL ambiguities as the file grows. Prefer qualifying withstd::explicitly, consistent with the rest of theatracsys_cameraSDK files.♻️ Proposed refactor
-using namespace std; - static void getDataDirOptionId(uint64_t sn, void* user, ftkOptionsInfo* oi);Then qualify
string,ifstream,cerr,ios,streampos,streamsizewithstd::throughout.As per coding guidelines (
**/*.{py,cpp,hpp,h,cc}): code must adhere to Holoscan SDK coding standards for style compliance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/sdk/geometry_helper.cpp` at line 28, Remove the header-scope "using namespace std;" and explicitly qualify all standard library symbols in this translation unit (e.g., replace bare uses of string, ifstream, cerr, ios, streampos, streamsize with std::string, std::ifstream, std::cerr, std::ios, std::streampos, std::streamsize) so the file follows Holoscan SDK style and avoids ADL/name-pollution; ensure any other std symbols in geometry_helper.cpp are similarly qualified and compile.operators/atracsys_camera/sdk/s3dk_wrapper.hpp (1)
29-78: Prefer moving these helpers into a.cpp(or keepinlinebut drop the anonymous namespace in the header).An anonymous namespace inside a header means every translation unit that includes this header gets its own distinct set of these symbols — harmless here since they're
inline, but the combination is redundant and causes unnecessary code duplication. Ifs3dk_wrapper.hpponly needs to be included by one TU (the wrapper/user), move these helpers to a.cpp. Otherwise, dropnamespace {and rely oninlinealone, or put them in a named internal namespace (e.g.,atracsys::sdk::detail).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/sdk/s3dk_wrapper.hpp` around lines 29 - 78, The helper functions enumerate_sdk_options, device_type_string, set_device_option, and configure_device_defaults in s3dk_wrapper.hpp are currently inside an anonymous namespace in a header causing redundant symbol duplication; move these helper implementations into the corresponding .cpp (e.g., s3dk_wrapper.cpp) and keep only their inline declarations or function prototypes in the header, or alternatively remove the anonymous namespace and place them in a named internal namespace (e.g., atracsys::sdk::detail) while retaining inline if they must remain in the header—update includes/usages to reference the moved functions accordingly.operators/atracsys_camera/master_source_op.cpp (1)
184-240: Consider discovering theAsynchronousConditionbefore allocating SDK resources.If the condition lookup at lines 225–235 fails,
start()throws afterframe_,stereo_params_,engine_, andgpu_frame_have been created. Holoscan will not invokestop()for a failedstart(), so these raw pointers leak until process exit. Running theasync_cond_discovery earlier (e.g., right afterreset_state()) keeps the happy path unchanged but avoids half-initialized cleanup. Same reasoning applies toset_scheduler_patternthrowing insideconfigure_cameraafter the SDK wrapper is already live.♻️ Sketch
void AtracsysMasterSourceOp::start() { reset_state(); frame_timeout_count_ = 0; first_frame_logged_ = false; first_structured_cloud_logged_ = false; + for (auto& [name, cond] : conditions()) { + if (auto async_cond = std::dynamic_pointer_cast<holoscan::AsynchronousCondition>(cond)) { + async_cond_ = async_cond; + break; + } + } + if (!async_cond_) { + throw std::runtime_error( + "AtracsysMasterSourceOp: AsyncCondition not found. The app must provide an " + "AsynchronousCondition."); + } + auto& dev = AtracsysDevice::instance(); dev.init(); ... - for (auto& [name, cond] : conditions()) { - if (auto async_cond = std::dynamic_pointer_cast<holoscan::AsynchronousCondition>(cond)) { - async_cond_ = async_cond; - break; - } - } - if (!async_cond_) { - throw std::runtime_error( - "AtracsysMasterSourceOp: AsyncCondition not found. The app must provide an " - "AsynchronousCondition."); - } - is_running_ = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/master_source_op.cpp` around lines 184 - 240, Move the AsynchronousCondition discovery out of the tail of start() so it runs immediately after reset_state() (before allocating any SDK resources or calling configure_camera()/set_scheduler_pattern()); specifically, perform the loop that finds async_cond_ (the dynamic_pointer_cast to holoscan::AsynchronousCondition) and validate async_cond_ early in start(), and if not found throw immediately, so that creation of s3dk_, frame_, stereo_params_, engine_, and gpu_frame_ (and any calls that may throw inside configure_camera()/set_scheduler_pattern) never occur when the condition is missing, avoiding leaks of s3dk_/frame_/stereo_params_/engine_/gpu_frame_.applications/atracsys_visualizer/CMakeLists.txt (1)
47-75: Guard against an undefinedHOLOHUB_DATA_DIR.When built via the top-level HoloHub CMake,
HOLOHUB_DATA_DIRis supplied, but this file does not define a fallback. If someone adds this application subtree to a build where the variable is unset, the check at line 47 evaluates against"/atracsys_visualizer/..."and the laterfile(MAKE_DIRECTORY "${HOLOHUB_DATA_DIR}")on line 65 operates on/, which is surprising. Either fail fast or pick a sane default.if(HOLOHUB_DOWNLOAD_DATASETS AND NOT EXISTS "${HOLOHUB_DATA_DIR}/atracsys_visualizer/ir_base.gxf_entities") + if(NOT HOLOHUB_DATA_DIR) + message(FATAL_ERROR "HOLOHUB_DATA_DIR is not set; cannot download atracsys_visualizer dataset.") + endif() set(ATRACSYS_VISUALIZER_DATASET_URL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/atracsys_visualizer/CMakeLists.txt` around lines 47 - 75, Add a defensive check for HOLOHUB_DATA_DIR before the existing data-download block: if the variable is undefined or empty (i.e., if(NOT DEFINED HOLOHUB_DATA_DIR OR HOLOHUB_DATA_DIR STREQUAL "")) emit a clear message(FATAL_ERROR "...") explaining that HOLOHUB_DATA_DIR must be set (or that HOLOHUB_DOWNLOAD_DATASETS must be turned off), placing this check above the if(HOLOHUB_DOWNLOAD_DATASETS ...) guard so later calls like file(MAKE_DIRECTORY "${HOLOHUB_DATA_DIR}") and the download/extract logic never operate on root or an unintended path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@applications/atracsys_visualizer/cpp/atracsys_visualizer.cpp`:
- Around line 248-282: The call to parse_arguments in main can throw (e.g.,
std::runtime_error for unsupported --source) but is executed before the
try/catch, letting exceptions escape; to fix, move the parse_arguments(argc,
argv, options) invocation (and any related defaulting of
options.config_path/options.data_path) inside the existing try block that
surrounds application creation and run, so any exceptions thrown by
parse_arguments are caught by the catch blocks; alternatively, modify
parse_arguments to catch its own exceptions and return false on invalid input,
ensuring parse_arguments is exception-safe — update references to
parse_arguments, CommandLineOptions, and main accordingly.
In `@operators/atracsys_camera/sdk/atracsys_device.hpp`:
- Around line 41-44: The current error path calls throw
std::runtime_error(buffer_.data ? buffer_.data : "ftkInitExt failed") which
assumes buffer_.data is NUL-terminated; instead construct a std::string (or
std::string_view) from buffer_.data and buffer_.size when buffer_.data is
non-null so the size-tracked buffer isn't read past its length, and pass that
string to std::runtime_error; update the ftkInitExt failure branch (references:
ftkInitExt, buffer_, ftkLibrary/new_lib) to check buffer_.data and buffer_.size
and build the exception message from those values, falling back to a literal
message if data is null or size is zero.
In `@operators/atracsys_camera/sdk/geometry_helper.cpp`:
- Around line 53-78: FTK_OPT_DATA_DIR and OPT_DIR are function-local static
caches that are mutated without synchronization (risking data races when
getFullFilePath/loadRigidBody run concurrently); fix by making their one-time
initialization thread-safe: either replace the lazy static pattern with
std::once_flag + std::call_once to perform the ftkEnumerateOptions/ftkGetData
initialization exactly once, or move the cache into the AtracsysDevice instance
and protect access with the existing std::mutex in that class (update references
in getFullFilePath/loadRigidBody to use the guarded member variables instead of
the function-local statics).
In `@operators/atracsys_camera/sdk/helpers.cpp`:
- Around line 59-61: The error check after calling ftkEnumerateDevices uses "err
> ftkError::FTK_OK" which can misclassify warnings and negative codes; change
the condition to "err != ftkError::FTK_OK" so any non-OK return (err from
ftkEnumerateDevices) is treated as error and triggers the throw; update the
check around the err variable where ftkEnumerateDevices is called and keep the
same std::runtime_error message.
In `@operators/atracsys_camera/sdk/s3dk_wrapper.hpp`:
- Around line 29-38: The anonymous-namespace function enumerate_sdk_options
calls optionEnumerator but optionEnumerator is declared in atracsys::sdk
(helpers.hpp) so the unqualified symbol will not resolve; fix by qualifying or
bringing it into scope: either reference atracsys::sdk::optionEnumerator
directly in the call to ftkEnumerateOptions inside enumerate_sdk_options, or add
a using declaration (e.g., using atracsys::sdk::optionEnumerator) at the top of
the anonymous namespace; ensure the change references optionEnumerator and
leaves ftkEnumerateOptions, enumerate_sdk_options, and the existing signature
unchanged.
- Around line 64-66: The call in set_device_option currently only throws when
ftkSetInt32(...) != ftkError::FTK_OK && required, which swallows SDK failures
when required==false; change the logic so any non-FTK_OK return from ftkSetInt32
causes an exception (remove the required check) and include the returned status
or descriptive text in the std::runtime_error message; reference ftkSetInt32,
ftkError::FTK_OK, set_device_option and options.find(name) to locate where to
replace the conditional and construct the thrown error.
In `@operators/atracsys_camera/sdk/sdk_wrapper.hpp`:
- Around line 18-22: The header RealSDKWrapper (class RealSDKWrapper in
sdk_wrapper.hpp) uses std::runtime_error in initExt but does not include
<stdexcept>; add an explicit `#include` <stdexcept> at the top of sdk_wrapper.hpp
so the use of std::runtime_error is guaranteed and not relying on transitive
includes.
---
Nitpick comments:
In `@applications/atracsys_visualizer/CMakeLists.txt`:
- Around line 47-75: Add a defensive check for HOLOHUB_DATA_DIR before the
existing data-download block: if the variable is undefined or empty (i.e.,
if(NOT DEFINED HOLOHUB_DATA_DIR OR HOLOHUB_DATA_DIR STREQUAL "")) emit a clear
message(FATAL_ERROR "...") explaining that HOLOHUB_DATA_DIR must be set (or that
HOLOHUB_DOWNLOAD_DATASETS must be turned off), placing this check above the
if(HOLOHUB_DOWNLOAD_DATASETS ...) guard so later calls like file(MAKE_DIRECTORY
"${HOLOHUB_DATA_DIR}") and the download/extract logic never operate on root or
an unintended path.
In `@operators/atracsys_camera/master_source_op.cpp`:
- Around line 184-240: Move the AsynchronousCondition discovery out of the tail
of start() so it runs immediately after reset_state() (before allocating any SDK
resources or calling configure_camera()/set_scheduler_pattern()); specifically,
perform the loop that finds async_cond_ (the dynamic_pointer_cast to
holoscan::AsynchronousCondition) and validate async_cond_ early in start(), and
if not found throw immediately, so that creation of s3dk_, frame_,
stereo_params_, engine_, and gpu_frame_ (and any calls that may throw inside
configure_camera()/set_scheduler_pattern) never occur when the condition is
missing, avoiding leaks of s3dk_/frame_/stereo_params_/engine_/gpu_frame_.
In `@operators/atracsys_camera/point_cloud_filter_op.cpp`:
- Around line 138-148: The call to launch_point_cloud_filter hardcodes min_z
(0.0f), max_z (5000.0f) and max_x/max_y (5000.0f) which should be operator
parameters; add spec.param definitions (e.g., min_z, max_z, max_xy or max_x and
max_y) with sensible defaults, read those params in the operator implementation,
and pass them instead of the hardcoded literals to launch_point_cloud_filter so
downstream users can tune the point-cloud bounding-volume without rebuilding.
- Around line 81-88: The code currently unwraps multiple GXF Expected<T> results
(Handle::Create, Entity::New, msg.add<nvidia::gxf::Tensor>, and
tensor->reshape<float>) via .value() which can throw opaque bad_optional_access
on allocation/device failures; update point_cloud_filter_op.cpp to check each
Expected before calling .value() (e.g., verify alloc, msg, tensor are valid and
capture the Expected returned by reshape<float>()), log a clear descriptive
error including the failing operation and any status/message, and early-return
or raise a GXF-aware error instead of letting .value() throw; reference the
symbols Handle::Create, Entity::New, add<nvidia::gxf::Tensor>, reshape<float>,
and structured_output_entities_[structured_output_entity_index_] when locating
and changing the code.
In `@operators/atracsys_camera/sdk/geometry_helper.cpp`:
- Line 28: Remove the header-scope "using namespace std;" and explicitly qualify
all standard library symbols in this translation unit (e.g., replace bare uses
of string, ifstream, cerr, ios, streampos, streamsize with std::string,
std::ifstream, std::cerr, std::ios, std::streampos, std::streamsize) so the file
follows Holoscan SDK style and avoids ADL/name-pollution; ensure any other std
symbols in geometry_helper.cpp are similarly qualified and compile.
In `@operators/atracsys_camera/sdk/s3dk_wrapper.hpp`:
- Around line 29-78: The helper functions enumerate_sdk_options,
device_type_string, set_device_option, and configure_device_defaults in
s3dk_wrapper.hpp are currently inside an anonymous namespace in a header causing
redundant symbol duplication; move these helper implementations into the
corresponding .cpp (e.g., s3dk_wrapper.cpp) and keep only their inline
declarations or function prototypes in the header, or alternatively remove the
anonymous namespace and place them in a named internal namespace (e.g.,
atracsys::sdk::detail) while retaining inline if they must remain in the
header—update includes/usages to reference the moved functions accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05a14879-cf28-4f94-b372-659787627f51
📒 Files selected for processing (20)
applications/atracsys_visualizer/CMakeLists.txtapplications/atracsys_visualizer/Dockerfileapplications/atracsys_visualizer/README.mdapplications/atracsys_visualizer/cpp/atracsys_visualizer.cppoperators/atracsys_camera/CMakeLists.txtoperators/atracsys_camera/README.mdoperators/atracsys_camera/master_source_op.cppoperators/atracsys_camera/master_source_op.hppoperators/atracsys_camera/point_cloud_filter_op.cppoperators/atracsys_camera/sdk/atracsys_device.hppoperators/atracsys_camera/sdk/geometry_helper.cppoperators/atracsys_camera/sdk/helpers.cppoperators/atracsys_camera/sdk/helpers.hppoperators/atracsys_camera/sdk/s3dk_interface.hppoperators/atracsys_camera/sdk/s3dk_wrapper.hppoperators/atracsys_camera/sdk/sdk_interface.hppoperators/atracsys_camera/sdk/sdk_wrapper.hppoperators/atracsys_mode_switcher/CMakeLists.txtoperators/atracsys_mode_switcher/README.mdoperators/atracsys_mode_switcher/mode_switcher_op.cpp
✅ Files skipped from review due to trivial changes (3)
- operators/atracsys_camera/README.md
- applications/atracsys_visualizer/README.md
- operators/atracsys_mode_switcher/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- operators/atracsys_mode_switcher/CMakeLists.txt
- operators/atracsys_camera/CMakeLists.txt
artrittelaku-wayland
force-pushed
the
feature/atracsys-visualizer-holohub
branch
from
39edd79 to
947fcd3
Compare
artrittelaku-wayland
force-pushed
the
feature/atracsys-visualizer-holohub
branch
from
947fcd3 to
c9eb488
Compare
artrittelaku-wayland
force-pushed
the
feature/atracsys-visualizer-holohub
branch
from
c9eb488 to
c9e1bf8
Compare
artrittelaku-wayland
force-pushed
the
feature/atracsys-visualizer-holohub
branch
2 times, most recently
from
17e075a to
4ed9886
Compare
- Move parse_arguments inside try/catch in main to prevent uncaught exceptions from std::terminate on bad CLI input - Use size-tracked std::string construction for ftkBuffer error messages in atracsys_device.hpp to avoid reading past buffer length - Guard static cache initialization in geometry_helper.cpp with std::once_flag/std::call_once for thread safety - Fix ftkEnumerateDevices error check from '> FTK_OK' to '!= FTK_OK' in helpers.cpp to correctly catch all non-OK returns - Qualify optionEnumerator as atracsys::sdk::optionEnumerator and move anonymous namespace helpers to atracsys::sdk::detail in s3dk_wrapper.hpp - Always throw on ftkSetInt32 failure in set_device_option regardless of required flag, include status code in error message - Add explicit #include <stdexcept> and <string> to sdk_wrapper.hpp - Add defensive HOLOHUB_DATA_DIR check in CMakeLists.txt - Move AsynchronousCondition discovery to start of start() in master_source_op.cpp to avoid resource leaks on missing condition - Expose min_z/max_z/max_x/max_y as operator parameters in PointCloudFilterOp instead of hardcoded literals - Add proper Expected<T> validation before .value() calls in point_cloud_filter_op.cpp for clearer error diagnostics - Qualify std::ios, std::streamsize, std::cerr in geometry_helper.cpp Signed-off-by: Artrit Telaku <artrit@waylandio.com> Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
artrittelaku-wayland
force-pushed
the
feature/atracsys-visualizer-holohub
branch
from
4ed9886 to
96e4d66
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
applications/atracsys_visualizer/CMakeLists.txt (1)
36-45: Avoid the leading-semicolon concat when appending toHOLOHUB_BUILD_OPERATORS.On line 38, if the existing cache value is empty (the common fresh-configure case, since line 33 initializes it to
""), the interpolation produces";atracsys_camera", i.e. a list whose first element is empty. Consumers that iterate the list or pass it tolist(FIND)/ macros likeadd_holohub_operatorcan misbehave on the stray empty entry. Uselist(APPEND)(which handles the empty case correctly) before writing back to the cache.Proposed refactor
if(ATRACSYS_VISUALIZER_ENABLE_LIVE_CAMERA) if(NOT "atracsys_camera" IN_LIST HOLOHUB_BUILD_OPERATORS) - set(HOLOHUB_BUILD_OPERATORS "${HOLOHUB_BUILD_OPERATORS};atracsys_camera" CACHE STRING "Operators to build dynamically" FORCE) + list(APPEND HOLOHUB_BUILD_OPERATORS "atracsys_camera") + list(REMOVE_DUPLICATES HOLOHUB_BUILD_OPERATORS) + set(HOLOHUB_BUILD_OPERATORS "${HOLOHUB_BUILD_OPERATORS}" CACHE STRING "Operators to build dynamically" FORCE) endif() else() if("atracsys_camera" IN_LIST HOLOHUB_BUILD_OPERATORS) list(REMOVE_ITEM HOLOHUB_BUILD_OPERATORS "atracsys_camera") set(HOLOHUB_BUILD_OPERATORS "${HOLOHUB_BUILD_OPERATORS}" CACHE STRING "Operators to build dynamically" FORCE) endif() endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/atracsys_visualizer/CMakeLists.txt` around lines 36 - 45, The HOLOHUB_BUILD_OPERATORS cache is being updated by concatenating strings which yields a leading empty-list element when the cache is initially empty; replace the manual semicolon concat in the ATRACSYS_VISUALIZER_ENABLE_LIVE_CAMERA true branch by using list(APPEND HOLOHUB_BUILD_OPERATORS "atracsys_camera") to correctly handle empty lists, then write the updated variable back into the cache with set(HOLOHUB_BUILD_OPERATORS "${HOLOHUB_BUILD_OPERATORS}" CACHE STRING "Operators to build dynamically" FORCE); keep the existing list(REMOVE_ITEM ...) + cache set in the else branch for removal of "atracsys_camera".operators/atracsys_camera/sdk/geometry_helper.cpp (1)
84-92:fromSystemnot reset on the failure path.When the candidate file cannot be opened, the function returns
falsewithout writing to*fromSystem. Callers (e.g.,loadRigidBody) initializefromDataDir = false, so this happens to be harmless today, but it's an implicit contract. Consider explicitly setting*fromSystem = falsebefore the finalreturn falseto make the postcondition unambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/sdk/geometry_helper.cpp` around lines 84 - 92, The failure path currently returns false without setting the output flag, so explicitly set the out-parameter to false before returning; in the function that computes candidate and opens system_input, ensure that if system_input.is_open() is false you do "if (fromSystem != nullptr) *fromSystem = false" (mirroring the true branch) so callers like loadRigidBody see a deterministic postcondition for fromSystem even when the candidate file can't be opened.operators/atracsys_camera/point_cloud_filter_op.cpp (1)
155-158: Uncheckedget<Tensor>()result.
out_gxf_entity.get<nvidia::gxf::Tensor>(kStructuredTensorName)is dereferenced directly viatensor.value()->pointer()without checking success. This should be an invariant afterensure_structured_output_entities()— the tensor was just added and reshaped — but a defensive check (or at least aHOLOSCAN_EXPECT) keeps diagnostics clear if the invariant ever breaks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/point_cloud_filter_op.cpp` around lines 155 - 158, The code dereferences the optional result of out_gxf_entity.get<nvidia::gxf::Tensor>(kStructuredTensorName) directly (used to call pointer()), which is unsafe; update the block that reads structured_output_entities_ / out_gxf_entity to check the get result before using it (e.g., test tensor.has_value() or use HOLOSCAN_EXPECT/tidy assertion) and only call tensor.value()->pointer() when present, otherwise log/error out; reference the symbols structured_output_entities_, structured_output_entity_index_, nvidia::gxf::Entity, get<nvidia::gxf::Tensor>, kStructuredTensorName, and pointer() when making the change.operators/atracsys_camera/sdk/atracsys_device.hpp (1)
37-71: Buffer lifecycle inconsistency: missing reset after successful init.The concern is valid. The codebase establishes a pattern of calling
buffer.reset()before using buffers (seen in geometry_helper.cpp line 102), yetatracsys_device.hppnever calls it afterftkInitExt()succeeds. Although callers like master_source_op.cpp manually reset the buffer before use, this is inconsistent and error-prone. Abuffer_.reset()call after the options assignment (line 66) would align with the codebase's own patterns and ensure proper cleanup of any data populated byftkInitExt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/sdk/atracsys_device.hpp` around lines 37 - 71, The init() function calls ftkInitExt(...) which populates buffer_, but never resets it afterward; to follow the project's buffer lifecycle pattern, after successfully assigning lib_, sn_, and options_ (i.e., after setting lib_ = new_lib; sn_ = serial; options_ = std::move(options);) call buffer_.reset() to clear the held data; ensure this happens before leaving init() and still occurs inside the successful path (not the catch where ftkClose is used).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@applications/atracsys_visualizer/CMakeLists.txt`:
- Around line 47-49: The unconditional check for HOLOHUB_DATA_DIR causes
configuration to fail even when datasets are disabled; move the if(NOT DEFINED
HOLOHUB_DATA_DIR OR HOLOHUB_DATA_DIR STREQUAL "") guard so it only runs when
HOLOHUB_DOWNLOAD_DATASETS is ON (or where HOLOHUB_DATA_DIR is actually
consumed). Concretely, wrap or relocate the HOLOHUB_DATA_DIR presence validation
inside the HOLOHUB_DOWNLOAD_DATASETS branch (or the code path that uses
datasets/replay), ensuring that when HOLOHUB_DOWNLOAD_DATASETS is OFF (or
live-camera-only), CMake does not require HOLOHUB_DATA_DIR.
In `@applications/atracsys_visualizer/cpp/atracsys_visualizer.cpp`:
- Around line 182-185: The PointCloudFilterOp instance named
"point_cloud_filter" is constructed without loading its YAML config, so runtime
filter parameters aren't applied in live_camera mode; update the operator
creation (the make_operator call that constructs point_cloud_filter) to include
from_config("point_cloud_filter") so the operator reads the YAML section and
wires the exposed filter parameters into the live graph (i.e., use
PointCloudFilterOp configured via from_config("point_cloud_filter") instead of
the current constructor-only form, keeping the existing
Arg("structured_allocator") and Arg("cuda_stream_pool") arguments).
In `@operators/atracsys_camera/master_source_op.cpp`:
- Around line 203-249: Move the AtracsysDevice::instance().init() call inside
the try block and, in the catch(...), clean up any partially initialized
operator resources before rethrowing: stop/join the capture_thread_ if started,
reset is_running_ and set async_cond_->event_state back if needed, destroy/free
frame_ via the SDK, release s3dk_ created resources (engine_, gpu_frame_,
stereo_params_) and any other unique members created after
s3dk_->initializeDeviceHelper (frame_, engine_, gpu_frame_, stereo_params_,
capture_thread_, is_running_, async_cond_), then call
AtracsysDevice::instance().shutdown() and rethrow; ensure the cleanup order
handles nullptr/uninitialized checks so double-free is avoided.
In `@operators/atracsys_camera/point_cloud_filter_op.cpp`:
- Around line 180-185: Add a clear inline comment near the ring-buffer
emit/reset sequence (around op_output.emit, structured_output_entities_, and
structured_output_entity_index_) stating that this emit-then-reset pattern is
only safe under the Greedy Scheduler (single worker, sequential execution);
explain that op_output.emit() takes its reference to the entity synchronously
before returning, so immediately calling
structured_output_entities_[structured_output_entity_index_].reset() relies on
sequential execution and would be unsafe under MultiThreadScheduler or
EventBasedScheduler. Document that this design intentionally mirrors the
Greedy-only approach used in st2110_source to avoid concurrent reuse of entity
slots.
In `@operators/atracsys_camera/sdk/geometry_helper.cpp`:
- Around line 57-74: Add a brief inline comment next to the ftkEnumerateOptions
call explaining that when called with serial 0uLL (global scope) the SDK returns
FTK_WAR_OPT_GLOBAL_ONLY (not FTK_OK) to indicate options are global-only;
reference ftkEnumerateOptions, FTK_WAR_OPT_GLOBAL_ONLY, getDataDirOptionId, and
FTK_OPT_DATA_DIR in the comment and note that the code intentionally checks for
that specific warning so OPT_DIR is populated only when the global option ID is
returned.
In `@operators/atracsys_camera/sdk/s3dk_wrapper.hpp`:
- Around line 40-52: The device_type_string function builds a std::string from
ftkBuffer using buffer.size which can include a trailing NUL; after creating the
string (from buffer.data, buffer.size) check if the string is non-empty and its
last character is '\0' and remove it (e.g., pop_back()) so
convert_string_image_type sees the trimmed value; keep the rest of the logic
(ftkGetData, error throws, options lookup) unchanged and reference ftkBuffer,
ftkGetData, device_type_string and the "sTk device type" option when making the
change.
In `@operators/atracsys_camera/sdk/sdk_wrapper.hpp`:
- Around line 60-70: The initExt implementation must avoid unbounded reads and
must honor the ftkError return contract: when ftkInitExt returns null, build the
error string using the buffer size (e.g., construct a
std::string(reinterpret_cast<const char*>(buffer->data), buffer->size) or
equivalent) instead of assuming NUL-termination, and then return an appropriate
non-FTK_OK ftkError value rather than throwing; update initExt (and any callers
if needed) to return the error code on failure (or, if you prefer exception
semantics, change the function signature to void in the interface and
implementation consistently) so that initExt and the base/interface agree.
---
Nitpick comments:
In `@applications/atracsys_visualizer/CMakeLists.txt`:
- Around line 36-45: The HOLOHUB_BUILD_OPERATORS cache is being updated by
concatenating strings which yields a leading empty-list element when the cache
is initially empty; replace the manual semicolon concat in the
ATRACSYS_VISUALIZER_ENABLE_LIVE_CAMERA true branch by using list(APPEND
HOLOHUB_BUILD_OPERATORS "atracsys_camera") to correctly handle empty lists, then
write the updated variable back into the cache with set(HOLOHUB_BUILD_OPERATORS
"${HOLOHUB_BUILD_OPERATORS}" CACHE STRING "Operators to build dynamically"
FORCE); keep the existing list(REMOVE_ITEM ...) + cache set in the else branch
for removal of "atracsys_camera".
In `@operators/atracsys_camera/point_cloud_filter_op.cpp`:
- Around line 155-158: The code dereferences the optional result of
out_gxf_entity.get<nvidia::gxf::Tensor>(kStructuredTensorName) directly (used to
call pointer()), which is unsafe; update the block that reads
structured_output_entities_ / out_gxf_entity to check the get result before
using it (e.g., test tensor.has_value() or use HOLOSCAN_EXPECT/tidy assertion)
and only call tensor.value()->pointer() when present, otherwise log/error out;
reference the symbols structured_output_entities_,
structured_output_entity_index_, nvidia::gxf::Entity, get<nvidia::gxf::Tensor>,
kStructuredTensorName, and pointer() when making the change.
In `@operators/atracsys_camera/sdk/atracsys_device.hpp`:
- Around line 37-71: The init() function calls ftkInitExt(...) which populates
buffer_, but never resets it afterward; to follow the project's buffer lifecycle
pattern, after successfully assigning lib_, sn_, and options_ (i.e., after
setting lib_ = new_lib; sn_ = serial; options_ = std::move(options);) call
buffer_.reset() to clear the held data; ensure this happens before leaving
init() and still occurs inside the successful path (not the catch where ftkClose
is used).
In `@operators/atracsys_camera/sdk/geometry_helper.cpp`:
- Around line 84-92: The failure path currently returns false without setting
the output flag, so explicitly set the out-parameter to false before returning;
in the function that computes candidate and opens system_input, ensure that if
system_input.is_open() is false you do "if (fromSystem != nullptr) *fromSystem =
false" (mirroring the true branch) so callers like loadRigidBody see a
deterministic postcondition for fromSystem even when the candidate file can't be
opened.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f6cd2da-0ae9-4559-85f5-84cc400b7b3a
📒 Files selected for processing (11)
applications/atracsys_visualizer/CMakeLists.txtapplications/atracsys_visualizer/cpp/atracsys_visualizer.cppoperators/atracsys_camera/master_source_op.cppoperators/atracsys_camera/master_source_op.hppoperators/atracsys_camera/point_cloud_filter_op.cppoperators/atracsys_camera/point_cloud_filter_op.hppoperators/atracsys_camera/sdk/atracsys_device.hppoperators/atracsys_camera/sdk/geometry_helper.cppoperators/atracsys_camera/sdk/helpers.cppoperators/atracsys_camera/sdk/s3dk_wrapper.hppoperators/atracsys_camera/sdk/sdk_wrapper.hpp
artrittelaku-wayland
force-pushed
the
feature/atracsys-visualizer-holohub
branch
from
b024782 to
e36dfa2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
operators/atracsys_camera/sdk/s3dk_wrapper.hpp (1)
40-52:⚠️ Potential issue | 🟡 MinorTrailing NUL can still bleed into the device-type string.
ftkGetDatafor string options returnsbuffer.sizecounting the trailing'\0', sostd::string(buffer.data, buffer.size)produces a string with an embedded NUL.convert_string_image_type("sTk 300\0", ...)will then fail to match the expected literal and causeinitializeDeviceHelperto reject valid devices.geometry_helper.cppalready trims the trailing NUL — apply the same pattern here.🛡️ Proposed fix
- return std::string(buffer.data, buffer.size); + std::string result(buffer.data, buffer.size); + if (!result.empty() && result.back() == '\0') { + result.pop_back(); + } + return result;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/sdk/s3dk_wrapper.hpp` around lines 40 - 52, The device_type_string function constructs a std::string using buffer.size which includes a trailing NUL and can produce an embedded NUL that breaks convert_string_image_type/initializeDeviceHelper; after ftkGetData succeeds, trim the trailing NUL before constructing the std::string (same approach as geometry_helper.cpp) — e.g. detect if buffer.size>0 and buffer.data[buffer.size-1]=='\0' and reduce the length by one when creating the std::string from ftkBuffer returned by ftkGetData inside device_type_string.operators/atracsys_camera/sdk/sdk_wrapper.hpp (1)
60-70:⚠️ Potential issue | 🟡 Minor
initExtstill does an unbounded read ofbuffer->dataand contradicts its own return-type contract.Two unresolved concerns from a prior review:
reinterpret_cast<const char*>(buffer->data)assumes NUL-termination.ftkBuffertracks an explicitsizeand is not guaranteed NUL-terminated on the error path — usestd::string(data, size)with the size check, matching the bounded construction already used inatracsys_device.hppands3dk_wrapper.hpp.- The override is declared to return
ftkError, but on failure it throws and on success it always returnsFTK_OK. Any caller written against theISDKInterfacesignature cannot distinguish non-fatal SDK errors. Pick one contract (throw +void, or return the code without throwing) and align the interface accordingly.🛡️ Proposed fix (bounded read; return-path decision still needed)
ftkError initExt(const char* config, ftkBuffer* buffer) override { ftkLibrary lib = ftkInitExt(config, buffer); if (!lib) { - if (buffer && buffer->data) { - throw std::runtime_error(std::string("ftkInitExt failed: ") + - reinterpret_cast<const char*>(buffer->data)); + if (buffer && buffer->data && buffer->size > 0) { + throw std::runtime_error( + std::string("ftkInitExt failed: ") + + std::string(reinterpret_cast<const char*>(buffer->data), buffer->size)); } throw std::runtime_error("ftkInitExt failed (unknown error)"); } return ftkError::FTK_OK; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/sdk/sdk_wrapper.hpp` around lines 60 - 70, initExt currently performs an unbounded NUL-assuming read of buffer->data and mixes throwing with a ftkError return contract; change the error-string construction to use a bounded read (e.g., construct std::string from buffer->data and buffer->size, only after checking buffer and size) matching the pattern used in atracsys_device.hpp and s3dk_wrapper.hpp, and then choose and apply a single error contract: either (A) make initExt return ftkError and remove all throws (return the appropriate ftkError on failure and FTK_OK on success), or (B) make initExt (and the ISDKInterface declaration) void and throw std::runtime_error on failure; update the override signature in sdk_wrapper.hpp and any callers to match the chosen contract.applications/atracsys_visualizer/CMakeLists.txt (2)
47-49:⚠️ Potential issue | 🟠 Major
HOLOHUB_DATA_DIRis still required unconditionally.With
-DHOLOHUB_DOWNLOAD_DATASETS=OFF(live-camera-only build, or any environment that supplies data out-of-band), configure still fails with "HOLOHUB_DATA_DIR must be set when building with datasets" even though no dataset is actually consumed. Gate the check on the branch that actually uses the variable.🛠️ Proposed fix
-if(NOT DEFINED HOLOHUB_DATA_DIR OR HOLOHUB_DATA_DIR STREQUAL "") - message(FATAL_ERROR "HOLOHUB_DATA_DIR must be set when building with datasets") -endif() - -if(HOLOHUB_DOWNLOAD_DATASETS AND NOT EXISTS "${HOLOHUB_DATA_DIR}/atracsys_visualizer/ir_base.gxf_entities") +if(HOLOHUB_DOWNLOAD_DATASETS) + if(NOT DEFINED HOLOHUB_DATA_DIR OR HOLOHUB_DATA_DIR STREQUAL "") + message(FATAL_ERROR "HOLOHUB_DATA_DIR must be set when HOLOHUB_DOWNLOAD_DATASETS=ON") + endif() +endif() + +if(HOLOHUB_DOWNLOAD_DATASETS AND NOT EXISTS "${HOLOHUB_DATA_DIR}/atracsys_visualizer/ir_base.gxf_entities")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/atracsys_visualizer/CMakeLists.txt` around lines 47 - 49, The check for HOLOHUB_DATA_DIR is currently unconditional; make it conditional on the option that requires datasets. Guard the existing fatal check with the same flag that controls dataset usage (HOLOHUB_DOWNLOAD_DATASETS) or move the check inside the branch that downloads/uses datasets so the message(FATAL_ERROR "HOLOHUB_DATA_DIR must be set when building with datasets") runs only when HOLOHUB_DOWNLOAD_DATASETS is enabled; reference the HOLOHUB_DATA_DIR and HOLOHUB_DOWNLOAD_DATASETS symbols and update the CMakeLists logic accordingly.
32-45:⚠️ Potential issue | 🟡 MinorLeading empty list element when
HOLOHUB_BUILD_OPERATORSstarts empty.When
HOLOHUB_BUILD_OPERATORSis initialized to""at Line 33 and live-camera is enabled, the expansion on Line 38 yields";atracsys_camera"— a list with a leading empty element. CMake list operations and downstream consumers that iterate the list will see a bogus empty operator name. Build it vialist(APPEND)(which properly handles empty state) instead of string concatenation.♻️ Proposed fix
if(ATRACSYS_VISUALIZER_ENABLE_LIVE_CAMERA) if(NOT "atracsys_camera" IN_LIST HOLOHUB_BUILD_OPERATORS) - set(HOLOHUB_BUILD_OPERATORS "${HOLOHUB_BUILD_OPERATORS};atracsys_camera" CACHE STRING "Operators to build dynamically" FORCE) + list(APPEND HOLOHUB_BUILD_OPERATORS "atracsys_camera") + list(REMOVE_ITEM HOLOHUB_BUILD_OPERATORS "") + set(HOLOHUB_BUILD_OPERATORS "${HOLOHUB_BUILD_OPERATORS}" CACHE STRING "Operators to build dynamically" FORCE) endif() else() if("atracsys_camera" IN_LIST HOLOHUB_BUILD_OPERATORS) list(REMOVE_ITEM HOLOHUB_BUILD_OPERATORS "atracsys_camera") set(HOLOHUB_BUILD_OPERATORS "${HOLOHUB_BUILD_OPERATORS}" CACHE STRING "Operators to build dynamically" FORCE) endif() endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/atracsys_visualizer/CMakeLists.txt` around lines 32 - 45, The HOLOHUB_BUILD_OPERATORS list is being built via string concatenation when ATRACSYS_VISUALIZER_ENABLE_LIVE_CAMERA is true, causing a leading empty entry when HOLOHUB_BUILD_OPERATORS is "" (so ";atracsys_camera" is produced); replace the concatenation/set pattern that adds "atracsys_camera" with a proper CMake list append (use list(APPEND HOLOHUB_BUILD_OPERATORS "atracsys_camera")) to avoid the empty element, and after mutating the cached variable update the cached value with set(HOLOHUB_BUILD_OPERATORS ... CACHE STRING FORCE) as needed; keep the existing list(REMOVE_ITEM HOLOHUB_BUILD_OPERATORS "atracsys_camera") logic for the disable path but ensure the cache is updated afterward as well so downstream consumers see the correct list.applications/atracsys_visualizer/cpp/atracsys_visualizer.cpp (1)
182-185:⚠️ Potential issue | 🟡 MinorLoad the YAML config for
PointCloudFilterOp.The operator exposes filter parameters, but live-camera mode constructs it without
from_config("point_cloud_filter"), so YAML overrides are ignored.🛠️ Proposed fix
auto point_cloud_filter = make_operator<holoscan::ops::PointCloudFilterOp>( "point_cloud_filter", + from_config("point_cloud_filter"), Arg("structured_allocator") = structured_points_allocator, Arg("cuda_stream_pool") = cuda_stream_pool);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@applications/atracsys_visualizer/cpp/atracsys_visualizer.cpp` around lines 182 - 185, The PointCloudFilterOp is constructed without loading YAML config so runtime filter parameters are not applied; update the operator construction for point_cloud_filter to include the YAML config by calling from_config("point_cloud_filter") (e.g., in the make_operator<holoscan::ops::PointCloudFilterOp> call add the from_config("point_cloud_filter") argument) while preserving existing Args like "structured_allocator" and "cuda_stream_pool" so YAML overrides are respected at runtime.operators/atracsys_camera/master_source_op.cpp (1)
208-254:⚠️ Potential issue | 🟠 MajorClean up partially initialized resources when
start()fails.
dev.init()still runs before the guarded block, and the catch path only shuts down the device. If a later initialization step throws,stop()is not guaranteed to run, leaving frame/S3DK/thread state partially initialized.🛠️ Proposed fix
auto& dev = AtracsysDevice::instance(); - dev.init(); try { + dev.init(); device_sn_ = dev.serial(); @@ capture_thread_ = std::thread(&AtracsysMasterSourceOp::capture_loop, this); } catch (...) { + is_running_ = false; + if (capture_thread_.joinable()) { + capture_thread_.join(); + } + reset_state(); + destroy_frame(); + gpu_frame_ = nullptr; + engine_ = nullptr; + stereo_params_ = nullptr; + s3dk_.reset(); AtracsysDevice::instance().shutdown(); throw; }As per coding guidelines, “Code contributions must include appropriate error handling and validation for invalid inputs”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@operators/atracsys_camera/master_source_op.cpp` around lines 208 - 254, The start() sequence currently calls dev.init() before the try block and the catch only shuts down the device, which leaves partial state (s3dk_, frame_, engine_, gpu_frame_, stereo_params_, capture_thread_, is_running_) when an exception occurs; fix by either moving AtracsysDevice::instance().init() into the try or by expanding the catch to perform deterministic cleanup: set is_running_ = false, if capture_thread_.joinable() join it, release/destroy frame_ (using sdk_ destroy if available), reset s3dk_, engine_, gpu_frame_, stereo_params_ unique_ptrs/resources, and call AtracsysDevice::instance().shutdown(); ensure the cleanup references the exact symbols used (dev.init(), s3dk_->initializeDeviceHelper, frame_, gpu_frame_, stereo_params_, engine_, capture_thread_, is_running_, AtracsysDevice::instance().shutdown()) so no partially initialized resources remain after failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@applications/atracsys_visualizer/cpp/atracsys_visualizer.cpp`:
- Around line 78-80: parse_arguments() currently returns false for both --help
and invalid options causing main() to treat both as success; change
parse_arguments() to return distinct outcomes (e.g., an enum or int status such
as PARSE_OK, PARSE_HELP, PARSE_ERROR) instead of a single bool, have
print_usage() still called for HELP but ensure parse_arguments() yields
PARSE_HELP, and update main() to exit normally on PARSE_HELP but return a
non-zero exit code for PARSE_ERROR; apply the same distinction to the related
block referenced around the other instance (lines 251-252) and update any
callers to handle the new status values.
In `@operators/atracsys_camera/master_source_op.cpp`:
- Around line 337-350: The capture loop is marking the SDK frame reusable
(async_cond_->event_state(AsynchronousEventState::EVENT_WAITING)) before the
async CUDA memcpy operations launched in emit_visible_frame, emit_ir_frame,
emit_marker_poses, and emit_structured_points complete; change the code to
synchronize the copy stream before setting EVENT_WAITING (for example, wait on
the provided cuda_stream via cudaStreamSynchronize or record and wait on a
cudaEvent tied to the same stream) so the source buffers
(frame_->imageLeftPixels, marker_poses_, gpu_frame_->_d_disparityMap) are not
overwritten while DMA is in flight.
In `@operators/atracsys_camera/master_source_op.hpp`:
- Around line 30-34: Remove the redundant forward declaration "extern \"C\" {
typedef struct CUstream_st* cudaStream_t; }" (the cudaStream_t typedef) from
master_source_op.hpp and rely on the transitively provided cudaStream_t from
holoscan/holoscan.hpp; update/remove only that typedef block and verify no local
code relies on that bespoke declaration (functions or members referencing
cudaStream_t remain unchanged and will use the definition from included
headers).
---
Duplicate comments:
In `@applications/atracsys_visualizer/CMakeLists.txt`:
- Around line 47-49: The check for HOLOHUB_DATA_DIR is currently unconditional;
make it conditional on the option that requires datasets. Guard the existing
fatal check with the same flag that controls dataset usage
(HOLOHUB_DOWNLOAD_DATASETS) or move the check inside the branch that
downloads/uses datasets so the message(FATAL_ERROR "HOLOHUB_DATA_DIR must be set
when building with datasets") runs only when HOLOHUB_DOWNLOAD_DATASETS is
enabled; reference the HOLOHUB_DATA_DIR and HOLOHUB_DOWNLOAD_DATASETS symbols
and update the CMakeLists logic accordingly.
- Around line 32-45: The HOLOHUB_BUILD_OPERATORS list is being built via string
concatenation when ATRACSYS_VISUALIZER_ENABLE_LIVE_CAMERA is true, causing a
leading empty entry when HOLOHUB_BUILD_OPERATORS is "" (so ";atracsys_camera" is
produced); replace the concatenation/set pattern that adds "atracsys_camera"
with a proper CMake list append (use list(APPEND HOLOHUB_BUILD_OPERATORS
"atracsys_camera")) to avoid the empty element, and after mutating the cached
variable update the cached value with set(HOLOHUB_BUILD_OPERATORS ... CACHE
STRING FORCE) as needed; keep the existing list(REMOVE_ITEM
HOLOHUB_BUILD_OPERATORS "atracsys_camera") logic for the disable path but ensure
the cache is updated afterward as well so downstream consumers see the correct
list.
In `@applications/atracsys_visualizer/cpp/atracsys_visualizer.cpp`:
- Around line 182-185: The PointCloudFilterOp is constructed without loading
YAML config so runtime filter parameters are not applied; update the operator
construction for point_cloud_filter to include the YAML config by calling
from_config("point_cloud_filter") (e.g., in the
make_operator<holoscan::ops::PointCloudFilterOp> call add the
from_config("point_cloud_filter") argument) while preserving existing Args like
"structured_allocator" and "cuda_stream_pool" so YAML overrides are respected at
runtime.
In `@operators/atracsys_camera/master_source_op.cpp`:
- Around line 208-254: The start() sequence currently calls dev.init() before
the try block and the catch only shuts down the device, which leaves partial
state (s3dk_, frame_, engine_, gpu_frame_, stereo_params_, capture_thread_,
is_running_) when an exception occurs; fix by either moving
AtracsysDevice::instance().init() into the try or by expanding the catch to
perform deterministic cleanup: set is_running_ = false, if
capture_thread_.joinable() join it, release/destroy frame_ (using sdk_ destroy
if available), reset s3dk_, engine_, gpu_frame_, stereo_params_
unique_ptrs/resources, and call AtracsysDevice::instance().shutdown(); ensure
the cleanup references the exact symbols used (dev.init(),
s3dk_->initializeDeviceHelper, frame_, gpu_frame_, stereo_params_, engine_,
capture_thread_, is_running_, AtracsysDevice::instance().shutdown()) so no
partially initialized resources remain after failure.
In `@operators/atracsys_camera/sdk/s3dk_wrapper.hpp`:
- Around line 40-52: The device_type_string function constructs a std::string
using buffer.size which includes a trailing NUL and can produce an embedded NUL
that breaks convert_string_image_type/initializeDeviceHelper; after ftkGetData
succeeds, trim the trailing NUL before constructing the std::string (same
approach as geometry_helper.cpp) — e.g. detect if buffer.size>0 and
buffer.data[buffer.size-1]=='\0' and reduce the length by one when creating the
std::string from ftkBuffer returned by ftkGetData inside device_type_string.
In `@operators/atracsys_camera/sdk/sdk_wrapper.hpp`:
- Around line 60-70: initExt currently performs an unbounded NUL-assuming read
of buffer->data and mixes throwing with a ftkError return contract; change the
error-string construction to use a bounded read (e.g., construct std::string
from buffer->data and buffer->size, only after checking buffer and size)
matching the pattern used in atracsys_device.hpp and s3dk_wrapper.hpp, and then
choose and apply a single error contract: either (A) make initExt return
ftkError and remove all throws (return the appropriate ftkError on failure and
FTK_OK on success), or (B) make initExt (and the ISDKInterface declaration) void
and throw std::runtime_error on failure; update the override signature in
sdk_wrapper.hpp and any callers to match the chosen contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87b150fb-b620-4a8e-9d79-2151763ebbcb
📒 Files selected for processing (11)
applications/atracsys_visualizer/CMakeLists.txtapplications/atracsys_visualizer/cpp/atracsys_visualizer.cppoperators/atracsys_camera/master_source_op.cppoperators/atracsys_camera/master_source_op.hppoperators/atracsys_camera/point_cloud_filter_op.cppoperators/atracsys_camera/point_cloud_filter_op.hppoperators/atracsys_camera/sdk/atracsys_device.hppoperators/atracsys_camera/sdk/geometry_helper.cppoperators/atracsys_camera/sdk/helpers.cppoperators/atracsys_camera/sdk/s3dk_wrapper.hppoperators/atracsys_camera/sdk/sdk_wrapper.hpp
artrittelaku-wayland
force-pushed
the
feature/atracsys-visualizer-holohub
branch
from
e36dfa2 to
0e17ca7
Compare
Contributor
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
artrittelaku-wayland
force-pushed
the
feature/atracsys-visualizer-holohub
branch
from
0e17ca7 to
0825008
Compare
Member
|
Hi @artrittelaku-wayland, thanks for your contribution. Could you please provide me write access to this PR's branch of your fork, so we can help updating and manage the PR merge? Thanks |
artrittelaku-wayland
force-pushed
the
feature/atracsys-visualizer-holohub
branch
from
0825008 to
afb40c5
Compare
Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
artrittelaku-wayland
force-pushed
the
feature/atracsys-visualizer-holohub
branch
from
afb40c5 to
10c37fe
Compare
bhashemian
reviewed
Apr 22, 2026
Member
bhashemian
left a comment
bhashemian
left a comment
There was a problem hiding this comment.
Thanks @artrittelaku-wayland for this interesting contribution. I reviewed and added a few comments inline.
| @@ -0,0 +1,50 @@ | |||
| { | |||
| "operator": { | |||
| "name": "AtracsysCameraOp", | |||
Member
There was a problem hiding this comment.
Is this the correct name for this operator? The actual operators that this metadata represets seems to be AtracsysMasterSourceOp and PointCloudFilterOp, right?
I suggest the following options:
- Either change the name to "Atracsys Camera Operators" (note the "s" at the end)
- OR separate each operator in its own subdirectory.
Thanks
Author
There was a problem hiding this comment.
I updated the metadata name to Atracsys Camera Operators since this package contains both AtracsysMasterSourceOp and PointCloudFilterOp.
Comment on lines
+48
to
+82
| if(HOLOHUB_DOWNLOAD_DATASETS) | ||
| if(NOT DEFINED HOLOHUB_DATA_DIR OR HOLOHUB_DATA_DIR STREQUAL "") | ||
| message(FATAL_ERROR "HOLOHUB_DATA_DIR must be set when building with datasets") | ||
| endif() | ||
|
|
||
| if(NOT EXISTS "${HOLOHUB_DATA_DIR}/atracsys_visualizer/ir_base.gxf_entities") | ||
| set(ATRACSYS_VISUALIZER_DATASET_URL | ||
| "https://github.com/waylandio/holohub/releases/download/dataset-v1.0/atracsys_visualizer.zip") | ||
| set(ATRACSYS_VISUALIZER_DATASET_SHA256 "674551dee1c40bf4f2b62c0ea23be01b6126d0cf549971d87b2fd86b0d4895e9") | ||
| message(STATUS "Downloading Atracsys Visualizer demo dataset...") | ||
| file(DOWNLOAD "${ATRACSYS_VISUALIZER_DATASET_URL}" | ||
| "${CMAKE_CURRENT_BINARY_DIR}/atracsys_visualizer.zip" | ||
| EXPECTED_HASH SHA256=${ATRACSYS_VISUALIZER_DATASET_SHA256} | ||
| TLS_VERIFY ON | ||
| SHOW_PROGRESS | ||
| STATUS DOWNLOAD_STATUS) | ||
|
|
||
| list(GET DOWNLOAD_STATUS 0 STATUS_CODE) | ||
| if(NOT STATUS_CODE EQUAL 0) | ||
| message(FATAL_ERROR "Failed to download dataset. Status: ${DOWNLOAD_STATUS}") | ||
| endif() | ||
|
|
||
| message(STATUS "Extracting Atracsys Visualizer demo dataset...") | ||
| file(MAKE_DIRECTORY "${HOLOHUB_DATA_DIR}") | ||
| execute_process( | ||
| COMMAND ${CMAKE_COMMAND} -E tar xf "${CMAKE_CURRENT_BINARY_DIR}/atracsys_visualizer.zip" | ||
| WORKING_DIRECTORY "${HOLOHUB_DATA_DIR}" | ||
| RESULT_VARIABLE EXTRACT_STATUS | ||
| ) | ||
| if(NOT EXTRACT_STATUS EQUAL 0) | ||
| message(FATAL_ERROR "Failed to extract dataset.") | ||
| endif() | ||
| file(REMOVE "${CMAKE_CURRENT_BINARY_DIR}/atracsys_visualizer.zip") | ||
| endif() | ||
| endif() |
Member
There was a problem hiding this comment.
Can this download/extraction logic be replaced by holoscan_download_data
Like these examples:
holohub/applications/imaging_ai_segmentator/CMakeLists.txt
Lines 53 to 58 in 09cfd82
holohub/workflows/ai_surgical_video/CMakeLists.txt
Lines 38 to 69 in 09cfd82
Author
There was a problem hiding this comment.
I replaced the custom download/extraction logic with holoscan_download_data and verified the replay dataset downloads correctly and ends up under the expected data/atracsys_visualizer layout.
Comment on lines
+93
to
+117
| std::shared_ptr<holoscan::ops::CameraCalibration> make_camera_calibration( | ||
| const std::string& config_path) { | ||
| YAML::Node config = YAML::LoadFile(config_path); | ||
| auto calibration = std::make_shared<holoscan::ops::CameraCalibration>(); | ||
| calibration->fx = static_cast<float>(config["camera_calibration_fx"].as<double>()); | ||
| calibration->fy = static_cast<float>(config["camera_calibration_fy"].as<double>()); | ||
| calibration->cx = static_cast<float>(config["camera_calibration_cx"].as<double>()); | ||
| calibration->cy = static_cast<float>(config["camera_calibration_cy"].as<double>()); | ||
| calibration->skew = static_cast<float>(config["camera_calibration_skew"].as<double>()); | ||
| calibration->image_width = config["camera_calibration_image_width"].as<int>(); | ||
| calibration->image_height = config["camera_calibration_image_height"].as<int>(); | ||
|
|
||
| auto distortion = config["camera_calibration_distortion"].as<std::vector<double>>(); | ||
| if (distortion.size() != calibration->distortion.size()) { | ||
| throw std::runtime_error("camera_calibration_distortion must contain exactly 5 coefficients"); | ||
| } | ||
| for (size_t i = 0; i < distortion.size(); ++i) { | ||
| calibration->distortion[i] = static_cast<float>(distortion[i]); | ||
| } | ||
|
|
||
| if (!calibration->valid()) { | ||
| throw std::runtime_error("Atracsys visualizer calibration is invalid"); | ||
| } | ||
| return calibration; | ||
| } |
Member
There was a problem hiding this comment.
Could we use Holoscan's from_config like every other parameter, removing the explicit yaml-cpp::yaml-cpp link here? Thanks
Author
There was a problem hiding this comment.
I switched the calibration loading over to Holoscan config access instead of YAML::LoadFile and removed the explicit yaml-cpp link. I also verified the replay app still builds and runs.
|
|
||
| ## Visual Showcase | ||
|
|
||
|  |
Member
There was a problem hiding this comment.
Can we move this upper, like right before ## Architecture?
Author
There was a problem hiding this comment.
I moved the visual showcase section up so it appears right before ## Architecture.
Member
There was a problem hiding this comment.
Is it possible to reduce the size of tissue gif? Thanks
Author
There was a problem hiding this comment.
I replaced the demo GIF with a smaller version. The new file is half size of the previous one.
Member
|
@artrittelaku-wayland, considering that building OpenCV 4.10 from source incurs significant container build time, it would be nice to explicitly mention this as a known cost in the README, thereby setting the user’s expectations. Thanks |
Signed-off-by: artrittelaku-wayland <artrit.telaku@wayland.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR introduces the
atracsys_visualizer, a HoloHub application designed to showcase the imaging and submillimetric tracking capabilities of the Atracsys spryTrack 300 RGB camera.To ensure continuous CI reliability without hardware dependencies, the application defaults to a
replayermode using a high-fidelity dataset hosted on the Wayland Technologies GitHub Releases page.Features Highlighted:
geometry10.inias a custom reflective fiducial reference.Notes for Reviewers:
replayer): The dataset (.zip) is downloaded dynamically in CMake (file(DOWNLOAD...)) natively for CI tests and quick-starts.live_camera): Fully configured operator, but requires proprietary SDKs (spryTrack SDK v4.9.0 and S3DK) provided bysupport@atracsys.com.--cuda 12specified in the Docker build due to underlying OpenCV4.10.0behavior with CUDA 13 headers.Checklist
./holohub lintlocallyreplayermodeSPDX-FileCopyrightText: Copyright (c) 2026 Wayland Technologies.) includedmetadata.jsonconforms to HoloHub schemasSummary by CodeRabbit
New Features
Documentation