feat(crop_box_filter): simplify validation logic for input point cloud by interimadd · Pull Request #875 · autowarefoundation/autoware_core

interimadd · 2026-03-02T08:54:50Z

Description

Simplify the validation criteria for input point clouds and extract the validation logic into crop_box_filter.cpp.

Background

The original is_valid() method in CropBoxFilter checked whether the input PointCloud2 matched specific Autoware point type layouts (e.g., PointXYZIRCAEDT, PointXYZIRC). This was overly strict; the crop box filter only requires x, y, z as FLOAT32 fields and a consistent data size to function correctly.

Changes

src/crop_box_filter.hpp / crop_box_filter.cpp: Newly added.
- Extracted the validate_pointcloud2() logic.
crop_box_filter_node.cpp: Removed ~230 lines.
- Deleted is_valid() and all four is_data_layout_compatible_with_point_*() methods.
- Updated pointcloud_callback() to call validate_pointcloud2().
test/test_crop_box_filter.cpp: Added new unit tests for validate_pointcloud2().

How was this PR tested?

Clean build with colcon build --packages-select autoware_crop_box_filter succeeded.
All unit tests passed:
- test_crop_box_filter (new): 3/3 cases passed — accepts xyz-only, accepts XYZIRC, and rejects missing 'z'.
- test_crop_box_filter_node (existing): No regressions.

Notes for reviewers

The validation criteria are intentionally relaxed compared to the original. The previous code rejected any cloud whose layout did not exactly match specific types like PointXYZIRC. The new code only requires x, y, z as FLOAT32 fields, which is the minimum the filter needs. This makes the node compatible with a broader range of point cloud types.
There is only one function in crop_box_filter.hpp for now, but I plan to extract more crop box filter logic into it in a future pull request.

Interface changes

None.

…ve redundant functions Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

…ation logic Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

…improve clarity Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

…emoving redundant lambda function Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

…stamp handling in validation logic Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

…nt ownership Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

… xyz field and data size checks Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

github-actions · 2026-03-02T08:55:02Z

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

You've checked our contribution guidelines.
Your PR follows our pull request guidelines.
All required CI checks pass before marking the PR ready for review.

codecov · 2026-03-02T08:59:39Z

Codecov Report

❌ Patch coverage is 50.00000% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.34%. Comparing base (e232aec) to head (a977c76).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...g/autoware_crop_box_filter/src/crop_box_filter.cpp	53.84%	7 Missing and 5 partials ⚠️
...oware_crop_box_filter/src/crop_box_filter_node.cpp	33.33%	1 Missing and 1 partial ⚠️
...g/autoware_crop_box_filter/src/crop_box_filter.hpp	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #875      +/-   ##
==========================================
+ Coverage   50.16%   50.34%   +0.18%     
==========================================
  Files         356      358       +2     
  Lines       22892    22780     -112     
  Branches    10139    10120      -19     
==========================================
- Hits        11483    11468      -15     
+ Misses      10305    10205     -100     
- Partials     1104     1107       +3

Flag	Coverage Δ		*Carryforward flag
daily-humble	`50.69% <ø> (+0.18%)`	⬆️	Carriedforward from b1a37e9
daily-jazzy	`50.18% <ø> (+0.18%)`	⬆️	Carriedforward from b1a37e9
differential-humble	`50.24% <50.00%> (?)`
total	`50.38% <ø> (+0.19%)`	⬆️	Carriedforward from b1a37e9
total-humble	`50.73% <ø> (+0.18%)`	⬆️	Carriedforward from b1a37e9
total-jazzy	`50.21% <ø> (+0.18%)`	⬆️	Carriedforward from b1a37e9

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mojomex

I left some small nit-picky comments, but feel free to leave some un-addressed. Merge at your convenience.

mojomex · 2026-03-02T09:19:12Z

sensing/autoware_crop_box_filter/src/crop_box_filter.cpp

+namespace autoware::crop_box_filter
+{
+
+static ValidationResult validate_xyz_fields(const PointCloud2ConstPtr & cloud)


(nit) More idiomatic would be to pass a const& PointCloud2 or const PointCloud2 * const since the function does not care about ownership.

As you pointed out, shared_ptr was unnecessary.
I've updated the functions to use raw pointers instead.
81a0a30

This change also simplified the unit tests.
afabdda

mojomex · 2026-03-02T09:22:20Z

sensing/autoware_crop_box_filter/src/crop_box_filter.cpp

+
+static ValidationResult validate_data_size(const PointCloud2ConstPtr & cloud)
+{
+  if (cloud->width * cloud->height * cloud->point_step != cloud->data.size()) {


(optional / discussion) Not sure what PointCloud2 best practice is here. Technically, <= would be enough here. Foxglove still processes pointclouds where width*height*point_step < data.size() and shows a warning icon.

Thanks for the insightful feedback.
As long as data.size() exceeds cloud->width * cloud->height * cloud->point_step, the crop box filter should process the data without any issues. Thus, I think it's reasonable to allow such point clouds.
Fixed in: 2c225e2

mojomex · 2026-03-02T09:23:20Z

sensing/autoware_crop_box_filter/src/crop_box_filter.cpp

+{
+  if (cloud->width * cloud->height * cloud->point_step != cloud->data.size()) {
+    std::ostringstream oss;
+    oss << "Invalid PointCloud (data = " << cloud->data.size() << ", width = " << cloud->width


Suggested change

oss << "Invalid PointCloud (data = " << cloud->data.size() << ", width = " << cloud->width

oss << "Invalid PointCloud (data.size = " << cloud->data.size() << ", width = " << cloud->width

It looks more concrete.
addressed e48e8b9

mojomex · 2026-03-02T09:26:25Z

sensing/autoware_crop_box_filter/src/crop_box_filter.hpp

+namespace autoware::crop_box_filter
+{
+
+struct ValidationResult


(nit) Not sure if tl is easily accessible in autoware_crop_box_filter, but parts of Autoware already use tl::expected<T, E> as a backport of std::expected from C++23, which is more idiomatic.

Thank you for the suggestion.
I didn't know about tl::expected, so I'm glad to learn about it.
In this case, to keep the implementation simple, I would like to keep the current implementation. However, I will keep the concept of tl::expected in mind.

mojomex · 2026-03-02T09:28:36Z

sensing/autoware_crop_box_filter/test/test_crop_box_filter.cpp

+
+int main(int argc, char ** argv)
+{
+  rclcpp::init(0, nullptr);


rclcpp should not be needed here - message types work without rclcpp.
Once removed, the main function shouldn't be needed altogether since GTest usually auto-discovers tests.

Thank you for pointing that out!
I totally overlooked it.
It's addressed in: be22678

Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

sasakisasaki

Reviewed. Readability is now much higher now. Thank you!

I left some minor comments 👍

sasakisasaki · 2026-03-02T09:25:39Z

sensing/autoware_crop_box_filter/src/crop_box_filter.cpp

+namespace autoware::crop_box_filter
+{
+
+static ValidationResult validate_xyz_fields(const PointCloud2ConstPtr & cloud)


For making the role explicit 👍

Suggested change

static ValidationResult validate_xyz_fields(const PointCloud2ConstPtr & cloud)

static ValidationResult contains_xyz_fields(const PointCloud2ConstPtr & cloud)

Since the return value is ValidationResult, not the bool expected from a function called contains_..., I'm not sure on this one 🤔

Thank you for your comments!
In this case, validate_xyz_fields() checks if its fields have a data type of sensor_msgs::msg::PointField::FLOAT32. So I believe validate is more appropriate in this case.

sasakisasaki · 2026-03-02T09:30:01Z

sensing/autoware_crop_box_filter/src/crop_box_filter.cpp

+
+ValidationResult validate_pointcloud2(const PointCloud2ConstPtr & cloud)
+{
+  const ValidationResult xyz_validation_result = validate_xyz_fields(cloud);


If above my proposal is accepted.

Suggested change

const ValidationResult xyz_validation_result = validate_xyz_fields(cloud);

const ValidationResult xyz_validation_result = contains_xyz_fields(cloud);

sasakisasaki · 2026-03-02T09:39:55Z

sensing/autoware_crop_box_filter/src/crop_box_filter.cpp

+    else if (field.name == "z")
+      has_z = true;
+  }


Although the number of loops is not huge $\mathcal{O}(n_{fields})$, I guess early return would make sense 👍

Suggested change

else if (field.name == "z")

has_z = true;

}

else if (field.name == "z")

has_z = true;

if (has_x && has_y && has_z) break;

}

Thank you for suggestion!
It looks better in the perspective of this loop's objective.
Addressed in b1a37e9

…tCloud2 Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

…g shared_ptr usage Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

…er than comparison Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

…elds in PointCloud2 Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

…filter.cpp Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

Takahisa.Ishikawa added 9 commits March 2, 2026 14:54

feat(crop_box_filter): simplify point cloud validation logic and remo…

0c3413c

…ve redundant functions Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

feat(crop_box_filter): remove dependency on rclcpp::Logger from valid…

95072fa

…ation logic Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

refactor(crop_box_filter): rename validation function

41d7c7d

Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

refactor(crop_box_filter): simplify point cloud validation tests and …

c1c4b92

…improve clarity Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

refactor(crop_box_filter): simplify point cloud validation logic by r…

dba8fed

…emoving redundant lambda function Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

refactor(crop_box_filter): separate validation logic into its own files

01ccbd9

Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

refactor(crop_box_filter): remove rclcpp dependency and simplify time…

8d5a6cb

…stamp handling in validation logic Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

chore(crop_box_filter): update copyright information to reflect curre…

a81cc82

…nt ownership Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

refactor(crop_box_filter): restructure validation logic by separating…

d6517c5

… xyz field and data size checks Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

interimadd added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Mar 2, 2026

interimadd requested review from a team, YoshiRi, amc-nu, badai-nguyen, drwnz, knzo25, kyoichi-sugahara, liuXinGangChina, meliketanrikulu, mojomex, vividf and yukkysaito as code owners March 2, 2026 08:54

sasakisasaki self-assigned this Mar 2, 2026

mojomex approved these changes Mar 2, 2026

View reviewed changes

refactor(test): remove rclcpp dependency from test file

be22678

Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

sasakisasaki approved these changes Mar 2, 2026

View reviewed changes

Takahisa.Ishikawa added 2 commits March 2, 2026 19:34

refactor(crop_box_filter): update validation functions to accept Poin…

81a0a30

…tCloud2 Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

refactor(crop_box_filter): improve error message comment

e48e8b9

Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

Takahisa.Ishikawa added 3 commits March 2, 2026 19:46

refactor(crop_box_filter): simplify PointCloud2 test cases by removin…

afabdda

…g shared_ptr usage Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

feat(crop_box_filter): update data size validation logic to use great…

2c225e2

…er than comparison Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

refactor(crop_box_filter): streamline validation logic for x, y, z fi…

b1a37e9

…elds in PointCloud2 Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

interimadd enabled auto-merge (squash) March 2, 2026 11:26

interimadd disabled auto-merge March 2, 2026 11:27

refactor(crop_box_filter): remove unused memory header from crop_box_…

a977c76

…filter.cpp Signed-off-by: Takahisa.Ishikawa <takahisa.ishikawa@tier4.jp>

interimadd enabled auto-merge (squash) March 2, 2026 11:28

interimadd merged commit d091381 into autowarefoundation:main Mar 2, 2026
42 of 43 checks passed

interimadd deleted the refactor-simplify-crop-box-filter-validation-logic branch March 2, 2026 11:53

	oss << "Invalid PointCloud (data = " << cloud->data.size() << ", width = " << cloud->width
	oss << "Invalid PointCloud (data.size = " << cloud->data.size() << ", width = " << cloud->width

	static ValidationResult validate_xyz_fields(const PointCloud2ConstPtr & cloud)
	static ValidationResult contains_xyz_fields(const PointCloud2ConstPtr & cloud)

	const ValidationResult xyz_validation_result = validate_xyz_fields(cloud);
	const ValidationResult xyz_validation_result = contains_xyz_fields(cloud);

Conversation

interimadd commented Mar 2, 2026

Description

Background

Changes

Related links

How was this PR tested?

Notes for reviewers

Interface changes

Uh oh!

github-actions bot commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mojomex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sasakisasaki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Mar 2, 2026 •

edited

Loading

codecov bot commented Mar 2, 2026 •

edited

Loading