Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add voxel_min_points param for voxel filter (ROS2) #161

Merged
merged 4 commits into from
Mar 5, 2020

Conversation

akiroz
Copy link

@akiroz akiroz commented Mar 5, 2020

See #160

Copy link
Owner

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same items from ROS1

README.md Outdated
@@ -116,6 +116,7 @@ rgbd_obstacle_layer:
inf_is_valid: false #default false, for laser scans
clear_after_reading: true #default false, clear the buffer after the layer gets readings from it
voxel_filter: true #default off, apply voxel filter to sensor, recommend on
voxel_min_points: 10 #default 0, minimum points per voxel for voxel filter
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change this to 0.

@@ -154,6 +154,7 @@ void MeasurementBuffer::BufferROSCloud(
sor.setDownsampleAllData(false);
float v_s = static_cast<float>(_voxel_size);
sor.setLeafSize(v_s, v_s, v_s);
sor.setMinimumPointsNumberPerVoxel((unsigned int) _voxel_min_points);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use static_cast<unsigned int>(_voxel_min_points);

@SteveMacenski SteveMacenski merged commit cf9cd2c into SteveMacenski:eloquent-devel Mar 5, 2020
@nickvaras
Copy link

There's an issue with minimum points per voxel in PCL, so just wanted to confirm what I'd expect, which is that the feature added in this PR doesn't really have any effect when applied?? Unless it's been fixed in newer versions of PCL...

@SteveMacenski
Copy link
Owner

I'm not sure - I haven't tested this personally, @akiroz I would have thought did since they proposed it

@akiroz
Copy link
Author

akiroz commented Apr 9, 2021

We actually ended up not using this param in the end but I believe it's been fixed in PCL upstream and planned for the coming 1.12 release.

@SteveMacenski
Copy link
Owner

This appears to have potentially broken things per #187 and #199

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants