Generator: creates CGenericPointsMap by default; add sanity checks in most filters#53
Merged
Generator: creates CGenericPointsMap by default; add sanity checks in most filters#53
Conversation
|
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:
📝 WalkthroughWalkthroughRemoved MRPT-version conditional branches and standardized point-field access/insertion across filters; added pointcloud field utilities and sanity checks; introduced Generator parameters Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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📝 Generate docstrings
🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mp2p_icp_map/src/pointcloud_sanity_check.cpp (1)
46-48:⚠️ Potential issue | 🟠 MajorEither keep the compatibility branch or raise the minimum MRPT version to 2.15.4.
The API
getPointsBufferRef_uint16_field()was introduced in MRPT 2.15.4. Your repository declares minimum MRPT 2.15.0 in CMakeLists.txt, which means lines 46–48 will fail to compile on MRPT 2.15.0–2.15.3. SinceFilterByRing.cppstill maintains the compatibility branch, remove the hard-coded call and restore the version check, or explicitly raise the minimum required MRPT version to 2.15.4.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mp2p_icp_map/src/pointcloud_sanity_check.cpp` around lines 46 - 48, The loop currently calls getPointsBufferRef_uint16_field() which only exists in MRPT >= 2.15.4 while the project still declares MRPT 2.15.0; either restore the compatibility branch used in FilterByRing.cpp around getPointsBufferRef_uint16_field() so you call the older API when MRPT < 2.15.4 (guard by the same MRPT_VERSION check used in FilterByRing.cpp and iterate getPointFieldNames_uint16()/the older buffer accessor), or update the minimum MRPT version in CMakeLists.txt to 2.15.4 so getPointsBufferRef_uint16_field() can be used unconditionally (adjust any related target_compile_definitions/version checks accordingly).
🤖 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/include/mp2p_icp_filters/FilterDeskew.h`:
- Around line 64-65: The header comment in FilterDeskew.h incorrectly mandates
mrpt::maps::CGenericPointsMap; change the doc to state the input layer must
provide a mrpt::maps::CPointsMap (or any points-map type) that exposes a float
timestamp field named "t" instead of naming a concrete class; update the
relevant sentence(s) near the class/struct comment for FilterDeskew so it
matches the actual requirement enforced in FilterDeskew.cpp and mentions the
required 't' field and its float type.
In `@mp2p_icp_filters/src/Generator.cpp`:
- Around line 354-359: The current assertion checks obj instead of the dynamic
cast result, so if the factory returns an object not derived from
mrpt::maps::CPointsMap the code silently leaves outPc null; change the assertion
to validate outPc (the result of std::dynamic_pointer_cast) and emit the failure
message using params.default_pointcloud_class so it clearly reports that the
created object is not derived from CPointsMap (i.e., replace the ASSERTMSG_
check on obj with one on outPc and keep the same formatted message referencing
params.default_pointcloud_class and CPointsMap).
---
Outside diff comments:
In `@mp2p_icp_map/src/pointcloud_sanity_check.cpp`:
- Around line 46-48: The loop currently calls getPointsBufferRef_uint16_field()
which only exists in MRPT >= 2.15.4 while the project still declares MRPT
2.15.0; either restore the compatibility branch used in FilterByRing.cpp around
getPointsBufferRef_uint16_field() so you call the older API when MRPT < 2.15.4
(guard by the same MRPT_VERSION check used in FilterByRing.cpp and iterate
getPointFieldNames_uint16()/the older buffer accessor), or update the minimum
MRPT version in CMakeLists.txt to 2.15.4 so getPointsBufferRef_uint16_field()
can be used unconditionally (adjust any related
target_compile_definitions/version checks 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7c266aa-7000-49c8-8972-4f44494952aa
📒 Files selected for processing (6)
mp2p_icp_filters/include/mp2p_icp_filters/FilterDeskew.hmp2p_icp_filters/include/mp2p_icp_filters/Generator.hmp2p_icp_filters/src/FilterDecimateVoxels.cppmp2p_icp_filters/src/FilterDeskew.cppmp2p_icp_filters/src/Generator.cppmp2p_icp_map/src/pointcloud_sanity_check.cpp
💤 Files with no reviewable changes (2)
- mp2p_icp_filters/src/FilterDeskew.cpp
- mp2p_icp_filters/src/FilterDecimateVoxels.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
demos/sm2mm_pointcloud_voxelize_no_deskew.yaml (1)
7-8:⚠️ Potential issue | 🟡 MinorUpdate stale pipeline description comments to match the new generator block.
Line 7 and Line 8 still say generators are empty/default, but Lines 16-19 now define an explicit generator with
filterOutPointsAtZero. Please align the header comments with current behavior.✏️ Suggested doc fix
-# - Generators: empty, so the default generator is used (everything in one -# layer named 'raw' with all points). +# - Generators: explicit default `mp2p_icp_filters::Generator`, configured to +# keep points in 'raw' while discarding (0,0,0) organized-scan points.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/sm2mm_pointcloud_voxelize_no_deskew.yaml` around lines 7 - 8, Update the stale header comment that says "Generators: empty, so the default generator is used" to reflect the explicit generator block now present (the generator that sets filterOutPointsAtZero) — locate the YAML header comment near the top of demos/sm2mm_pointcloud_voxelize_no_deskew.yaml and reword it to describe that an explicit generator is defined with filterOutPointsAtZero (and any other generator settings present) so the comment matches the actual generator configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demos/sm2mm_no_decim_imu.yaml`:
- Around line 16-19: Update the outdated intro comments to reflect that this
demo now includes an explicit generator configuration (generators: - class_name:
mp2p_icp_filters::Generator with params.filterOutPointsAtZero: true) instead of
saying “generators are empty/default”; edit the top-of-file description to
mention that a mp2p_icp_filters::Generator is configured and that invalid/zero
points are filtered via filterOutPointsAtZero.
In `@demos/sm2mm_pointcloud_voxelize.yaml`:
- Around line 16-19: Update the top-of-file header comment to reflect that an
explicit generator is now configured (mp2p_icp_filters::Generator with
params.filterOutPointsAtZero=true) instead of saying generators are
empty/default; locate the YAML header comment and change its text to mention the
configured generator and the purpose of filterOutPointsAtZero (removes invalid
points in organized LiDAR scans) so the header and the generator block are
consistent.
---
Outside diff comments:
In `@demos/sm2mm_pointcloud_voxelize_no_deskew.yaml`:
- Around line 7-8: Update the stale header comment that says "Generators: empty,
so the default generator is used" to reflect the explicit generator block now
present (the generator that sets filterOutPointsAtZero) — locate the YAML header
comment near the top of demos/sm2mm_pointcloud_voxelize_no_deskew.yaml and
reword it to describe that an explicit generator is defined with
filterOutPointsAtZero (and any other generator settings present) so the comment
matches the actual generator configuration.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a89db044-6105-427d-a19d-f89ae23432d6
📒 Files selected for processing (11)
demos/sm2mm_bonxai_voxelmap.yamldemos/sm2mm_bonxai_voxelmap_gridmap.yamldemos/sm2mm_bonxai_voxelmap_gridmap_no_deskew.yamldemos/sm2mm_no_decim_imu.yamldemos/sm2mm_no_decim_imu_mls.yamldemos/sm2mm_no_decim_imu_mls_keyframe_map.yamldemos/sm2mm_pointcloud_voxelize.yamldemos/sm2mm_pointcloud_voxelize_no_deskew.yamldemos/sm2mm_voxels_static_dynamic_points.yamlmp2p_icp_filters/include/mp2p_icp_filters/Generator.hmp2p_icp_filters/src/Generator.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- mp2p_icp_filters/include/mp2p_icp_filters/Generator.h
- mp2p_icp_filters/src/Generator.cpp
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #53 +/- ##
===========================================
+ Coverage 78.60% 78.63% +0.02%
===========================================
Files 190 191 +1
Lines 10572 10632 +60
Branches 977 982 +5
===========================================
+ Hits 8310 8360 +50
- Misses 2262 2272 +10
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
demos/sm2mm_pointcloud_voxelize.yaml (1)
7-7:⚠️ Potential issue | 🟡 MinorHeader comment is stale and contradicts the active generator config.
Line 7 says the default generator is used, but Lines 15–18 define an explicit
mp2p_icp_filters::GeneratorwithfilterOutPointsAtZero: true.📝 Suggested fix
-# - Generators: default generator is used (creates an 'raw' layer). +# - Generators: explicit Generator for layer 'raw', with +# filterOutPointsAtZero=true to drop invalid (0,0,0) points.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/sm2mm_pointcloud_voxelize.yaml` at line 7, The header comment on Line 7 is stale because an explicit generator (mp2p_icp_filters::Generator) is configured further down with filterOutPointsAtZero: true; update or remove the header line that claims the "default generator is used" so it accurately reflects that an explicit mp2p_icp_filters::Generator is active (or change the YAML to actually rely on the default generator if that was intended).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@demos/sm2mm_pointcloud_voxelize.yaml`:
- Line 7: The header comment on Line 7 is stale because an explicit generator
(mp2p_icp_filters::Generator) is configured further down with
filterOutPointsAtZero: true; update or remove the header line that claims the
"default generator is used" so it accurately reflects that an explicit
mp2p_icp_filters::Generator is active (or change the YAML to actually rely on
the default generator if that was intended).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00aaf682-a2db-49f8-b9fa-9ccb6a752adb
📒 Files selected for processing (20)
CMakeLists.txtagents.mdapps/txt2mm/main.cppdemos/sm2mm_no_decim_imu.yamldemos/sm2mm_no_decim_imu_mls.yamldemos/sm2mm_pointcloud_voxelize.yamldemos/sm2mm_pointcloud_voxelize_no_deskew.yamlmp2p_icp_filters/src/FilterAdjustTimestamps.cppmp2p_icp_filters/src/FilterBoundingBox.cppmp2p_icp_filters/src/FilterByIntensity.cppmp2p_icp_filters/src/FilterByRange.cppmp2p_icp_filters/src/FilterByRing.cppmp2p_icp_filters/src/FilterCurvature.cppmp2p_icp_filters/src/FilterDecimateVoxels.cppmp2p_icp_filters/src/FilterDeskew.cppmp2p_icp_filters/src/FilterFartherPointSampling.cppmp2p_icp_filters/src/FilterPoleDetector.cppmp2p_icp_filters/src/FilterRemoveByVoxelOccupancy.cppmp2p_icp_filters/src/FilterSOR.cppmp2p_icp_filters/src/Generator.cpp
💤 Files with no reviewable changes (6)
- mp2p_icp_filters/src/FilterAdjustTimestamps.cpp
- mp2p_icp_filters/src/FilterDeskew.cpp
- mp2p_icp_filters/src/FilterFartherPointSampling.cpp
- mp2p_icp_filters/src/FilterCurvature.cpp
- mp2p_icp_filters/src/FilterSOR.cpp
- mp2p_icp_filters/src/FilterByIntensity.cpp
✅ Files skipped from review due to trivial changes (3)
- agents.md
- CMakeLists.txt
- demos/sm2mm_no_decim_imu_mls.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- demos/sm2mm_no_decim_imu.yaml
- demos/sm2mm_pointcloud_voxelize_no_deskew.yaml
- mp2p_icp_filters/src/FilterDecimateVoxels.cpp
jlblancoc
force-pushed
the
generator-generic-cloud
branch
from
cb21bdd to
59fd8ed
Compare
jlblancoc
force-pushed
the
generator-generic-cloud
branch
from
59fd8ed to
d48ceab
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
mp2p_icp_map/src/pointcloud_field_utils.cpp (1)
37-39: Rename loop variable typo for readability.
mameat Line 37 looks accidental; renaming tonameimproves clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mp2p_icp_map/src/pointcloud_field_utils.cpp` around lines 37 - 39, The loop variable `mame` in the range-for over `dst_names_org` is a typo and hurts readability; update the loop in pointcloud_field_utils.cpp to use a descriptive variable name (e.g., `name`) so the loop becomes `for (const auto& name : dst_names_org)` and adjust the `dst_names.emplace_back` call to use `name` accordingly.mp2p_icp_filters/src/FilterDecimateVoxels.cpp (1)
176-176: Apply field-padding mismatch warnings in allInsertCtxinitialization paths.Line 176 adds the warning in the bypass branch, but the regular voxel paths (Line 256/257, Line 271/272, Line 308/309) still initialize insertion contexts without the same warning. That leaves common mixed-field insertions silent.
Suggested patch
@@ auto& ctx = ctxs[pc]; if (!ctx.xs_src) { outPc->registerPointFieldsFrom(*pc); ctx = outPc->prepareForInsertPointsFrom(*pc); + mp2p_icp::warn_on_field_padding_mismatch(*pc, *outPc, *this); } @@ auto& ctx = ctxs[pc]; if (!ctx.xs_src) { outPc->registerPointFieldsFrom(*pc); ctx = outPc->prepareForInsertPointsFrom(*pc); + mp2p_icp::warn_on_field_padding_mismatch(*pc, *outPc, *this); } @@ outPc->registerPointFieldsFrom(pc); auto ctx = outPc->prepareForInsertPointsFrom(pc); + mp2p_icp::warn_on_field_padding_mismatch(pc, *outPc, *this);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mp2p_icp_filters/src/FilterDecimateVoxels.cpp` at line 176, The field-padding mismatch warning call mp2p_icp::warn_on_field_padding_mismatch(*pcPtrs[mapIdx], *outPc, *this) is only applied in the bypass branch; add the same call immediately before every other InsertCtx initialization so mixed-field insertions are warned. Locate every place InsertCtx is constructed in this file (the regular voxel insertion paths) and insert the warn_on_field_padding_mismatch(...) invocation with the same arguments (*pcPtrs[mapIdx], *outPc, *this) just prior to creating the InsertCtx object (the same relative spot where it’s present in the bypass branch). Ensure the call precedes the InsertCtx constructor in each path so the warning runs for all insertion flows.
🤖 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/FilterMLS.cpp`:
- Around line 619-632: The output layer reuse problem happens because existing
normal_x/normal_y/normal_z fields remain in outPc before calling
outPc->prepareForInsertPointsFrom(), so insertPointFrom() snapshots and
zero-pads them and later push_back() desynchronizes buffers; in FilterMLS,
detect if the destination point layer (from GetOrCreatePointLayer()) already
contains any "normal_x"/"normal_y"/"normal_z" fields and either remove/clear
those fields from outPc before calling prepareForInsertPointsFrom(), or
alternatively create a fresh layer when normals exist; ensure the check
references outPc, prepareForInsertPointsFrom(), registerField_float("normal_x")
and the push_back()/insertPointFrom() code paths and that firstIdx is computed
after clearing or recreating the layer so normals are not present in the
snapshot.
In `@mp2p_icp_map/include/mp2p_icp/pointcloud_field_utils.h`:
- Around line 32-52: warn_on_field_padding_mismatch currently treats an empty
source as missing fields and emits WARNs; update the function implementation in
pointcloud_field_utils.cpp to early-return false when the source cloud is empty
(e.g., check src.empty() or equivalent) before performing any field comparison
or logging so callers that skip insertion on empty input don't get spurious
warnings.
---
Nitpick comments:
In `@mp2p_icp_filters/src/FilterDecimateVoxels.cpp`:
- Line 176: The field-padding mismatch warning call
mp2p_icp::warn_on_field_padding_mismatch(*pcPtrs[mapIdx], *outPc, *this) is only
applied in the bypass branch; add the same call immediately before every other
InsertCtx initialization so mixed-field insertions are warned. Locate every
place InsertCtx is constructed in this file (the regular voxel insertion paths)
and insert the warn_on_field_padding_mismatch(...) invocation with the same
arguments (*pcPtrs[mapIdx], *outPc, *this) just prior to creating the InsertCtx
object (the same relative spot where it’s present in the bypass branch). Ensure
the call precedes the InsertCtx constructor in each path so the warning runs for
all insertion flows.
In `@mp2p_icp_map/src/pointcloud_field_utils.cpp`:
- Around line 37-39: The loop variable `mame` in the range-for over
`dst_names_org` is a typo and hurts readability; update the loop in
pointcloud_field_utils.cpp to use a descriptive variable name (e.g., `name`) so
the loop becomes `for (const auto& name : dst_names_org)` and adjust the
`dst_names.emplace_back` call to use `name` 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3562cd18-6943-4e5b-ad9a-f052a4716483
📒 Files selected for processing (12)
mp2p_icp_filters/src/FilterBoundingBox.cppmp2p_icp_filters/src/FilterByRange.cppmp2p_icp_filters/src/FilterDecimate.cppmp2p_icp_filters/src/FilterDecimateAdaptive.cppmp2p_icp_filters/src/FilterDecimateVoxels.cppmp2p_icp_filters/src/FilterDeskew.cppmp2p_icp_filters/src/FilterMLS.cppmp2p_icp_filters/src/FilterSOR.cppmp2p_icp_filters/src/FilterVoxelSOR.cppmp2p_icp_map/CMakeLists.txtmp2p_icp_map/include/mp2p_icp/pointcloud_field_utils.hmp2p_icp_map/src/pointcloud_field_utils.cpp
✅ Files skipped from review due to trivial changes (2)
- mp2p_icp_map/CMakeLists.txt
- mp2p_icp_filters/src/FilterByRange.cpp
Comment on lines
+32
to
+52
| /** Checks whether copying points from \a src into \a dst via | ||
| * CPointsMap::prepareForInsertPointsFrom() + insertPointFrom() would result | ||
| * in any destination field being zero-padded (i.e. the field exists in \a dst | ||
| * but is absent in \a src). | ||
| * | ||
| * This situation is benign on MRPT < 2.15.13 (those fields were simply | ||
| * skipped, which caused sanity-check crashes), and is | ||
| * handled correctly from MRPT 2.15.13 onwards by writing explicit zeros. | ||
| * However, silent zero-padding may still indicate a configuration problem, | ||
| * or sensor drops. | ||
| * | ||
| * One WARN-level message is emitted per mismatched field name. | ||
| * | ||
| * \param src Source point cloud (the one being inserted FROM). | ||
| * \param dst Destination point cloud (the one being inserted INTO). | ||
| * \param logger Logger used for WARN messages. | ||
| * \return True if at least one mismatch was found. | ||
| */ | ||
| bool warn_on_field_padding_mismatch( | ||
| const mrpt::maps::CPointsMap& src, const mrpt::maps::CPointsMap& dst, | ||
| const mrpt::system::COutputLogger& logger); |
There was a problem hiding this comment.
Skip warnings when the source cloud is empty.
The implementation currently treats an empty source buffer as “field absent”, so callers that return early on empty input can emit WARNs even though no insertPointFrom() will run. A fast if (src.empty()) return false; in pointcloud_field_utils.cpp would avoid noisy false positives here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mp2p_icp_map/include/mp2p_icp/pointcloud_field_utils.h` around lines 32 - 52,
warn_on_field_padding_mismatch currently treats an empty source as missing
fields and emits WARNs; update the function implementation in
pointcloud_field_utils.cpp to early-return false when the source cloud is empty
(e.g., check src.empty() or equivalent) before performing any field comparison
or logging so callers that skip insertion on empty input don't get spurious
warnings.
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
Documentation
Chores