FIX: Avoid potential division by zero in FilterDecimateAdaptive#48
FIX: Avoid potential division by zero in FilterDecimateAdaptive#48
Conversation
📝 WalkthroughWalkthroughAdded runtime parameter validation and an early exit to avoid division by zero; removed MRPT-version conditional compilation for Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mp2p_icp_filters/src/FilterDecimateAdaptive.cpp (1)
191-195: Refine the warning text to match actual condition.At Line 193, the message says “No occupied voxels”, but
nVoxelshere means “no voxels passedminimum_input_points_per_voxel”. This can mislead debugging.Suggested log-message tweak
- MRPT_LOG_WARN("FilterDecimateAdaptive: No occupied voxels. Output will be empty."); + MRPT_LOG_WARN( + "FilterDecimateAdaptive: No eligible voxels after minimum_input_points_per_voxel " + "filtering. No points will be inserted.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mp2p_icp_filters/src/FilterDecimateAdaptive.cpp` around lines 191 - 195, The warning message logged in FilterDecimateAdaptive when nVoxels == 0 is misleading—nVoxels here means "no voxels passed minimum_input_points_per_voxel" not simply "no occupied voxels"; update the MRPT_LOG_WARN call to clearly state that no voxels met minimum_input_points_per_voxel (include the variable name or value if desirable) so the message reflects the actual condition checked (refer to nVoxels and minimum_input_points_per_voxel in FilterDecimateAdaptive).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mp2p_icp_filters/src/FilterDecimateAdaptive.cpp`:
- Around line 182-183: Wrap the MRPT-2.15+ API calls with preprocessor guards:
surround registerPointFieldsFrom and prepareForInsertPointsFrom (used to obtain
mrpt::maps::CPointsMap::InsertCtx) with `#if` MRPT_VERSION >= 0x020f00 / `#endif`;
for insertPointFrom use the newer no-source signature guarded with `#if`
MRPT_VERSION >= 0x020f03, and provide an `#elif` MRPT_VERSION >= 0x020f00 fallback
to the three-argument insertPointFrom version; ensure you reference the same
symbols (registerPointFieldsFrom, prepareForInsertPointsFrom,
mrpt::maps::CPointsMap::InsertCtx, insertPointFrom) so the code compiles on MRPT
2.11.5–2.15.x.
---
Nitpick comments:
In `@mp2p_icp_filters/src/FilterDecimateAdaptive.cpp`:
- Around line 191-195: The warning message logged in FilterDecimateAdaptive when
nVoxels == 0 is misleading—nVoxels here means "no voxels passed
minimum_input_points_per_voxel" not simply "no occupied voxels"; update the
MRPT_LOG_WARN call to clearly state that no voxels met
minimum_input_points_per_voxel (include the variable name or value if desirable)
so the message reflects the actual condition checked (refer to nVoxels and
minimum_input_points_per_voxel in FilterDecimateAdaptive).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 392f3478-e5a9-4634-9937-bee1eed472b2
📒 Files selected for processing (1)
mp2p_icp_filters/src/FilterDecimateAdaptive.cpp
| outPc->registerPointFieldsFrom(pc); | ||
| mrpt::maps::CPointsMap::InsertCtx ctx = outPc->prepareForInsertPointsFrom(pc); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== MRPT version constraints in CMake files =="
git ls-files | grep -i cmake | head -20
echo
echo "== Finding CMakeLists.txt files =="
fd '^CMakeLists\.txt$' | head -10Repository: MOLAorg/mp2p_icp
Length of output: 957
🏁 Script executed:
#!/bin/bash
# Check main CMakeLists.txt for MRPT version requirements
cat CMakeLists.txt | grep -i "mrpt\|find_package" | head -30Repository: MOLAorg/mp2p_icp
Length of output: 468
🏁 Script executed:
#!/bin/bash
# Read FilterDecimateAdaptive.cpp around lines 182-183 and 235
wc -l mp2p_icp_filters/src/FilterDecimateAdaptive.cpp
echo "=== Lines 180-190 ==="
sed -n '180,190p' mp2p_icp_filters/src/FilterDecimateAdaptive.cpp
echo
echo "=== Lines 233-237 ==="
sed -n '233,237p' mp2p_icp_filters/src/FilterDecimateAdaptive.cppRepository: MOLAorg/mp2p_icp
Length of output: 582
🏁 Script executed:
#!/bin/bash
# Search for MRPT_VERSION guards and insertPointFrom usage in the codebase
echo "=== Existing MRPT_VERSION guards in source ==="
rg -n 'MRPT_VERSION\s*[<>=]' mp2p_icp_filters/src --type cpp
echo
echo "=== All insertPointFrom calls ==="
rg -n 'insertPointFrom' mp2p_icp_filters/src --type cpp -B2 -A2
echo
echo "=== All prepareForInsertPointsFrom calls ==="
rg -n 'prepareForInsertPointsFrom' mp2p_icp_filters/src --type cpp -B2 -A2
echo
echo "=== All registerPointFieldsFrom calls ==="
rg -n 'registerPointFieldsFrom' mp2p_icp_filters/src --type cpp -B2 -A2Repository: MOLAorg/mp2p_icp
Length of output: 50372
🌐 Web query:
MRPT library mrpt::maps::CPointsMap insertPointFrom API signature MRPT 2.11.5 2.15.0 2.15.3 overload prepareForInsertPointsFrom registerPointFieldsFrom
💡 Result:
MRPT mrpt::maps::CPointsMap point-copy API signatures
MRPT 2.11.5
void insertPointFrom(const mrpt::maps::CPointsMap& source, size_t sourcePointIndex);Copies XYZ plus the built-in optional fields (intensity, ring, timestamp, color channels) if they exist in both maps. (raw.githubusercontent.com)
MRPT 2.15.0
New “generic fields” workflow: first register fields, then build an insertion context, then insert points.
bool registerPointFieldsFrom(const mrpt::maps::CPointsMap& source);
struct InsertCtx { /* ... */ };
InsertCtx prepareForInsertPointsFrom(const CPointsMap& source) const;
void insertPointFrom(
const CPointsMap& source, size_t sourcePointIndex, const InsertCtx& ctx);MRPT 2.15.3
insertPointFrom() drops the redundant source argument (the source buffers are already inside InsertCtx). Also registerPointFieldsFrom() expands to cover more field types.
bool registerPointFieldsFrom(const mrpt::maps::CPointsMap& source); // now includes double, uint8, etc.
struct InsertCtx { /* ... */ };
[[nodiscard]] InsertCtx prepareForInsertPointsFrom(const CPointsMap& source) const;
void insertPointFrom(size_t sourcePointIndex, const InsertCtx& ctx);Citations:
- 1: https://raw.githubusercontent.com/MRPT/mrpt/2.11.5/libs/maps/include/mrpt/maps/CPointsMap.h
- 2: https://raw.githubusercontent.com/MRPT/mrpt/2.15.0/libs/maps/include/mrpt/maps/CPointsMap.h
- 3: https://raw.githubusercontent.com/MRPT/mrpt/2.15.3/libs/maps/include/mrpt/maps/CPointsMap.h
Add MRPT-version guards for InsertCtx / insertPointFrom / registerPointFieldsFrom calls.
Lines 182–183 and 235 use APIs introduced in MRPT 2.15.0 without version guards. The project requires MRPT ≥ 2.11.5 (see CMakeLists.txt), so these calls will cause build failures on 2.11.5–2.15.2.
The codebase pattern is already established in FilterDecimate.cpp and nearly every other filter. Add:
#if MRPT_VERSION >= 0x020f00around registerPointFieldsFrom and prepareForInsertPointsFrom (lines 182–183)#if MRPT_VERSION >= 0x020f03for the insertPointFrom signature without source (line 235), with#elif MRPT_VERSION >= 0x020f00fallback for the 3-argument form
Compatibility-safe patch pattern
- outPc->registerPointFieldsFrom(pc);
- mrpt::maps::CPointsMap::InsertCtx ctx = outPc->prepareForInsertPointsFrom(pc);
+#if MRPT_VERSION >= 0x020f00 // 2.15.0
+ outPc->registerPointFieldsFrom(pc);
+ mrpt::maps::CPointsMap::InsertCtx ctx = outPc->prepareForInsertPointsFrom(pc);
+#endif
@@
- outPc->insertPointFrom(ptIdx, ctx);
+#if MRPT_VERSION >= 0x020f03 // 2.15.3
+ outPc->insertPointFrom(ptIdx, ctx);
+#elif MRPT_VERSION >= 0x020f00 // 2.15.0
+ outPc->insertPointFrom(pc, ptIdx, ctx);
+#else
+ outPc->insertPointFrom(pc, ptIdx);
+#endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mp2p_icp_filters/src/FilterDecimateAdaptive.cpp` around lines 182 - 183, Wrap
the MRPT-2.15+ API calls with preprocessor guards: surround
registerPointFieldsFrom and prepareForInsertPointsFrom (used to obtain
mrpt::maps::CPointsMap::InsertCtx) with `#if` MRPT_VERSION >= 0x020f00 / `#endif`;
for insertPointFrom use the newer no-source signature guarded with `#if`
MRPT_VERSION >= 0x020f03, and provide an `#elif` MRPT_VERSION >= 0x020f00 fallback
to the three-argument insertPointFrom version; ensure you reference the same
symbols (registerPointFieldsFrom, prepareForInsertPointsFrom,
mrpt::maps::CPointsMap::InsertCtx, insertPointFrom) so the code compiles on MRPT
2.11.5–2.15.x.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CMakeLists.txt (1)
58-64:⚠️ Potential issue | 🟠 MajorBump declared MRPT minimum to 2.15.4 to match actual code requirements.
CMakeLists.txt line 58 declares MRPT 2.15.0, but multiple filters require MRPT 2.15.4: FilterSOR.cpp explicitly throws at runtime if MRPT_VERSION < 0x020f04, and FilterByRing.cpp, FilterByExpression.cpp, and FilterAbsoluteTimestamp.cpp all guard on >= 0x020f04 (2.15.4). This allows a build to succeed but fail at runtime on the older version.
Proposed fix
-find_package(MRPT 2.15.0 REQUIRED COMPONENTS +find_package(MRPT 2.15.4 REQUIRED COMPONENTS containers tfest maps gui topography )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` around lines 58 - 64, Update the MRPT minimum version declared in CMakeLists.txt to 2.15.4 so the configured build requirement matches runtime checks; specifically change the version in the find_package(MRPT ...) call to 2.15.4. This aligns CMake with the version guards and runtime throw in FilterSOR.cpp and the preprocessor/version checks in FilterByRing.cpp, FilterByExpression.cpp, and FilterAbsoluteTimestamp.cpp that require MRPT >= 2.15.4.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@CMakeLists.txt`:
- Around line 58-64: Update the MRPT minimum version declared in CMakeLists.txt
to 2.15.4 so the configured build requirement matches runtime checks;
specifically change the version in the find_package(MRPT ...) call to 2.15.4.
This aligns CMake with the version guards and runtime throw in FilterSOR.cpp and
the preprocessor/version checks in FilterByRing.cpp, FilterByExpression.cpp, and
FilterAbsoluteTimestamp.cpp that require MRPT >= 2.15.4.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #48 +/- ##
===========================================
- Coverage 78.60% 78.59% -0.01%
===========================================
Files 190 190
Lines 10564 10569 +5
Branches 979 980 +1
===========================================
+ Hits 8304 8307 +3
- Misses 2260 2262 +2
🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor