Use strongly typed 'enum' instead of 'int' for properties in PCLVisualizer code
Right now functions like setPointCloudRenderingProperties use an int to represent the property to be modified. This means that there is no way of knowing which properties are accepted by the function. A solution is to use strongly typed enum instead of int for properties. The issue was brought up in #1668.
I could take this issue if its still relevant considering its been dormant since 2016.
That would be great I would say. Just self assign yourself to this issue to help us keep track of things.
@SergioRAgostinho I don't have write access so I cannot assign this to myself
You're right. In this case the only way of assigning a non member of the organization to the issue is if that person is the one responsible for opening the issue. In this case, we can't do much else.
Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.
Hello, Could i work on this one? Is just using enum class instead of using ints in function singatures?
Hello, Could i work on this one?
Sure, that would be great
Is just using enum class instead of using ints in function singatures?
On a quick look, I saw that there are already enums defined here, but each function only accepts some of those values. So maybe it makes sense to specify in the documentation of each function, which properties are accepted? Feel free to propose something, or maybe @larshg has an idea
I thought adding an assert or [[deprecated("reason")]] for property but it is already handled with corresponding pcl::console::print_error. I think updating the documentation of setPointCloudRenderingProperties and maybe turning RenderingProperties to enum class are enough.
Also, viewport is never used in any setPointCloudRenderingProperties and the setters in my eyes shall return [[noreturn]] bool or void.
Feel free to create a pull request with your proposed changes, then we can discuss them in detail :+1:
Feel free to create a pull request with your proposed changes, then we can discuss them in detail 👍
I created an initial PR. Please feel free to add your suggestions.
@mvieth: Could you please have a look to PR 5526?