pcl
pcl copied to clipboard
PointCloudLibrary
Use strongly typed 'enum' instead of 'int' for properties in PCLVisua…
…lizer code Fixes https://github.com/PointCloudLibrary/pcl/issues/1692
@theoniko Sorry for the late response, this pull request was kind of lost in my notifications list :frowning_face:
Changing enum RenderingProperties to enum class RenderingProperties is problematic if it changes the way the enum values can be used (additional scoping with RenderingProperties:: necessary). This would break many people's code, which we definitely want to avoid.
Similar problem for the removal of the parameter viewport: If we simply remove it but someone uses it (even if it does not do anything), they would get a compiler error. If we want to remove it for a function, we need to add a copy of that function where viewport still exists and has no default value, then deprecate this copy (see here for example), and call the function without viewport from the copy. You can decide whether you want to do this work, I am also fine with keeping viewport.
Maybe it would also make sense to state in the documentation which properties are accepted? For example that the functions with 3 double parameters only accepts PCL_VISUALIZER_COLOR? Yes, the print_error tells the user if a function is not used correctly, but it could be nice to see this directly in the function documentation.
All setters return some boolean flag which is never checked in code. I was thinking adding and attribute [[noreturn]] if the wrong behavior (false value return) could cause issues. If is not critical, without the attribute should also be fine.
You probably mean [[nodiscard]]? PCL currently still aims to be C++14 compatible, while nodiscard was added in C++17.
