[ProgressiveMorphologicalFilter] Max window size considered as int instead of double (number of cells rather than dimension)

[Alessio-Parmeggiani]

Describe the bug
In the progressive morphological filter implementation I see that there is the following snippet of code to compute the different window sizes to use.

pcl/segmentation/include/pcl/segmentation/impl/progressive_morphological_filter.hpp

Lines 85 to 92 in e8ed4be

	while (window_size < max_window_size_)
	{
	// Determine the initial window size.
	if (exponential_)
	window_size = cell_size_ * (2.0f * std::pow (base_, iteration) + 1.0f);
	else
	window_size = cell_size_ * (2.0f * (iteration+1) * base_ + 1.0f);

I think there is an error in how max_window_size is being considered.
From the code window_size is treated as a dimension quantity (e.g. meters) while max window size is stored as an int, as if it represents the number of cells rather than the maximum dimension of a window size. So for me it is wrong that they are compared directly.

Expected behavior

I would expect max_window_size to be a float or double like window_size and represent the maximum dimension of the window size.

Another possibility is to keep max_window_size as an int but consider it using something like:
while (window_size < max_window_size_ * cell_size)
so that dimensions are coherent.

Your Environment

• PCL Version 1.12 but the same is visible also in other versions

Other context
The same is true also for ApproximateProgressiveMorphologicalFilter

[mvieth]

Hi, thank you for reporting this. I agree that this is inconsistent, and it would make sense to make max_window_size a float in ProgressiveMorphologicalFilter. And improving the documentation of setMaxWindowSize and getMaxWindowSize would be great as well.
In ApproximateProgressiveMorphologicalFilter, max_window_size etc. unfortunately have a different meaning: window_size, despite being a float, represents the number of cells because it is not multiplied with cell_size. This inconsistency between ProgressiveMorphologicalFilter and ApproximateProgressiveMorphologicalFilter is annoying, but if we now change the code in ApproximateProgressiveMorphologicalFilter to give all variables the same meaning as in ProgressiveMorphologicalFilter, we would break the code of everyone who is using ApproximateProgressiveMorphologicalFilter and has adapted the value of max_window_size to their data. I think the best we can do is to keep max_window_size unchanged as an int in ApproximateProgressiveMorphologicalFilter, and perhaps improve the documentation to point out the difference to ProgressiveMorphologicalFilter.
If you like, feel free to open a pull request.

[Alessio-Parmeggiani]

Thank you for answering! All clear now!
I agree that changing this behavior could break the code that already used the function, maybe for future versions it could be possible to have a more consistent implementation.

I will certainly consider doing a PR to add at least some documentation to better explain how the parameters are used.