Fix usage of some deprecated cloud types#23
Conversation
📝 WalkthroughWalkthroughMultiple tests, tools, and filters migrate MRPT-specific point-map types to the generic CGenericPointsMap and replace MRPT-versioned field handling with unified POINT_FIELD_* constants and consistent register/insert field APIs across the codebase. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test-mp2p_deskew.cpp (2)
414-415: Inconsistent field name assertions.These assertions use hardcoded strings
"intensity"and"t", while the equivalent assertions inrun_deskew_in_sm2mm_test(lines 623-624) use thePOINT_FIELD_INTENSITYandPOINT_FIELD_TIMESTAMPconstants. This inconsistency could cause test failures if the constant values differ from the hardcoded strings.Proposed fix to use constants consistently
- ASSERT_EQUAL_(outFields.count("intensity"), 1); - ASSERT_EQUAL_(outFields.count("t"), 1); + ASSERT_EQUAL_(outFields.count(std::string(mrpt::maps::CPointsMap::POINT_FIELD_INTENSITY)), 1); + ASSERT_EQUAL_(outFields.count(std::string(mrpt::maps::CPointsMap::POINT_FIELD_TIMESTAMP)), 1);
559-598: Fix formatting to pass CI.The CI pipeline reports a clang-format violation in this code block. Run the formatter to fix the style:
clang-format --style=file -i tests/test-mp2p_deskew.cpp
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test-mp2p_FilterDecimateVoxels.cpptests/test-mp2p_deskew.cpp
🧰 Additional context used
🪛 GitHub Actions: CI clang-format
tests/test-mp2p_deskew.cpp
[error] 556-596: clang-format diff detected. Formatting does not match style. To fix, run '/usr/bin/clang-format-14 --style=file -i /home/runner/work/mp2p_icp/mp2p_icp/tests/test-mp2p_deskew.cpp'. Code Style does not match coding style.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build_docker (ubuntu:noble, rolling, true, true)
- GitHub Check: build_docker (ubuntu:noble, jazzy, true)
- GitHub Check: build_docker (ubuntu:noble, rolling, true, true)
- GitHub Check: build_docker (ubuntu:jammy, humble, false)
- GitHub Check: build_docker (ubuntu:noble, rolling, true)
- GitHub Check: build_docker (ubuntu:jammy, humble, true)
- GitHub Check: build_docker (ubuntu:noble, jazzy, false)
🔇 Additional comments (8)
tests/test-mp2p_deskew.cpp (4)
25-25: LGTM!The include directive correctly reflects the migration to
CGenericPointsMap.
50-50: LGTM!The default
outputMapClassis correctly updated to use the generic point map type.
241-284: LGTM!The function correctly migrates to
CGenericPointsMap:
- Proper field registration before point insertion
- Uses standard
POINT_FIELD_TIMESTAMPandPOINT_FIELD_INTENSITYconstants
623-624: LGTM!These assertions correctly use the standard
POINT_FIELD_INTENSITYandPOINT_FIELD_TIMESTAMPconstants for field validation.tests/test-mp2p_FilterDecimateVoxels.cpp (4)
23-23: LGTM!The include update correctly reflects the migration to
CGenericPointsMap.
47-48: Correct field insertion pattern.The sequence of
insertPointFastfollowed byinsertPointField_floatis the proper approach for populating point data with additional fields in the generic map.
196-198: LGTM!The validation correctly uses the generic
hasPointFieldAPI to verify intensity data is preserved through the decimation filter.
34-36: Clean migration to the generic point map API.The pattern of creating a
CGenericPointsMapand explicitly registering the intensity field is the correct approach for moving away from deprecated specialized types. UsingPOINT_FIELD_INTENSITYconstant ensures forward compatibility and matches the established pattern throughout the test suite.
jlblancoc
force-pushed
the
fix/some-deprecations
branch
from
87d3d1a to
6980c3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/txt2mm/main.cpp (1)
30-36: Potential compile error on MRPT versions < 2.15.0.The includes at lines 30-36 conditionally include
<mrpt/maps/CGenericPointsMap.h>only forMRPT_VERSION >= 0x20f00, but thexyziformat handling (lines 164-181) unconditionally usesCGenericPointsMap::Create()andPOINT_FIELD_INTENSITY. This will cause a compile error on older MRPT versions.Either:
- Remove the version guard around the include and assume MRPT >= 2.15.0 is required, or
- Add version guards around the
xyziandxyzirthandling code blocks similar to howxyzrgbis handled.Option 1: Remove version guard if MRPT >= 2.15.0 is required
-#if MRPT_VERSION >= 0x20f00 // 2.15.0 #include <mrpt/maps/CGenericPointsMap.h> -#else -#include <mrpt/maps/CColouredPointsMap.h> -#include <mrpt/maps/CPointsMapXYZI.h> -#include <mrpt/maps/CPointsMapXYZIRT.h> -#endifAlso applies to: 161-181
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/mm2txt/main.cppapps/txt2mm/main.cppmp2p_icp_filters/src/FilterByIntensity.cppmp2p_icp_filters/src/FilterDecimateVoxels.cppmp2p_icp_filters/src/FilterNormalizeIntensity.cppmp2p_icp_filters/src/Generator.cpptests/test-mp2p_Filters.cpptests/test-mp2p_deskew.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build_docker (ubuntu:noble, jazzy, false)
- GitHub Check: build_docker (ubuntu:jammy, humble, true)
- GitHub Check: build_docker (ubuntu:noble, rolling, true, true)
- GitHub Check: build_docker (ubuntu:noble, rolling, true)
- GitHub Check: build_docker (ubuntu:noble, rolling, true, true)
- GitHub Check: build_docker (ubuntu:jammy, humble, false)
- GitHub Check: build_docker (ubuntu:noble, jazzy, true)
🔇 Additional comments (14)
mp2p_icp_filters/src/FilterNormalizeIntensity.cpp (1)
65-65: LGTM! Consistent migration to POINT_FIELD_INTENSITY.The change aligns with the project-wide migration to using the public
POINT_FIELD_INTENSITYconstant, consistent with the changes in FilterByIntensity.cpp.tests/test-mp2p_Filters.cpp (3)
41-41: LGTM! Good practice to define a constant.Defining the
INTENSITYconstant fromPOINT_FIELD_INTENSITYimproves readability and maintainability throughout the test file.
207-212: LGTM! Cleaner field access pattern.The migration to using
POINT_FIELD_INTENSITYdirectly eliminates version-specific branching and improves code clarity.
618-618: LGTM! Explicit cast improves clarity.The explicit cast to
doublemakes the division operation clearer and prevents potential integer division issues.apps/mm2txt/main.cpp (1)
196-219: LGTM! Proper handling of deprecated types with backward compatibility.The version-conditional logic correctly handles deprecated point cloud types (
CPointsMapXYZIRT,CPointsMapXYZI) for MRPT < 3.0.0 while suppressing deprecation warnings with appropriate diagnostic pragmas. The fallback chain ensures all point types are handled.mp2p_icp_filters/src/FilterDecimateVoxels.cpp (1)
264-279: LGTM! Correct context-based insertion with field preservation.The change properly uses context-based point insertion to preserve all point fields during the flatten operation, then overrides only the z coordinate. The per-source context caching is efficient, and field registration on first use is the correct pattern.
mp2p_icp_filters/src/FilterByIntensity.cpp (1)
112-113: Code change is correct. ThePOINT_FIELD_INTENSITYconstant is available in MRPT 2.11.5+ (the project's minimum required version).The migration from version-guarded intensity retrieval to using the public
POINT_FIELD_INTENSITYconstant simplifies the code. This constant is used throughout the codebase without version guards in multiple filters and test files, confirming availability in the minimum supported MRPT version.mp2p_icp_filters/src/Generator.cpp (2)
33-33: LGTM! Header updated to use the generic point map type.The include change aligns with the migration from
CPointsMapXYZIRTtoCGenericPointsMap.
186-200: Migration toCGenericPointsMapis consistent across type creation, cast, and assertion.The code change from
CPointsMapXYZIRTtoCGenericPointsMapis properly applied. However, the assumption thatCObservationVelodyneScan::insertObservation()automatically registers the required point fields (intensity, ring, timestamp) depends on MRPT library implementation details that cannot be verified within this repository. This should be confirmed against MRPT's source or documentation to ensure the Velodyne scan data is correctly integrated with all required fields.apps/txt2mm/main.cpp (1)
183-211: Field registration and insertion logic is correct.The
xyzirtformat correctly:
- Registers all three fields (intensity, ring ID, timestamp) before populating data
- Casts ring ID to
uint16_tappropriately- Maintains consistent order between field registration and insertion
Note: The same version guard issue flagged above applies to this code block as well.
tests/test-mp2p_deskew.cpp (4)
25-25: LGTM! Include updated for generic point map.
241-285: LGTM! Function migrated correctly to useCGenericPointsMap.The return type, creation, field registration, and field insertion are all consistently updated to use the generic point map type and
POINT_FIELD_*constants.
414-418: LGTM! Field existence assertions updated to usePOINT_FIELD_*constants.The assertions correctly verify that the deskewed output preserves both intensity and timestamp fields.
658-663: Version-conditional test coverage for both map types.The version guard correctly includes
CPointsMapXYZIRTonly for MRPT versions before 3.0.0 (where it will presumably be removed), whileCGenericPointsMapis always tested. This ensures backward compatibility testing while preparing for the deprecation.Note: This implies a minimum MRPT version requirement of 2.15.0 for the test to compile, since
CGenericPointsMapis used unconditionally elsewhere in this file.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/txt2mm/main.cpp:
- Around line 187-189: The registrations using POINT_FIELD_RING_ID and
POINT_FIELD_TIMESTAMP suffer the same MRPT-version compatibility issue as
POINT_FIELD_INTENSITY; update the code to use the same fix applied for
intensity: either wrap the calls to
pts->registerField_uint16(...POINT_FIELD_RING_ID) and
pts->registerField_float(...POINT_FIELD_TIMESTAMP) in the same
preprocessor/version check or replace them with the version-agnostic
registration API used for intensity (e.g., registerFieldByName or the compatible
constants), and apply the same change to the other occurrences around the
201-207 range so all three fields use the identical compatibility approach.
- Line 165: The code calls pts->registerField_float with
mrpt::maps::CGenericPointsMap::POINT_FIELD_INTENSITY which was added in MRPT
2.15.3, but the current include guard allows MRPT >= 2.15.0; either tighten the
guard to require MRPT 2.15.3 (change the MRPT_VERSION check to 0x20f03) or
replace uses of POINT_FIELD_INTENSITY with the compatibility fallback used in
mp2p_icp_filters/FilterBase.h (conditional compile to a string literal name when
POINT_FIELD_INTENSITY is unavailable) so pts->registerField_float compiles on
older MRPT 2.15.x versions.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/txt2mm/main.cppmp2p_icp_filters/include/mp2p_icp_filters/FilterBase.hmp2p_icp_filters/src/FilterByIntensity.cppmp2p_icp_filters/src/FilterNormalizeIntensity.cpptests/test-mp2p_FilterByExpression.cpptests/test-mp2p_FilterDecimateVoxels.cpptests/test-mp2p_Filters.cpptests/test-mp2p_deskew.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- mp2p_icp_filters/src/FilterNormalizeIntensity.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build_docker (ubuntu:jammy, humble, true)
- GitHub Check: build_docker (ubuntu:noble, rolling, true, true)
- GitHub Check: build_docker (ubuntu:noble, rolling, true, true)
- GitHub Check: build_docker (ubuntu:noble, rolling, true)
- GitHub Check: build_docker (ubuntu:noble, jazzy, true)
- GitHub Check: build_docker (ubuntu:noble, jazzy, false)
- GitHub Check: build_docker (ubuntu:jammy, humble, false)
🔇 Additional comments (14)
mp2p_icp_filters/src/FilterByIntensity.cpp (1)
112-113: LGTM! Clean simplification.The removal of version-specific branches in favor of the centralized
POINT_FIELD_INTENSITYconstant improves maintainability. The version handling is now centralized inFilterBase.h.tests/test-mp2p_FilterByExpression.cpp (2)
57-57: LGTM! Consistent with the centralized field constants.The migration to
POINT_FIELD_INTENSITYaligns with the PR objective of removing MRPT version-specific references.
91-91: LGTM! Consistent field constant usage.Using the unqualified
POINT_FIELD_RING_IDis correct here due to theusing namespace mp2p_icp_filters;directive at line 27.Also applies to: 96-96
tests/test-mp2p_Filters.cpp (3)
41-41: LGTM! Centralized field constant.The local alias for
POINT_FIELD_INTENSITYprovides clarity and consistency throughout the test file.
207-209: LGTM! Consistent field access.The migration to
POINT_FIELD_INTENSITYremoves hardcoded field name strings and aligns with the centralized constant approach.
615-615: Nice improvement with explicit cast.The explicit
static_cast<double>ensures floating-point division and improves code clarity compared to implicit conversion.mp2p_icp_filters/include/mp2p_icp_filters/FilterBase.h (2)
32-32: LGTM! Required for version checks.The inclusion of
<mrpt/version.h>is necessary to support the version-conditional constant definitions below.
121-131: Excellent backward compatibility approach.The version-conditional definition of field constants provides:
- Forward compatibility by aliasing to MRPT's built-in constants when available (>= 2.15.3)
- Backward compatibility via local string_view definitions for older MRPT versions
- Centralized field name management across the codebase
This enables the rest of the PR to use consistent, unqualified constant names while supporting a range of MRPT versions.
tests/test-mp2p_deskew.cpp (4)
241-249: LGTM! Clean migration to CGenericPointsMap.The function signature and implementation are correctly updated to use the generic point map type. The field registration using
POINT_FIELD_TIMESTAMPandPOINT_FIELD_INTENSITYconstants follows the new API pattern.
274-277: LGTM!The point insertion pattern correctly adds the point coordinates first, then populates the registered float fields using the new constants.
417-418: LGTM!The assertions correctly verify the presence of required fields using the new constants with appropriate
std::string()conversion.
661-666: Good backward compatibility handling.The version gating correctly includes
CPointsMapXYZIRTonly for MRPT versions prior to 3.0.0, ensuring the test suite works with both older and newer MRPT versions.tests/test-mp2p_FilterDecimateVoxels.cpp (2)
32-51: LGTM! Clean migration to CGenericPointsMap with field registration.The helper function correctly:
- Creates a
CGenericPointsMapinstance- Registers the intensity field before inserting points
- Uses the
POINT_FIELD_INTENSITYconstant consistently for both registration and insertion
196-198: LGTM!The output verification correctly uses
hasPointFieldwith thePOINT_FIELD_INTENSITYconstant to confirm the filter preserves the intensity field through the decimation process.
jlblancoc
force-pushed
the
fix/some-deprecations
branch
from
5bf191d to
27fe3ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/txt2mm/main.cpp (1)
40-43: Track removal of replicated constants when MRPT minimum version is updated.The constants are duplicated here temporarily until MRPT 2.15.3 becomes the minimum required version. While these standard field names are unlikely to change, the duplication could lead to inconsistencies.
Consider adding a TODO comment or tracking issue to remove these replicated definitions once the minimum MRPT version requirement is raised to 2.15.3+.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/txt2mm/main.cppmp2p_icp_filters/include/mp2p_icp_filters/FilterBase.hmp2p_icp_filters/src/FilterByIntensity.cppmp2p_icp_filters/src/FilterNormalizeIntensity.cpptests/test-mp2p_FilterByExpression.cpptests/test-mp2p_FilterDecimateVoxels.cpptests/test-mp2p_Filters.cpptests/test-mp2p_deskew.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test-mp2p_deskew.cpp (2)
mp2p_icp_filters/include/mp2p_icp_filters/FilterBase.h (1)
mp2p_icp_filters(39-91)mp2p_icp_filters/include/mp2p_icp_filters/sm2mm.h (1)
mp2p_icp_filters(34-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build_docker (ubuntu:noble, rolling, true)
- GitHub Check: build_docker (ubuntu:jammy, humble, false)
- GitHub Check: build_docker (ubuntu:jammy, humble, true)
- GitHub Check: build_docker (ubuntu:noble, rolling, true, true)
- GitHub Check: build_docker (ubuntu:noble, rolling, true, true)
- GitHub Check: build_docker (ubuntu:noble, jazzy, true)
- GitHub Check: build_docker (ubuntu:noble, jazzy, false)
🔇 Additional comments (19)
mp2p_icp_filters/src/FilterNormalizeIntensity.cpp (1)
65-65: LGTM! Consistent field access pattern.The change aligns with the unified field access approach using
POINT_FIELD_INTENSITY, removing version-specific conditional compilation. The error handling on lines 67-71 properly validates the field pointer.tests/test-mp2p_Filters.cpp (3)
41-41: LGTM! Consistent constant usage.The constant alias follows the same pattern as other test files, using the unified
POINT_FIELD_INTENSITYconstant.
207-209: LGTM! Unified field access approach.The changes replace hardcoded field names with the centralized
POINT_FIELD_INTENSITYconstant, improving consistency across the codebase.
615-615: LGTM! Ensures floating-point division.The explicit cast to
doubleensures floating-point division, preventing potential integer truncation. This correctly computes the average z-coordinate.apps/txt2mm/main.cpp (2)
170-183: LGTM! Unified field handling for xyzi format.The changes replace version-guarded field access with the unified
POINT_FIELD_INTENSITYconstant, simplifying the code and improving maintainability.
191-208: LGTM! Unified field handling for xyzirt format.The changes consistently use
POINT_FIELD_INTENSITY,POINT_FIELD_RING_ID, andPOINT_FIELD_TIMESTAMPconstants for both field registration and insertion, removing version-specific conditional branches.tests/test-mp2p_FilterByExpression.cpp (1)
91-96: POINT_FIELD_RING_ID is correctly accessible.The constant is defined in
FilterBase.hand available in test scope through the include chain: test file →FilterByExpression.h(line 25) →FilterBase.h(lines 124, 129). The unqualified usage at lines 91-96 is correct.mp2p_icp_filters/src/FilterByIntensity.cpp (1)
112-113: POINT_FIELD_INTENSITY constant is properly accessible across all MRPT versions.The constant is defined in FilterBase.h (included by FilterByIntensity.h) with appropriate version guards: for MRPT ≥ 2.15.3 it aliases
mrpt::maps::CPointsMap::POINT_FIELD_INTENSITY, and for earlier versions it defines the constant locally as"intensity". The code change is safe and correct.mp2p_icp_filters/include/mp2p_icp_filters/FilterBase.h (2)
32-32: LGTM!The addition of
<mrpt/version.h>is necessary to enable version-gated conditional compilation for the POINT_FIELD_* constants defined below.
121-131: The version-gated constant definitions correctly implement a safe compatibility layer. The fallback string literals ("intensity", "ring", "t") match the MRPT 2.15.3+ constants exactly, so there is no risk of subtle bugs when upgrading.tests/test-mp2p_deskew.cpp (6)
25-25: LGTM!The migration from
CPointsMapXYZIRTtoCGenericPointsMapaligns with the PR objectives of moving away from deprecated cloud types. The include and default parameter update are consistent with the changes throughout the file.Also applies to: 50-50
241-249: LGTM!The function signature and initialization properly migrate to
CGenericPointsMapwith explicit field registration using the new public constants. Theusing namespace mp2p_icp_filters;statement ensures the constants are accessible.
276-277: LGTM!Field insertion now uses the generic
insertPointField_floatAPI with the public constants, replacing version-specific methods. This ensures consistent behavior across MRPT versions.
304-304: LGTM!Adding
using namespace mp2p_icp_filters;at function scope provides access to the public constants while maintaining good namespace hygiene.Also applies to: 447-447
417-418: LGTM!The assertions now reference the public constants with
std::stringconversion, ensuring consistency with the field names used throughout the codebase. This prevents potential mismatches from string literals.Also applies to: 627-628
662-666: LGTM!The version-gated test matrix properly handles the deprecation of
CPointsMapXYZIRTin MRPT 3.0 while maintaining backward compatibility testing on older versions.CGenericPointsMapserves as the primary type going forward.tests/test-mp2p_FilterDecimateVoxels.cpp (3)
23-23: LGTM!The include migration from
CPointsMapXYZItoCGenericPointsMapaligns with the PR objectives of replacing deprecated point cloud types.
34-35: LGTM!The migration to
CGenericPointsMapwith explicit field registration and insertion using the publicPOINT_FIELD_INTENSITYconstant is correct and consistent with the broader API changes. The field must be registered before insertion, which is properly handled here.Also applies to: 48-48
196-197: LGTM!The validation correctly uses the generic map's
hasPointFieldAPI with the public constant to verify intensity field preservation after decimation.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #23 +/- ##
===========================================
+ Coverage 64.94% 64.96% +0.01%
===========================================
Files 156 156
Lines 7847 7860 +13
Branches 851 852 +1
===========================================
+ Hits 5096 5106 +10
- Misses 2751 2754 +3
🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
Tests
Apps
Filters
Chores
✏️ Tip: You can customize this high-level summary in your review settings.