pcl icon indicating copy to clipboard operation
pcl copied to clipboard

Use strongly typed 'enum' instead of 'int' for properties in PCLVisualizer code

Open aecins opened this issue 9 years ago • 7 comments

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.

aecins avatar Aug 24 '16 14:08 aecins

I could take this issue if its still relevant considering its been dormant since 2016.

StephenNneji avatar Jan 07 '18 19:01 StephenNneji

That would be great I would say. Just self assign yourself to this issue to help us keep track of things.

SergioRAgostinho avatar Jan 09 '18 14:01 SergioRAgostinho

@SergioRAgostinho I don't have write access so I cannot assign this to myself

StephenNneji avatar Jan 09 '18 15:01 StephenNneji

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.

SergioRAgostinho avatar Jan 09 '18 17:01 SergioRAgostinho

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

stale[bot] avatar May 19 '20 18:05 stale[bot]

Hello, Could i work on this one? Is just using enum class instead of using ints in function singatures?

theoniko avatar Jan 07 '22 19:01 theoniko

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

mvieth avatar Jan 08 '22 10:01 mvieth

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.

theoniko avatar Nov 11 '22 17:11 theoniko

Feel free to create a pull request with your proposed changes, then we can discuss them in detail :+1:

mvieth avatar Nov 12 '22 16:11 mvieth

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.

theoniko avatar Dec 03 '22 18:12 theoniko

@mvieth: Could you please have a look to PR 5526?

theoniko avatar Dec 22 '22 08:12 theoniko