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

Follow up better type on pcl::index_t #3682

Closed
jingyangcarl opened this issue Feb 27, 2020 · 2 comments · Fixed by #4935
Closed

Follow up better type on pcl::index_t #3682

jingyangcarl opened this issue Feb 27, 2020 · 2 comments · Fixed by #4935
Labels
kind: question Type of issue needs: feedback Specify why not closed/merged yet

Comments

@jingyangcarl
Copy link

jingyangcarl commented Feb 27, 2020

Hi, as described in PR #3651, and issue #3351, it's recommended to create a new indices type, say pcl::index_t, instead of the plain container std::vector, std::vectorstd::uint32_t, or other types.

When I went through here:

std::uint32_t cloud_row_step = static_cast<std::uint32_t> (sizeof (PointT) * cloud.width);

for (std::uint32_t row = 0; row < msg.height; ++row)

The iterator here is defined as std::uint32_t. I think if we want to have a better indices type, should the iterator here be modified to some flexible type as well?

@kunaltyagi @taketwo

@kunaltyagi
Copy link
Member

should the iterator here be modified to some flexible type as well

Technically, indices are not needed for input in IO. They are mostly for algorithms. We can use std::size_t for input and output.

Some points to consider:

  • std::size_t technically fails for multi-dimension data (each dimension's max index_type multiplied together can be more than max uint64)
  • common-use imaging sensors aren't even at 2^16 per dimension, so for images, 2^32 is ok (since the product for 2D will be 2^64, our max index size)
  • Beyond 2^64 should not be the topic right now. We'll cross the bridge when we have to (probably never)

@kunaltyagi kunaltyagi added the kind: question Type of issue label Feb 27, 2020
@stale
Copy link

stale bot commented May 19, 2020

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: question Type of issue needs: feedback Specify why not closed/merged yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants