Conversation
📝 WalkthroughWalkthroughAdds a new voxel-based Statistical Outlier Removal filter (FilterVoxelSOR) with implementation, header, docs, tests, CMake registration, MRPT-version guards, minor FilterSOR logging/statistics, and improved dynamic-creation error messaging. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Filter as "FilterVoxelSOR"
participant VoxelGrid as "Voxel Grid Builder"
participant Parallel as "Parallel Worker (TBB / seq)"
participant KNN as "Per-voxel k-NN Analyzer"
participant Layers as "Inliers/Outliers Layers"
User->>Filter: filter(metric_map_t)
Filter->>Filter: validate input layer & params
Filter->>VoxelGrid: build voxels (voxel_size)
VoxelGrid-->>Filter: voxel buckets
Filter->>Layers: ensure/create output layers
Filter->>Parallel: dispatch voxel buckets
loop per voxel (parallel/thread)
Parallel->>KNN: if size ≤ mean_k -> mark all inliers
alt size > mean_k
KNN->>KNN: build local points map
KNN->>KNN: compute avg dist to k-NN per point
KNN->>KNN: compute μ, σ and threshold = μ + std_dev_mul·σ
KNN-->>Parallel: classify points (inlier/outlier)
end
end
Parallel-->>Filter: per-thread results
Filter->>Layers: merge results into inliers/outliers
Layers-->>Filter: layers updated
Filter->>User: return updated metric_map_t (log stats)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mp2p_icp_filters/src/FilterSOR.cpp (1)
162-215: Counter increments missing in older MRPT version branches.The
num_inliersandnum_outlierscounters are only incremented within the#if MRPT_VERSION >= 0x020f03branch (lines 171 and 180). The other version branches (lines 185-196 for MRPT 2.15.0-2.15.2, and lines 197-209 for older versions) don't increment the counters, so the debug log will always report 0 inliers and 0 outliers for those MRPT versions.🐛 Proposed fix to add counters in all branches
`#elif` MRPT_VERSION >= 0x020f00 // 2.15.0 if (outInliers) { outInliers->insertPointFrom(pc, i, *ctxI); } + ++num_inliers; } else { + ++num_outliers; if (outOutliers) { outOutliers->insertPointFrom(pc, i, *ctxO); } `#else` if (outInliers) { outInliers->insertPointFrom(pc, i); } + ++num_inliers; } else { + ++num_outliers; if (outOutliers) { outOutliers->insertPointFrom(pc, i); } `#endif`
🤖 Fix all issues with AI agents
In `@mp2p_icp_filters/include/mp2p_icp_filters/FilterVoxelSOR.h`:
- Around line 15-20: Update the file-level documentation date in the header
comment of FilterVoxelSOR.h: change the `@date` value currently "Jan 21, 2025" to
"Jan 21, 2026" so the file header (the block at top of FilterVoxelSOR.h)
reflects the correct year.
In `@mp2p_icp_filters/src/FilterVoxelSOR.cpp`:
- Around line 54-56: After loading params in FilterVoxelSOR::initialize_filter,
add MRPT_START / MRPT_END wrapped assertions to validate parameters: assert
params.voxel_size > 0 with ASSERT_GT_(params.voxel_size, 0.0f), assert
params.mean_k >= 1 with ASSERT_GE_(params.mean_k, 1), and assert
params.parallelization_grain_size >= 1 with
ASSERT_GE_(params.parallelization_grain_size, 1) to prevent divide-by-zero and
invalid ranges as done in FilterMLS.
- Around line 61-85: Add a guard before creating output layers to reject
configurations where params.output_layer_inliers or params.output_layer_outliers
equals params.input_layer or equals each other: detect these name collisions and
return early (or raise) with an error; perform this check prior to calling
GetOrCreatePointLayer so you never obtain aliases to the input map (which would
lead to xs/ys/zs buffer invalidation and duplicate insertions). Ensure the
validation references params.input_layer, params.output_layer_inliers,
params.output_layer_outliers and runs before the GetOrCreatePointLayer calls in
FilterVoxelSOR.
In `@tests/test-mp2p_class_factory.cpp`:
- Around line 15-20: Update the file-level doc comment's `@brief`: replace the
existing "@brief Unit tests for Generator" with a concise description reflecting
this test suite's purpose (e.g., "@brief Unit tests for the class factory
mechanism and registered classes"); locate and edit the top comment block
containing the `@brief` tag in test-mp2p_class_factory.cpp to ensure the
description matches the tests.
- Around line 91-103: The final unconditional "return 0;" in main is unreachable
because both the try block (return test_class_factory()) and the catch block
(return 1) already return; remove the trailing "return 0;" (or alternatively
refactor main to assign test_class_factory() to an int and return it after the
try/catch) so that main no longer contains unreachable code; locate this in the
main function where test_class_factory() is returned.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@mp2p_icp_filters/src/FilterSOR.cpp`:
- Around line 58-60: Add a direct include for the C++ standard exception header
so the use of std::runtime_error in FilterSOR.cpp is not dependent on transitive
MRPT headers: add an `#include` <stdexcept> near the top of FilterSOR.cpp (before
the preprocessor check/throw) so the throw std::runtime_error("FilterSOR
requires MRPT >= 2.15.4") is valid and self-contained.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #27 +/- ##
===========================================
+ Coverage 65.25% 66.19% +0.94%
===========================================
Files 161 165 +4
Lines 7938 8100 +162
Branches 858 879 +21
===========================================
+ Hits 5180 5362 +182
+ Misses 2758 2738 -20
🚀 New features to boost your workflow:
|
This was referenced Apr 9, 2026
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
1 participant
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.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.