Add trivial FilterDecimate for fast downsampling without spatial awareness#25
Add trivial FilterDecimate for fast downsampling without spatial awareness#25
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a new FilterDecimate downsampling filter: implementation, header, build integration, RTTI registration, unit tests, and documentation entries; behavior implements fixed or target-size decimation of point-cloud layers. Changes
Sequence Diagram(s)sequenceDiagram
participant YAML as YAML/config
participant Filter as FilterDecimate
participant Map as metric_map_t
participant Layer as PointLayer (input/output)
YAML->>Filter: load parameters (input_layer, output_layer, decimation, target_max_size)
Filter->>Map: access input_layer
Map->>Layer: return input layer points
Filter->>Filter: compute decimation N (explicit or from target_max_size)
alt N <= 1 and different layers
Filter->>Map: shallow copy input_layer -> output_layer
else N > 1
Filter->>Map: GetOrCreatePointLayer(output_layer)
Filter->>Layer: reserve space / prepareForInsert
Filter->>Layer: insert every N-th point into output_layer
end
Filter->>Map: return modified metric_map_t
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
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: 1
🤖 Fix all issues with AI agents
In @docs/source/mp2p_icp_filters.rst:
- Around line 281-333: The docs reference a missing image '.. image::
decimate_example.png' in the FilterDecimate section; either add the referenced
screenshot file to the repo (named exactly decimate_example.png) or remove the
image directive and the surrounding rubric/Before → After block (the '.. image::
decimate_example.png' line and the preceding '.. rubric:: Before → After
Screenshot' and related lines) from the docs so the Sphinx build no longer
references a non-existent asset.
🧹 Nitpick comments (3)
mp2p_icp_filters/src/FilterDecimate.cpp (2)
33-39: Consider adding validation for mutually exclusive parameters.The filter accepts both
decimationandtarget_max_size, but neither is required. If a user provides neither (or both as 0), the filter effectively becomes a no-op or shallow copy. While this may be intentional, a warning or debug log when no decimation is performed could help users catch configuration mistakes.
67-77: Shallow copy may cause unexpected aliasing.When
N <= 1and layers differ, the output layer becomes a shared reference to the input layer. If downstream filters modify the output layer, they'll also modify the input layer, which may be unexpected.Consider using a deep copy or documenting this behavior more prominently in the header/docs. The current comment is only visible in the implementation.
tests/test-mp2p_FilterDecimate.cpp (1)
30-104: Consider adding edge case tests for completeness.The current test suite covers the main functionality well. For more robust coverage, consider adding tests for:
- Empty input layer (early return path)
- Same input/output layer with N>1 (in-place semantics)
- Both
decimation=0andtarget_max_size=0(undefined behavior?)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/source/mp2p_icp_filters.rstmp2p_icp_filters/CMakeLists.txtmp2p_icp_filters/include/mp2p_icp_filters/FilterDecimate.hmp2p_icp_filters/src/FilterDecimate.cppmp2p_icp_filters/src/register.cpptests/CMakeLists.txttests/test-mp2p_FilterDecimate.cpp
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test-mp2p_FilterDecimate.cpp (2)
mp2p_icp_filters/include/mp2p_icp_filters/FilterDecimate.h (1)
mp2p_icp_filters(26-78)mp2p_icp_filters/src/FilterDecimate.cpp (2)
filter(46-102)filter(46-46)
mp2p_icp_filters/include/mp2p_icp_filters/FilterDecimate.h (1)
mp2p_icp_filters/src/FilterDecimate.cpp (5)
FilterDecimate(31-31)filter(46-102)filter(46-46)initialize_filter(41-44)initialize_filter(41-41)
mp2p_icp_filters/src/register.cpp (2)
mp2p_icp_filters/include/mp2p_icp_filters/FilterDecimate.h (1)
FilterDecimate(43-76)mp2p_icp_filters/src/FilterDecimate.cpp (1)
FilterDecimate(31-31)
🔇 Additional comments (14)
mp2p_icp_filters/src/FilterDecimate.cpp (2)
56-65: LGTM!The decimation factor calculation correctly prioritizes
decimationovertarget_max_sizeand safely guards against division by zero.
85-101: API calls are correct and consistent with MRPT version design.The version-specific signatures for
insertPointFromfollow MRPT's intended API evolution:
- MRPT >= 0x020f03: Context object encapsulates the source point cloud reference, so only point index and context are passed
- MRPT >= 0x020f00: Context added as parameter alongside explicit source reference
- Earlier versions: Source point cloud explicitly required
This pattern is used consistently throughout the codebase (FilterDecimateVoxels, FilterSOR, FilterMLS, etc.), confirming the implementation is correct. No changes needed.
tests/CMakeLists.txt (1)
65-65: LGTM!The test entry follows the established pattern and is correctly positioned alphabetically among the filter tests.
mp2p_icp_filters/src/register.cpp (2)
34-34: LGTM!The include directive is correctly placed in alphabetical order.
73-73: LGTM!The RTTI class registration follows the established pattern and alphabetical ordering.
mp2p_icp_filters/CMakeLists.txt (2)
15-15: LGTM!The source file is correctly added to
LIB_SRCSin alphabetical order.
51-51: LGTM!The public header is correctly added to
LIB_PUBLIC_HDRSin alphabetical order.tests/test-mp2p_FilterDecimate.cpp (4)
1-28: LGTM!License header, includes, and namespace setup look correct for the test file.
44-59: LGTM!Test case A correctly validates fixed decimation factor: 100 points with decimation=4 yields 25 points (indices 0, 4, 8, ..., 96).
61-77: LGTM!Test case B correctly validates target_max_size: N=100/10=10, yielding 10 points.
79-94: LGTM!Test case C correctly validates no-decimation scenario when target_max_size exceeds input size.
mp2p_icp_filters/include/mp2p_icp_filters/FilterDecimate.h (3)
28-42: LGTM!The class documentation is thorough—clearly explaining the decimation behavior, accumulation semantics, and the non-deterministic spatial distribution caveat.
43-76: LGTM!The class structure follows the established FilterBase pattern correctly, with proper MRPT object registration, public filter interface, and nested Parameters struct.
59-60: No action required:output_layeris already validated as a required parameter.The
output_layerfield is enforced as required viaMCP_LOAD_REQ(c, output_layer)inload_from_yaml()(line 36), so missing it will cause a YAML load error. Additionally,GetOrCreatePointLayer()already throws an exception if an empty layer name is passed withallowEmptyName=false(line 81). The struct default to an empty string is never exposed in normal usage since the parameter must be provided in the YAML config. The shortcut path logic correctly handles both in-place (same layer) and copy (different layer) scenarios.Likely an incorrect or invalid review comment.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.