Fix typo in docs of getMaxBoxCoordinates functions in voxel grid

[sumir0]

Edited: Fix typo in documentation of getMaxBoxCoordinates functions in voxel grid

[sumir0]

Basically, std::floor function returns the largest integer value not greater than the specified value. Therefore, I think it can make upper bounds of a calculated bounding box to be smaller than the actual bounding box of a cloud. Please, let me know if I am mistaken.

[mvieth]

Basically, std::floor function returns the largest integer value not greater than the specified value. Therefore, I think it can make upper bounds of a calculated bounding box to be smaller than the actual bounding box of a cloud.

I think that is why + Eigen::Vector4i::Ones () was added when calculating div_b_ (think of it as being added to max_b_ ). So I believe, the results with your changes are the same as before?

Also, there is a segmentation fault when running test_grsd_estimation , can you please investigate why that happens?

[sumir0]

I think that is why + Eigen::Vector4i::Ones () was added when calculating div_b_ (think of it as being added to max_b_ ). So I believe, the results with your changes are the same as before?

The results of filtering should be the same as before except for one thing. There is a function getMaxBoxCoordinates in VoxelGrid.

inline Eigen::Vector3i
getMaxBoxCoordinates () const { return (max_b_.head<3> ()); }

I believe this PR will change the results of that function.

I am not sure about + Eigen::Vector4i::Ones (). There are corner cases, for example when max_p[0] * inverse_leaf_size_[0] or min_p[0] * inverse_leaf_size_[0] are already integers, i.e. std::ceil and std::floor will return values without "rounding" (will return exactly the same values which were passed to those functions). Maybe those values were supposed to be in a cell following the last. For example, if a point is located exactly on the right border of a bounding box, maybe the bounding box was supposed to be one cell bigger to the right, so points from left borders will create centroids in cells that are located to the right from borders. But if I had left + Eigen::Vector4i::Ones () (and div_b_[3] = 0;) then it would have changed the results of filtering.

Probably, the explanation would have been better if I had made a picture. I am apologizing for that, I am a little bit short of time today.

Also, there is a segmentation fault when running test_grsd_estimation , can you please investigate why that happens?

Of course, I am pleased to help. I will investigate it tomorrow if there will be no updates.

[sumir0]

I have just noticed that with corner cases the results will not be the same. But if this PR does indeed fix the getMaxBoxCoordinates function then it might be worth looking into this better.

[sumir0]

After giving it some more thought I believe the getMaxBoxCoordinates function is supposed to produce the original results. As I see it now it is supposed to give maximum bounding box index coordinates. Therefore, it is supposed to std::floor values to actual indexes. My apologies for trouble. I will change the PR to include only documentation changes tomorrow.

[mvieth]

To be honest, I am not completely sure why a user would want/need to access max_b_ via getMaxBoxCoordinates since it is already converted to integer indices. So I do not know what value exactly a user would expect there. Strangely, the documentation describes it as "coordinates" as if it were xyz float coordinates (but it actually is indices of a voxel).
Did you have any issues with the calculation of max_b_ in practice? Or did you only look at the code and thought it is a potential problem? I found that one easily creates new bugs when trying to fix potential bugs (which may not even actually cause any real problem). So please understand that I am cautious when changing code based on a hypothetical.

Of course, either way, it is a good idea to improve the documentation of getMaxBoxCoordinates.

Also, there is a segmentation fault when running test_grsd_estimation , can you please investigate why that happens?

Of course, I am pleased to help. I will investigate it tomorrow if there will be no updates.

Just to avoid a potential misunderstanding: there is a segfault due to the changes in this pull request. That is why most of the CI checks are currently failing. On the master branch, this segfault does not happen.

[sumir0]

Did you have any issues with the calculation of max_b_ in practice? Or did you only look at the code and thought it is a potential problem?

I was analysing and looking into ways of improving voxel grid for use in scenarios where larger volumetric capacity is needed. There are open issues you might know about. I had just started going through all related issues and pull requests. And after some time I ended up here. What was just supposed to be a simple documentation fix became an overanalysis which I guess was not really necessary.

I found that one easily creates new bugs when trying to fix potential bugs (which may not even actually cause any real problem). So please understand that I am cautious when changing code based on a hypothetical.

Yes, it is same in my experience. I have not done such a newbie mistake for a long time. My apologies for that.

Just to avoid a potential misunderstanding: there is a segfault due to the changes in this pull request. That is why most of the CI checks are currently failing. On the master branch, this segfault does not happen.

Thank you for explanation. I guess that means the changes were indeed wrong.

As for the latest changes, I have not expanded documentation. I think some things should remain untouched.

[mvieth]

I was analysing and looking into ways of improving voxel grid for use in scenarios where larger volumetric capacity is needed.

About that: It would be great if we could expand the capacity of VoxelGrid with minimal changes. The main limiting factor is the use of unsigned int (typically 32 bit) for storing the index of the voxel of a point here. We could consider changing that to std::size_t, but we should make sure that 1) filtering does not get slower (especially the boost::sort::spreadsort::integer_sort might be affected), and 2) the memory requirements do not blow up.
However, I think even the potential range of the currently used unsigned int is not completely used: looking at voxel_grid.hpp, it uses int in several places where it could use unsigned int, and only casts to unsigned int later (e.g. https://github.com/PointCloudLibrary/pcl/blob/master/filters/include/pcl/filters/impl/voxel_grid.hpp#L717 ). So effectively, it seems that only half of the available value range is used.
Let us know if you are interested in doing some experiments on this topic!

[mvieth]

Thanks!