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 VoxelGridLarge - to handle larger point clouds / smaller leaf sizes #4385

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

MarkHedleyJones
Copy link

This pull request addresses #4365
It replaces the use of an int based voxel index variable with int64_t type.
As an example, this modification increaces the capacity of the filter (with a 3cm leaf size in each axis) from 38.7m³ to 62.9km³.

The change was made in a copy of the voxel_grid filter, called voxel_grid_large.
I did this to prevent negatively impacting any users of the original voxel_grid filter (for example, I don't know what the performance cost would be on embedded systems).

A summary of the performance of this change is:
show_pr

  • Presented processing times are averages taken from 50 executions (processed on an i7-4770 CPU)
  • Leaf too small warning indicates whether or not the original/master voxel_grid filter produced an error alerting the user that the leaf size was too small.
  • The original pcd file can be downloaded from sourceforge. It is Rf14.pcd within the kitchen.zip file.

Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

Hi @MarkHedleyJones Thanks for the PR and the amazing writeup

PCL is going in a different direction to support large point clouds. We are using pcl::index_t and allowing the users to configure the library using PCL_INDEX_SIZE and PCL_INDEX_SIGNED.

As such, would it be possible for you to instead change the VoxelFilter itself to use index_t instead of int?

In the current state, this PR duplicates a lot of code and is hard to review. I would not be in favor of merging this.

@kunaltyagi kunaltyagi added module: filters needs: author reply Specify why not closed/merged yet labels Sep 6, 2020
@kunaltyagi kunaltyagi linked an issue Sep 6, 2020 that may be closed by this pull request
@MarkHedleyJones
Copy link
Author

Hi @kunaltyagi
Thanks for your review.
I think that method of supporting various index types make a lot of sense.
I'll look at reworking what I've done to fit that instead.

@kunaltyagi kunaltyagi added needs: more work Specify why not closed/merged yet and removed needs: author reply Specify why not closed/merged yet labels Sep 9, 2020
@MarkHedleyJones
Copy link
Author

@kunaltyagi Hi. I've made some changes to the existing voxel_grid implementation in line with your suggestion of using PCL_INDEX_SIZE to set the relevant voxel_grid indicies, however I can't compile even the master branch when PCL_INDEX_SIZE = 64.
The errors are related to calls with unsupported types in kdtree
Are builds being tested against 64 bit indicies in CI?

@kunaltyagi
Copy link
Member

kunaltyagi commented Sep 19, 2020

No. That's a WIP. There are PRs to (#4350,#4407, #4408) add the feature slowly across PCL

Please make the changes in voxel grid, instead of in voxel_grid_large

@stale
Copy link

stale bot commented Dec 19, 2020

Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this.

@stale stale bot added the status: stale label Dec 19, 2020
@tin1254 tin1254 mentioned this pull request Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[filters] Increase the volumetric capacity of voxel_grid
2 participants