FilterRemovePointCloudField now accepts multiple fields#42
Merged
Conversation
📝 WalkthroughWalkthroughReplaced the singular Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@mp2p_icp_filters/src/FilterRemovePointCloudField.cpp`:
- Around line 103-110: Reformat the THROW_EXCEPTION call to satisfy
clang-format: adjust the indentation and line breaks in the block where you
check (!removed && params.throw_on_missing_field) so the THROW_EXCEPTION(
mrpt::format( "Field '%s' does not exist in layer '%s'", field_name.c_str(),
params.pointcloud_layer.c_str() ) ); call is split/formatted per project style
(preserve use of THROW_EXCEPTION and mrpt::format and the field_name /
params.pointcloud_layer arguments) — run clang-format on
FilterRemovePointCloudField.cpp to apply the exact spacing and line breaks.
- Around line 98-124: The loop currently calls pc->unregisterField(...) and may
remove earlier fields before THROW_EXCEPTION is raised, causing partial
modification; modify RemovePointCloudField logic to perform a pre-validation
pass when params.throw_on_missing_field is true: iterate params.field_names and
check existence (e.g., via an existence API on pc or by querying fields) and
throw via the same THROW_EXCEPTION if any are missing, before performing any
pc->unregisterField calls; otherwise (when not throwing) keep the current
single-pass behavior and the MRPT_LOG_DEBUG_STREAM messages for removed/ignored
fields.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@mp2p_icp_filters/src/FilterRemovePointCloudField.cpp`: - Around line 98-124: The loop currently calls pc->unregisterField(...) and may remove earlier fields before THROW_EXCEPTION is raised, causing partial modification; modify RemovePointCloudField logic to perform a pre-validation pass when params.throw_on_missing_field is true: iterate params.field_names and check existence (e.g., via an existence API on pc or by querying fields) and throw via the same THROW_EXCEPTION if any are missing, before performing any pc->unregisterField calls; otherwise (when not throwing) keep the current single-pass behavior and the MRPT_LOG_DEBUG_STREAM messages for removed/ignored fields.mp2p_icp_filters/src/FilterRemovePointCloudField.cpp (1)
98-124: Partial removal on failure whenthrow_on_missing_fieldis true.If
field_namescontains e.g.["a", "b", "c"]and field"b"is missing,"a"will have already been removed before the exception is thrown. This leaves the point cloud in a partially modified state. If this is intentional "fail-fast" behavior, consider documenting it. Otherwise, a two-pass approach (validate all fields exist first, then remove) would be safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mp2p_icp_filters/src/FilterRemovePointCloudField.cpp` around lines 98 - 124, The loop currently calls pc->unregisterField(...) and may remove earlier fields before THROW_EXCEPTION is raised, causing partial modification; modify RemovePointCloudField logic to perform a pre-validation pass when params.throw_on_missing_field is true: iterate params.field_names and check existence (e.g., via an existence API on pc or by querying fields) and throw via the same THROW_EXCEPTION if any are missing, before performing any pc->unregisterField calls; otherwise (when not throwing) keep the current single-pass behavior and the MRPT_LOG_DEBUG_STREAM messages for removed/ignored fields.
jlblancoc
force-pushed
the
feat/filter-remove-several-point-fields
branch
from
6c2deae to
df9eeca
Compare
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@mp2p_icp_filters/src/FilterRemovePointCloudField.cpp`:
- Around line 98-123: The loop in FilterRemovePointCloudField.cpp that calls
pc->unregisterField for each entry in params.field_names can leave the
point-cloud layer partially modified when params.throw_on_missing_field is true;
change the logic so that when params.throw_on_missing_field is true you first
iterate over params.field_names to check existence (e.g., call pc->hasField or a
non-mutating check) and throw if any are missing, and only after that perform
the removals with pc->unregisterField; alternatively implement a rollback on
failure by recording removed fields and re-registering them, but prefer the
pre-validation approach for simplicity and atomic semantics.
In `@tests/test-mp2p_FilterRemovePointCloudField.cpp`:
- Around line 270-311: Add a new unit test that exercises
FilterRemovePointCloudField when throw_on_missing_field is true and the provided
sequence in p["field_names"] contains both existing and non-existing field
names: create a points map (like the existing case 10), register a subset of
fields (e.g., "field1" and "field2"), build p with p["pointcloud_layer"]="raw",
p["field_names"] as a sequence including "field1" and a nonexistent name (e.g.,
"missing_field"), set p["throw_on_missing_field"]=true, call
filter.initialize(p) and then filter.filter(map), and assert that the filter
throws (or the behavior matches the documented partial-removal behavior) and
that existing fields were removed or preserved per the expected contract;
reference FilterRemovePointCloudField, initialize(p), filter(map), and
p["field_names"]/p["throw_on_missing_field"] when adding the test.
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@mp2p_icp_filters/src/FilterRemovePointCloudField.cpp`: - Around line 98-123: The loop in FilterRemovePointCloudField.cpp that calls pc->unregisterField for each entry in params.field_names can leave the point-cloud layer partially modified when params.throw_on_missing_field is true; change the logic so that when params.throw_on_missing_field is true you first iterate over params.field_names to check existence (e.g., call pc->hasField or a non-mutating check) and throw if any are missing, and only after that perform the removals with pc->unregisterField; alternatively implement a rollback on failure by recording removed fields and re-registering them, but prefer the pre-validation approach for simplicity and atomic semantics. In `@tests/test-mp2p_FilterRemovePointCloudField.cpp`: - Around line 270-311: Add a new unit test that exercises FilterRemovePointCloudField when throw_on_missing_field is true and the provided sequence in p["field_names"] contains both existing and non-existing field names: create a points map (like the existing case 10), register a subset of fields (e.g., "field1" and "field2"), build p with p["pointcloud_layer"]="raw", p["field_names"] as a sequence including "field1" and a nonexistent name (e.g., "missing_field"), set p["throw_on_missing_field"]=true, call filter.initialize(p) and then filter.filter(map), and assert that the filter throws (or the behavior matches the documented partial-removal behavior) and that existing fields were removed or preserved per the expected contract; reference FilterRemovePointCloudField, initialize(p), filter(map), and p["field_names"]/p["throw_on_missing_field"] when adding the test.mp2p_icp_filters/src/FilterRemovePointCloudField.cpp (1)
98-123: Partial removal on throw: early exception leaves map in an inconsistent state.When
throw_on_missing_fieldistrueand multiple fields are provided, if the second field is missing but the first was already successfully removed, the exception leaves the map partially modified. Depending on whether callers expect atomic all-or-nothing semantics, this could be surprising.Consider either:
- Validating all field names exist before removing any, or
- Documenting that removal is best-effort and fields are removed in order until an error occurs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mp2p_icp_filters/src/FilterRemovePointCloudField.cpp` around lines 98 - 123, The loop in FilterRemovePointCloudField.cpp that calls pc->unregisterField for each entry in params.field_names can leave the point-cloud layer partially modified when params.throw_on_missing_field is true; change the logic so that when params.throw_on_missing_field is true you first iterate over params.field_names to check existence (e.g., call pc->hasField or a non-mutating check) and throw if any are missing, and only after that perform the removals with pc->unregisterField; alternatively implement a rollback on failure by recording removed fields and re-registering them, but prefer the pre-validation approach for simplicity and atomic semantics.tests/test-mp2p_FilterRemovePointCloudField.cpp (1)
270-311: Good addition of multi-field removal test.Case 10 properly exercises the sequence path with mixed field types. Consider also adding a test case where
throw_on_missing_field=trueand a sequence contains a mix of existing and non-existing field names, to document/verify the partial-removal behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test-mp2p_FilterRemovePointCloudField.cpp` around lines 270 - 311, Add a new unit test that exercises FilterRemovePointCloudField when throw_on_missing_field is true and the provided sequence in p["field_names"] contains both existing and non-existing field names: create a points map (like the existing case 10), register a subset of fields (e.g., "field1" and "field2"), build p with p["pointcloud_layer"]="raw", p["field_names"] as a sequence including "field1" and a nonexistent name (e.g., "missing_field"), set p["throw_on_missing_field"]=true, call filter.initialize(p) and then filter.filter(map), and assert that the filter throws (or the behavior matches the documented partial-removal behavior) and that existing fields were removed or preserved per the expected contract; reference FilterRemovePointCloudField, initialize(p), filter(map), and p["field_names"]/p["throw_on_missing_field"] when adding the test.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #42 +/- ##
===========================================
+ Coverage 77.41% 77.49% +0.07%
===========================================
Files 185 185
Lines 9848 9883 +35
Branches 931 934 +3
===========================================
+ Hits 7624 7659 +35
Misses 2224 2224
🚀 New features to boost your workflow:
|
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
Behavior Changes
Tests